feat[matrix-sdk-base]: add prev_content as a fallback in case of missing display name and avatar url (#6026)#6097
feat[matrix-sdk-base]: add prev_content as a fallback in case of missing display name and avatar url (#6026)#6097JoFrost wants to merge 12 commits intomatrix-org:mainfrom
Conversation
…ing display name and avatar url Signed-off-by: JoFrost <20685007+JoFrost@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6097 +/- ##
=======================================
Coverage 89.79% 89.80%
=======================================
Files 363 363
Lines 100499 100499
Branches 100499 100499
=======================================
+ Hits 90243 90249 +6
+ Misses 6718 6711 -7
- Partials 3538 3539 +1 ☔ View full report in Codecov by Sentry. |
bnjbvr
left a comment
bnjbvr
left a comment
There was a problem hiding this comment.
Thanks for writing the patch! Looks like it's on a great path. Would you be so kind as to include some new unit tests, that show the new behavior? They wouldn't pass in main, but would pass with the new changes you've incorporated in your PR.
| } else { | ||
| self.event.displayname_value() | ||
| // Profile cache first | ||
| // Not to fond of this double as_ref() chain, but it avoids an extra clone. |
There was a problem hiding this comment.
I don't think the second .as_ref() should be necessary; the compiler ought to call .as_original(&self), then the name is a deref so already a reference. Not sure what this .as_ref() call does?
There was a problem hiding this comment.
Because self.profile is an Arc<Option<_>>, I needed to borrow through both the Arc and the Option to access the inner value without moving it. However, I realized I overcomplicated this, so I reverted to the way it was before.
| p.as_original().and_then(|e| e.content.displayname.as_deref()) | ||
| } else { | ||
| self.event.displayname_value() | ||
| // Profile cache first |
There was a problem hiding this comment.
| // Profile cache first | |
| // Try from the cached profile first. |
| return Some(name); | ||
| } | ||
|
|
||
| // Then current event content |
There was a problem hiding this comment.
| // Then current event content | |
| // Then from the current event's content. |
| return Some(name); | ||
| } | ||
|
|
||
| // Last resort: prev_content |
There was a problem hiding this comment.
| // Last resort: prev_content | |
| // As a last resort, try the previous content. |
| p.as_original().and_then(|e| e.content.avatar_url.as_deref()) | ||
| } else { | ||
| self.event.avatar_url() | ||
| if let Some(url) = self |
There was a problem hiding this comment.
It would be sweet to:
- have the same kind of comments around here
- remove the extraneous
.as_ref()call below
|
All done, thanks for your feedbacks! @bnjbvr |
Signed-off-by: JoFrost <20685007+JoFrost@users.noreply.github.com>
|
I'm putting this on hold at the moment, because we've discovered this might be intentional, after all (sorry about that!). If a user has been kicked of a room (which materializes by a An alternative would be to not update timeline items predating the membership change, but this wouldn't address the moderation use case above. I'll chat with our internal UX folks on their opinions about this and will get back to this PR then. |
|
No worries! Thanks for letting me know. |
bnjbvr
left a comment
bnjbvr
left a comment
There was a problem hiding this comment.
Ok, I've had the possibility to discuss this internally, and we came to the following conclusion:
- we would like to maintain the latest known profile of a user, when they left (either because kicked or upon intentional leave)
- but we'd like a banned user to lose their profile (that's #6119)
As a result, we can't just use the prev_content in the room membership field, because that would make implementing the second bullet point impossible. Thus, I think we need a different approach here, but the good news is that it might be simple enough.
Here, we could append the condition "and it is not a leave membership change". With that, I think we ought to keep the desired behavior.
For bonus points, you can also fix #6119 by updating the condition at line 44, and add a test for it. But don't feel any obligation to do so, of course :-)
|
Actually that's cool, I can take care of it! It looks indeed very easy to handle, and I'll write an unit test for it! |
Signed-off-by: JoFrost <20685007+JoFrost@users.noreply.github.com>
|
I think this is good now. Banned users no longer resolve a profile in the timeline, while users who leave or get kicked still keep their last known profile. I was also able to remove the prev_content fallback entirely, and everything continues to behave correctly. |
…_profile_after_leaving comments
2 participants
Hello! This PR implements a potential fix for the issue #6026, following the investigation of the issue.
To briefly restate the context, during member sync, the SDK stores the full
m.room.memberevent, but display name resolution relies only onRoomMemberEventContent. In some cases (for example when a user leaves a room), the current content no longer contains the profile fields needed to compute a display name, even though they are still available inprev_content.This PR updates
RoomMember::display_nameto fall back toprev_contentwhen the current content lacks the required fields, allowing the last known display name to be used.This may not be the most idiomatic solution, but it seems to behave reliably in practice. The intent of this PR is to open discussion around whether this behavior is desirable and/or where it should be handled.
Signed-off-by: