Refactor contacts models to have a single model, remove useless properties and improve updating #16667
Conversation
jrainville
requested review from
micieslak,
caybro,
alexjba and
a team
as code owners
Jenkins BuildsClick to see older builds (107)
|
jrainville
force-pushed
the
refactor/users-models
branch
from
bbd1b72
to
ddc2efe
Compare
There was a problem hiding this comment.
That's really nice to see all those simplifications (and alignments to the architecture) there. It's good move for sure. But I'm afraid that there some additional work to do because other places that are in a questionable state and depend on the changes introduced there.
For example in TransactionAddress.qml we have connections listening to onItemChanged on myContactsModel, receivedContactRequestsModel, sentContactRequestsModel. It won't work as intended anymore. In general calling getContactDetailsAsJson in that delegate is far from ideal solution and should be fixed. We need to think how to provide all the necessary data via the model instead of accessing them from the backend on a delegate level. It may trigger necessity of refactor of the whole TransactionDetailView unfortunately.
Something similar I see in Chat/stores/RootStore and ActivityNotificationMessage. It all leads to hated getContactDetailsAsJson which I'd live to see removed finally.
I can try to provide some support with those further refactors here if needed. I'm not sure now how big it actually is...
jrainville
force-pushed
the
refactor/users-models
branch
from
ddc2efe
to
1945a45
Compare
|
@micieslak damn I hadn't thought of the use of I'll try to see if there is a way to rely on the model instead. Your help and @alexjba 's would be appreciated, because we basically have to get rid of |
Sorry for the delayed response! |
|
@jrainville Please let me know if I can help with the migration to Please have a look on this Epic #11983. |
micieslak
force-pushed
the
refactor/users-models
branch
from
1945a45
to
3ff81f2
Compare
micieslak
force-pushed
the
refactor/users-models
branch
from
3ff81f2
to
a01c69a
Compare
micieslak
force-pushed
the
refactor/users-models
branch
2 times, most recently
from
1498831
to
0ebc1fb
Compare
micieslak
force-pushed
the
refactor/users-models
branch
3 times, most recently
from
b1048da
to
ebc4009
Compare
| readonly property int lastUpdated: d.contactDetails.lastUpdated ?? 0 | ||
| readonly property int lastUpdatedLocally: d.contactDetails.lastUpdatedLocally ?? 0 | ||
| readonly property string thumbnailImage: d.contactDetails.thumbnailImage ?? "" | ||
| readonly property string largeImage: d.contactDetails.largeImage ?? "" | ||
| readonly property bool isContactRequestReceived: d.contactDetails.isContactRequestReceived ?? false | ||
| readonly property bool isContactRequestSent: d.contactDetails.isContactRequestSent ?? false | ||
| readonly property bool isSyncing: d.contactDetails.isSyncing ?? false |
There was a problem hiding this comment.
Indeed. We don't use that property anywhere
micieslak
force-pushed
the
refactor/users-models
branch
from
ebc4009
to
7cf5db4
Compare
|
hey @micieslak , test is failing because no contact request on Pending tab
|
micieslak
force-pushed
the
refactor/users-models
branch
from
58b9d72
to
75b2364
Compare
|
@micieslak I fixed the issue. I took the easy way of just checking the model when we get an update whether the item is already there and if not, adding it. I tried modifying the service, but what ended up happening is that we called I think it's fine to react to |
|
thanks for jumping in @jrainville , lets see if tests pass now :) |
|
|
Sorry about that, I kinda renamed a lot of function without testing lol. I fixed it, cleaned up some more and tested for real |
|
tests are still failing on missing contact requests showing up. now its even not present in activity centre |
|
@jrainville so when i send contact request, the application crashes on receiver side |
Yeah sorry about that. I'm fixing that. I guess I'm still tired 😞 |
|
Done, it should be good now. (please please please let it be good 🙏 ) |
thank you! Lets see :) |
|
@jrainville thanks for fixes, the contact requests are now feeling alright. However current run will fail because of the random app crash, we will have to restart :/ |
There was a problem hiding this comment.
I reviewed all the code that includes @micieslak 's changes and LGTM. I can't approve my own PR though haha
| readonly property int lastUpdated: d.contactDetails.lastUpdated ?? 0 | ||
| readonly property int lastUpdatedLocally: d.contactDetails.lastUpdatedLocally ?? 0 | ||
| readonly property string thumbnailImage: d.contactDetails.thumbnailImage ?? "" | ||
| readonly property string largeImage: d.contactDetails.largeImage ?? "" | ||
| readonly property bool isContactRequestReceived: d.contactDetails.isContactRequestReceived ?? false | ||
| readonly property bool isContactRequestSent: d.contactDetails.isContactRequestSent ?? false | ||
| readonly property bool isSyncing: d.contactDetails.isSyncing ?? false |
There was a problem hiding this comment.
Indeed. We don't use that property anywhere
|
I switched this PR to High Risk since it touches so many places in the end. @glitchminer can you take a look at this PR and run some tests with regards to contacts, contact requests, banned members. Moving contacts from one state to the other, updating their properties, etc. |
i will launch autotests to check that, but banning users should be tested manually @glitchminer ,out autotest is blocked by the floating buttons on hover (cant click them ) |
thats done, only 1 test to send nft failed which i dont know yet how to fix properly cc @glitchminer |
|
In progress, will complete this tomorrow. @jrainville , are notifications for contact actions in scope for this, like when removing or blocking contact? |
(repeating for posterity) everything should work as it did before, so if they worked on master, then they should still work |
What does the PR do
Fixes #16549
Fixes #14964
Fixes #14966
This PR is in three parts. I recommend revieing per commit.
allContactsand use an Adaptor on the QML side to filter into the needed models.This cleans the Nim side a lot and makes applying updates to the contacts' model way simpler.
OptionalName and isSyncing were never used.
DefaultDisplayName was not really used and is actually a duplication of preferredDisplayName, so I replaced the limited usages of DefaultDisplayName by preferredDisplayName
We used to update contact items by removing them from the models and re-adding them. This is highly inefficient.
Instead, the proper way is to update only the values that changed.
Affected areas
Contacts models, so the contacts settings, but also the profile popups and context menus
Architecture compliance
My PR is consistent with this document: Status Desktop Architecture Guide
Screenshot of functionality (including design for comparison)
contacts-updates.webm
icontothumbnailImage. This is something we should refactor, but it seemed too risky for too little payoff. Basically,iconandthumbnailImageare both the same thing.Impact on end user
No direct impact. The memory usage should be a little lower and the performance a little better when contacts get updated
How to test
Risk
Tick one:
Worst case, some properties don't update until a reboot.