Chore/cache contact details #10805
Chore/cache contact details #10805
Conversation
osmaczko
requested review from
a team,
borismelnik and
igor-sirotin
and removed request for
a team
Jenkins BuildsClick to see older builds (28)
|
igor-sirotin
left a comment
igor-sirotin
left a comment
There was a problem hiding this comment.
Nice! 👍
This implementation looks good to me, but I wonder if everything that we have in ContactDetails should actually be in status-go 🤔 E.g. mobile will have to duplicate the logic of name/optionalName.
| ## same image and name displayed everywhere in the app. | ||
| result.name = contactDto.userDefaultDisplayName() | ||
| result.optionalName = contactDto.userOptionalName() | ||
| if(contactDto.image.thumbnail.len > 0): |
There was a problem hiding this comment.
I know this code was here already, but just noticed that this condition looks quite suspicious and doesn't make sense to me. Didn't dig deepers, but looks smth like if a user removes profile image, we will not process the update.
There was a problem hiding this comment.
Yes, seems so 🤔 We would need to check if that's really the case.
| # of contacts. Be sure when you change any condition here. | ||
| let myPubKey = singletonInstance.userProfile.getPubKey() | ||
| let contacts = toSeq(self.contacts.values) | ||
| let contacts = toSeq(self.contacts.values).map(cd => cd.dto) |
There was a problem hiding this comment.
Maybe return seq[ContactsDetails] from this function?
There was a problem hiding this comment.
This function is called with getContacts in initContactRequestsModel.
And then for each contact we call createItemFromPublicKey which calls getContactDetails.
So we do have ContactDetails in the beginning, then switch it to ContactDto and eventually back to ContactDetails.
There was a problem hiding this comment.
👍 I will address that in a follow-up PR to keep this one compact and avoid potential conflicts.
jrainville
left a comment
jrainville
left a comment
There was a problem hiding this comment.
Looks good. Nice job.
Thank you for changing contactDetails.details to contactDetails.dto. IMO, it never made sense to have details inside something that is a already [contact]Details.
| trustStatus: TrustStatus.Trusted, | ||
| bio: self.settingsService.getBio(), | ||
| socialLinks: self.settingsService.getSocialLinks() | ||
| return self.constructContactDetails( |
There was a problem hiding this comment.
Make me wonder, can't we save our own ContactDetails in a cache? Maybe here or in the singleton. That way we don't have to reprocess it each time.
There was a problem hiding this comment.
We need to make sure to update the cache when we update our profile though. It can be done in another PR if you want. (just remove the commit and put it in another branch)
There was a problem hiding this comment.
Right, haven't thought of it 👍
| if(self.contacts.hasKey(pubkey)): | ||
| return self.contacts[pubkey] | ||
|
|
||
| result = self.fetchContact(pubkey) |
There was a problem hiding this comment.
I just realized that we may hit the DB here, so it's a possible case where we could have a freeze. That may explain why we still see freezes when switching.
No need to fix here, but something to consider.
There was a problem hiding this comment.
Haven't seen that as a culprit in profiled data, although yes, we should make all potential DB hits async. I would leave it as is for now, otherwise it will complicate logic of getting contact details a lot.
Good point. As far as I know mobile is using media server for color hash and we are drawing it by our own on qml scene. Other fields we can probably move to status-go's |
osmaczko
force-pushed
the
chore/cache-contact-details
branch
2 times, most recently
from
2eb2587 to
42489ed
Compare
Yeah, good plan 👍 |
osmaczko
force-pushed
the
chore/cache-contact-details
branch
from
42489ed to
67c98ea
Compare
osmaczko
force-pushed
the
chore/cache-contact-details
branch
from
67c98ea to
475eb58
Compare
5 participants
What does the PR do
closes: #10797
Affected areas