feat(wallet) save/load collectibles user handled state in DB #13903
Conversation
Jenkins BuildsClick to see older builds (70)
|
|
@caybro and @dlipicar I'm currently working on this task of redirecting the collectible settings from being stored in I'm planning to emit signals for save/load events and call the load/save of Could you please have a look at the current code and see if there is an easier way to do that that I might have missed? Thanks |
stefandunca
force-pushed
the
collectibles_settings_to_backend-13313
branch
4 times, most recently
from
da12104 to
840351e
Compare
stefandunca
force-pushed
the
collectibles_settings_to_backend-13313
branch
from
840351e to
aaf1e7a
Compare
stefandunca
force-pushed
the
collectibles_settings_to_backend-13313
branch
3 times, most recently
from
7b8a7a9 to
7c2c4fb
Compare
stefandunca
force-pushed
the
collectibles_settings_to_backend-13313
branch
2 times, most recently
from
cb79f45 to
73b3cdd
Compare
There was a problem hiding this comment.
In general I like the direction where this is going however...
I don't think it's desirable to merge this as-is at this point in the release cycle (during a bugfixing period) when this is clearly:
- new feature
- breaking change
- incomplete (deals with collectibles only)
stefandunca
force-pushed
the
collectibles_settings_to_backend-13313
branch
from
73b3cdd to
c211c82
Compare
stefandunca
force-pushed
the
collectibles_settings_to_backend-13313
branch
from
c211c82 to
8acdec6
Compare
There was a problem hiding this comment.
Really nice work, code LGTM!
Here my comments, there are just some questions or suggestions!
I cannot test the pr properly to ensure there are no regressions bc right now we have a bug on master that doesn't apply settings to the main wallet view. It should be solved in here: #14018 and it would be nice if you can rebase on top of that to be able to do some proper testing.
Then, related to the topic @caybro exposed, as I explained offline, we need to consider if this refactoring PR should be integrated on this estabilization phase or wait for the next milestone. Considering that the 2.28 rc branch will be create this Wednesday, maybe we can keep this pr open until the branch is created and integrate later, so this code will go to the 2.29 release. WDYT?
I don't really have strong opinion on it if we can do a full and proper test to ensure there are no regressions introduced and we also ensure the bug fixed in here #13984 is also integrated.
CC: @alexjba @micieslak
There was a problem hiding this comment.
I cannot test the pr properly to ensure there are no regressions bc right now we have a bug on master that doesn't apply settings to the main wallet view. It should be solved in here: #14018 and it would be nice if you can rebase on top of that to be able to do some proper testing.
I see a lot of parallel work on token master. I talked with you, @Cuteivist and reach out to @caybro and @dlipicar but still end-up now with parallel refactoring and major changes. Where can I find the work streams on token management (mainly managetokenscontroller.cpp and managetokensmodel.h) so that I could stream the rebase, review, regression test and not drag it forever?
Then, related to the topic @caybro exposed, as I explained offline, we need to consider if this refactoring PR should be integrated on this estabilization phase or wait for the next milestone. Considering that the 2.28 rc branch will be create this Wednesday, maybe we can keep this pr open until the branch is created and integrate later, so this code will go to the 2.29 release. WDYT?
In my testing the functionality was kept and worked fine both in management view and display model. I trust your judgement and will follow with rebase and conflict after the parallel redesign for later on.
I don't really have strong opinion on it if we can do a full and proper test to ensure there are no regressions introduced ...
Testing is waiting for a rebase, however it make no sense to rebase until we merge the all conflicting changes.
and we also ensure the bug fixed in here #13984 is also integrated.
At first glance that change is implementing group ordering and conflicting with my changes. So I expect a rebase to be costly. As discussed privately, I will follow your collective decision and wait for all new features and fixes be in master, then rebase and adapt to the new changes.
stefandunca
force-pushed
the
collectibles_settings_to_backend-13313
branch
from
8acdec6 to
6d69658
Compare
There was a problem hiding this comment.
Even though the code is still quite complicated I think it's a move into a right direction. ManageTokensController seems to have too many responsibilities inside and should be split into smaller chunks, excluding persistence layer is really good first step here.
I see some benefits if we could merge it asap - it may safe us from handling migrations from qsettings to db-base storage in the future.
At first glance allowing controller to decide when to load/save makes it more complex than it should be, maybe we could reconsider it in the future.
In the long term I think there is refactor needed, similar as the one done for Profile Showcase Management. CollectiblesView and AssetsView should not depend on the controller at all. The order/visibility info could be attached to the main model using LeftJoinModel making things much easier and cleaner. But I'm aware it requires deeper analysis bc of grouping and additional transformations needed bc of that. Probably GroupingModel should be finished first (#12683) to make it doable.
|
|
||
| // TODO #13313: call the assets controller for all events | ||
| onRequestSaveSettings: (jsonData) => { | ||
| // savingStarted() |
There was a problem hiding this comment.
why in this case this started/finished pair is not used?
There was a problem hiding this comment.
They are already done serialy in the internal saveToQSettings (as previously). The marker methods started/finished are just for external storage as discussed above.
I'd wait for the release branch on Wednesday before merging it, but no longer after that. It could land in 2.28.1 depending on how it will be branched out. |
|
this task is for 2.28 so we need to merge it before release branch cut. Other words - this change is needed for 2.28 |
|
@micieslak I can't reply to your question so I answer in a comment
The main reason is the inability to guarantee that a slot will answer to the save request, hence avoid unnecessary invalidation of the model (this way start and finish notify the controller of storage being active and mark the invalidation). Another consideration was to be explicit and not to risk an inconstancy in case of connection type between |
Why is it needed and what added value or fix does it bring to the user? |
|
@caybro no idea, i just checked the milestone for the task :) @iurimatias @alexandraB99 please clarify |
I totally agree, the implementation should be refactored with more separation of concerns in mind. I found it outside my task requirements and tried to do an initial quick implementation that escalated in too many long debugging sessions but good learnings. |
|
Tested after rebase and the "Apply to my wallet" worked at first albeit with a delay (it looked like the changes were reflected only after the wallet activity was refreshed?). Then after about 30 minutes of inactivity it stopped working again and i am unable to get it to apply the order again. Other than this i did not find any other issues here. |
|
I agree with @alexjba and @micieslak on landing this into
I don't see any risk if we can do a full testing and ensure the existing functionality is still working properly. |
stefandunca
force-pushed
the
collectibles_settings_to_backend-13313
branch
2 times, most recently
from
49a4aeb to
94556db
Compare
Add a separation layer for save/load/clear to ManageTokensModel so that we can save/load from external sources. The separate layer is composed of JSON as protocol, a set of signals and slots for interface. The implementation forwards data to current QSettings for storybook and nim controllers for the app. Updates #13313, #13312
It seems `%*` operator we use in the `rpc` generics doesn't use the `serializedFieldName` marker and `type` was serialized as `itemType` and value the enum name instead of `type` and integer value Updates: #13971
stefandunca
force-pushed
the
collectibles_settings_to_backend-13313
branch
from
94556db to
2536c66
Compare
|
@lukaszso I just rebased on latest master and fixed all found issues. Can you please give it another try? |
Closes #13313, #13971 and Updates #13312
Add a separation layer for save/load/clear to
ManageTokensModelso that we can save/load from external sources.The separate layer is composed of JSON as protocol, a set of signals and slots for interface. The implementation forwards data to current
QSettingsfor storybook and nim controllers for the app.Groups are not handled in this PR
Note: I auto formatted some C++ files by mistake using vscode, please use ignore spaces in diff for easier review :(