feat: Add new simplified model for profile showcase preferences #13708
Conversation
MishkaRogachev
force-pushed
the
feat/issue-13688-featprofileshowcase-simplified-nim-backend-for-profile-showcase-settings
branch
from
29e52ad
to
62c4b79
Compare
Jenkins BuildsClick to see older builds (73)
|
| @@ -157,6 +162,67 @@ method storeProfileShowcasePreferences(self: Module, | |||
| revealedAddresses | |||
| ) | |||
|
|
|||
| method saveProfileShowcasePreferences*(self: Module, showcase: ProfileShowcaseSaveData) = | |||
There was a problem hiding this comment.
@alexjba @noeliaSD @micieslak
this part looks dirty from my perspective, to simplify it we need to add some extra data to the initial json:
- wallet accounts metadata
- collectibles's raw identification fields
-and i expect some extra data for tokens (contractAddress and tokeId)
There was a problem hiding this comment.
the initial json means the json composed in qml when saving?
I find it ok if this data is really needed here, in saveProfileShowcasePreferences. My reasoning is that the profileShowcasePreferences only operates on visibility and position. If the metadata is needed on save operation, I find it ok to be collected on save as opposed to be carried around in other places where it's not immediatly needed.
But I don't really understand why this data needs to be collected here just to be forwarded to status-go. I'd expect status-go not to need accounts metadata for example when saving some accounts showcase preferences. IMO this data could be kept in sync by status-go internals.
Unless I'm missing something, I think it would be more or less the same if we're collecting this data in saveProfileShowcasePreferences or qml. The best solution IMO is for this data not to be needed on save. The profile showcase preferences is not meant to save accounts metadata, just some visibilities and positions.
There was a problem hiding this comment.
On the other hand, I see it's quite an overhead to parse the qml updates. The profile settings are going through the following changes:
---qml---
- model to string
---nim--- - string -> json
- json -> nim object 1
- nim object 1 -> nim object 2 (similar object)
- nim object 2 -> json
- json -> back to string
would your proposal help in simplifying this flow as well?
There was a problem hiding this comment.
@alexjba thanks for the detailed comment, let's try to figure out the best way to do it!
Yes, by json I meant the data that qml passes to nim for saving.
There is one thing about filling additional fields in status-go: these data are in different services and not all status-go api are available from messenger service. It is still possible to route them, but it is much more laborious than in nim. However, for profile showcase preferences this problem is not so noticeable. I can move wallet filling to go, and it will even be correct. But not everything is so easy with tokens. The most correct structure seems to me to be:
{
communities: [ { communityId: <string>, visibility: <int> }],
accounts: [ { address <string>, visibility: <int> }],
collectibles: [ { chainId: <int>, tokenId: <string>, contractAddress: <string>, visibility: <int> }],
verifiedTokens: [ { symbol: <string>, visibility: <int> }],
unverifiedTokens: [ { chainId: <int>, contractAddress: <string>, visibility: <int> }],
socialLinks: [ { url: <string>, text: <string>, visibility: <int> }]
}
I would prefer to use the same nim structure for both saving and querying profile showcase preferences from status-go. And it is possible to completely remove converying in the nim layer, but then the position and correct id names will have to be filled in qml.
There was a problem hiding this comment.
For verifiedTokens and unverifiedTokens I'd use the word assets instead of tokens to have more nomenclature consistency!
There was a problem hiding this comment.
@alexjba thanks for the detailed comment, let's try to figure out the best way to do it!
Yes, by json I meant the data that qml passes to nim for saving.
There is one thing about filling additional fields in status-go: these data are in different services and not all status-go api are available from messenger service. It is still possible to route them, but it is much more laborious than in nim. However, for profile showcase preferences this problem is not so noticeable. I can move wallet filling to go, and it will even be correct. But not everything is so easy with tokens. The most correct structure seems to me to be:
{ communities: [ { communityId: <string>, visibility: <int> }], accounts: [ { address <string>, visibility: <int> }], collectibles: [ { chainId: <int>, tokenId: <string>, contractAddress: <string>, visibility: <int> }], verifiedTokens: [ { symbol: <string>, visibility: <int> }], unverifiedTokens: [ { chainId: <int>, contractAddress: <string>, visibility: <int> }], socialLinks: [ { url: <string>, text: <string>, visibility: <int> }] }I would prefer to use the same nim structure for both saving and querying profile showcase preferences from status-go. And it is possible to completely remove converying in the nim layer, but then the position and correct id names will have to be filled in qml.
I'm not sure if I'm understanding correctly what you say here but if I'm not wrong for the UI to do the proper joins we will need to indentify each item by a key being this key the following corresponding data for each case:
- Communities -->
key=communityId - Accounts -->
key=walletAddress - VerifiedTokens (assets) -->
key=symbolin case of non-community ones but ahash(chainId, tokenId)or something (we need to look at theassetsModelfrom wallet) for community ones - UnverifiedTokens --> I really don't know about how these are treat in wallet
- Collectibles -->
key=uidvalue build from chainId, tokenId and contractAddress
I think that if there's the need of some key transformation, it should happen in nim and the UI should not know anything about the rest of params, just the key.
There was a problem hiding this comment.
I think that if there's the need of some key transformation, it should happen in nim and the UI should not know anything about the rest of params, just the key.
I think it's a good point to keep the nim specific logic in nim so that we don't have to duplicate it in qml as well.
MishkaRogachev
force-pushed
the
feat/issue-13688-featprofileshowcase-simplified-nim-backend-for-profile-showcase-settings
branch
from
76376aa
to
13969c0
Compare
src/app/modules/main/profile_section/profile/models/profile_save_data.nim
Outdated
Show resolved
Hide resolved
| profileShowcasePreferencesAssetsModel: ProfileShowcasePreferencesModel | ||
| profileShowcasePreferencesAssetsModelVariant: QVariant | ||
| profileShowcasePreferencesSocialLinksModel: ProfileShowcasePreferencesModel | ||
| profileShowcasePreferencesSocialLinksModelVariant: QVariant |
There was a problem hiding this comment.
Does it make sense to publish the social links related stuff when it's still TODO?
There was a problem hiding this comment.
Was waiting for status-im/status-go#4775, will update PR soon with social links soon
MishkaRogachev
force-pushed
the
feat/issue-13688-featprofileshowcase-simplified-nim-backend-for-profile-showcase-settings
branch
from
13969c0
to
6f3cf67
Compare
|
For reviewers: this PR is not for merging for now, it's a proposal |
| @@ -110,6 +110,9 @@ proc toJsonNode*(self: ProfileShowcaseCollectiblePreference): JsonNode = | |||
| "order": self.order, | |||
| } | |||
|
|
|||
| proc toCombinedCollectibleId*(self: ProfileShowcaseCollectiblePreference): string = | |||
There was a problem hiding this comment.
WDYT about adding a symmetric fromCombiledCollectibleId instead of spreading this formatting logic into other pieces of code?
There was a problem hiding this comment.
ill improve it soon, after merging current PRs
There was a problem hiding this comment.
ill improve it soon, after merging current PRs
After merging this PR?
| @@ -157,6 +162,67 @@ method storeProfileShowcasePreferences(self: Module, | |||
| revealedAddresses | |||
| ) | |||
|
|
|||
| method saveProfileShowcasePreferences*(self: Module, showcase: ProfileShowcaseSaveData) = | |||
There was a problem hiding this comment.
the initial json means the json composed in qml when saving?
I find it ok if this data is really needed here, in saveProfileShowcasePreferences. My reasoning is that the profileShowcasePreferences only operates on visibility and position. If the metadata is needed on save operation, I find it ok to be collected on save as opposed to be carried around in other places where it's not immediatly needed.
But I don't really understand why this data needs to be collected here just to be forwarded to status-go. I'd expect status-go not to need accounts metadata for example when saving some accounts showcase preferences. IMO this data could be kept in sync by status-go internals.
Unless I'm missing something, I think it would be more or less the same if we're collecting this data in saveProfileShowcasePreferences or qml. The best solution IMO is for this data not to be needed on save. The profile showcase preferences is not meant to save accounts metadata, just some visibilities and positions.
| @@ -157,6 +162,67 @@ method storeProfileShowcasePreferences(self: Module, | |||
| revealedAddresses | |||
| ) | |||
|
|
|||
| method saveProfileShowcasePreferences*(self: Module, showcase: ProfileShowcaseSaveData) = | |||
There was a problem hiding this comment.
On the other hand, I see it's quite an overhead to parse the qml updates. The profile settings are going through the following changes:
---qml---
- model to string
---nim--- - string -> json
- json -> nim object 1
- nim object 1 -> nim object 2 (similar object)
- nim object 2 -> json
- json -> back to string
would your proposal help in simplifying this flow as well?
| function storeProfileShowcasePreferences() { | ||
| root.profileModule.storeProfileShowcasePreferences() | ||
| } | ||
|
|
||
| function saveProfileShowcasePreferences(json) { |
There was a problem hiding this comment.
Just a reminder: Here we will need to add the identity tab data management.
There was a problem hiding this comment.
Not for now, because of profile image. Let's open an issue instead :)
There was a problem hiding this comment.
Will you open it? Thanks!
| @@ -157,6 +162,67 @@ method storeProfileShowcasePreferences(self: Module, | |||
| revealedAddresses | |||
| ) | |||
|
|
|||
| method saveProfileShowcasePreferences*(self: Module, showcase: ProfileShowcaseSaveData) = | |||
There was a problem hiding this comment.
For verifiedTokens and unverifiedTokens I'd use the word assets instead of tokens to have more nomenclature consistency!
| ProfileShowcasePreferencesItem* = object of RootObj | ||
| showcaseKey*: string | ||
| showcaseVisibility*: ProfileShowcaseVisibility | ||
| showcasePosition*: int |
There was a problem hiding this comment.
Do we then need the position? I think we talked about using the natural order at least in the UI, with the natural order it should be enough
There was a problem hiding this comment.
If we don't have plans to support diff changes, i can remove it. But just in case we will need it in the future it's worth having them in the model.
There was a problem hiding this comment.
@alexjba @micieslak wdyt?
There was a problem hiding this comment.
I think it's ok to have it in the preferences model. We already have code in the new models that depends on showcasePosition. IMO the contacts profile doesn't need the position and can use the natural order.
src/app/modules/main/profile_section/profile/models/profile_save_data.nim
Outdated
Show resolved
Hide resolved
|
|
||
| import app_service/service/profile/dto/profile_showcase_preferences | ||
|
|
||
| type ProfileShowcaseSaveEntry* = ref object of RootObj |
There was a problem hiding this comment.
Why do we need a different type than ProfileShowcasePreferencesItem in here?
There was a problem hiding this comment.
In case we have similar requirements to the model source item and json structure, it may work. But on the latest discussion stage they are a bit different.
There was a problem hiding this comment.
Which are the differences?
There was a problem hiding this comment.
fields naming and extra fields for collectibles
There was a problem hiding this comment.
Even if they are two identical structures, I'm against using the same type for models and for reflecting the json structure. It would be a time bomb.
There was a problem hiding this comment.
Even if they are two identical structures, I'm against using the same type for models and for reflecting the json structure. It would be a time bomb.
Probably I'm missing something but I'd opt for using same type of object when we are talking about the same data instead of creating different types each time we do different actions, otherwise we are acumulating types and types that indeed define the same things. However, you have a better view of the complete flow so I leave it up to you! It was just a suggestion! :)
I don't have strong opinion here, but it's better to have same naming as wallet team @dlipicar @alaibe @Khushboo-dev-cpp WDYT? |
|
One note: currently when saving the profile showcase the app is briefly freezing. Could we move the |
Yes! I will update the PR soon |
MishkaRogachev
force-pushed
the
feat/issue-13688-featprofileshowcase-simplified-nim-backend-for-profile-showcase-settings
branch
from
6f3cf67
to
4bcb6d6
Compare
MishkaRogachev
force-pushed
the
feat/issue-13688-featprofileshowcase-simplified-nim-backend-for-profile-showcase-settings
branch
2 times, most recently
from
a5c91a6
to
8177ced
Compare
MishkaRogachev
force-pushed
the
feat/issue-13688-featprofileshowcase-simplified-nim-backend-for-profile-showcase-settings
branch
3 times, most recently
from
391eb98
to
22b2ef7
Compare
MishkaRogachev
force-pushed
the
feat/issue-13688-featprofileshowcase-simplified-nim-backend-for-profile-showcase-settings
branch
from
22b2ef7
to
925c03b
Compare
MishkaRogachev
force-pushed
the
feat/issue-13688-featprofileshowcase-simplified-nim-backend-for-profile-showcase-settings
branch
from
925c03b
to
a4a32d5
Compare
| result.showcaseVisibility = visibility | ||
|
|
||
| type | ||
| ModelRole {.pure.} = enum |
There was a problem hiding this comment.
This model won't have the showcasePosition?
There was a problem hiding this comment.
i'll add it now
| @@ -110,6 +110,9 @@ proc toJsonNode*(self: ProfileShowcaseCollectiblePreference): JsonNode = | |||
| "order": self.order, | |||
| } | |||
|
|
|||
| proc toCombinedCollectibleId*(self: ProfileShowcaseCollectiblePreference): string = | |||
There was a problem hiding this comment.
ill improve it soon, after merging current PRs
After merging this PR?
@alexjba yes, i want refactor this to helpers function and use it in the wallet section too |
MishkaRogachev
force-pushed
the
feat/issue-13688-featprofileshowcase-simplified-nim-backend-for-profile-showcase-settings
branch
from
a4a32d5
to
1dda3b1
Compare
MishkaRogachev
force-pushed
the
feat/issue-13688-featprofileshowcase-simplified-nim-backend-for-profile-showcase-settings
branch
3 times, most recently
from
bbe75af
to
a337176
Compare
MishkaRogachev
force-pushed
the
feat/issue-13688-featprofileshowcase-simplified-nim-backend-for-profile-showcase-settings
branch
from
a337176
to
063ff3c
Compare
MishkaRogachev
force-pushed
the
feat/issue-13688-featprofileshowcase-simplified-nim-backend-for-profile-showcase-settings
branch
from
063ff3c
to
15c9bb0
Compare
|
@MishkaRogachev Found an issue while saving the collectibles. The status-go error is saying the owner account is not in showcase, but I see it there. Screen.Recording.2024-03-08.at.18.22.31.mov |
- Created JSON file according to new backend structure when saving. - Updated `dirty state` to sync writable and movable models when position is changed to have a better internal models sync. - Reenabled identity fields save. Closes #13799
Updated showcase limits with values coming from backend.
We can create task for next week! |
MishkaRogachev
deleted the
feat/issue-13688-featprofileshowcase-simplified-nim-backend-for-profile-showcase-settings
branch
Close #13688
Waits status-im/status-go#4854 (comment)
What does the PR do
Add simplified profile showcase preferences models for:
Add api for saving profile changes
JSon structure:
Affected areas
Profile showcase