Refactor and clean up our ambiguity handling #4218
Conversation
The ambiguity map tracks the users which are using a single display name, so let's reflect that in the name.
poljar
force-pushed
the
poljar/refactor-ambiguity-handling
branch
from
fe8868b
to
4c376a1
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4218 +/- ##
==========================================
- Coverage 84.89% 84.85% -0.04%
==========================================
Files 272 272
Lines 29167 29167
==========================================
- Hits 24761 24750 -11
- Misses 4406 4417 +11 ☔ View full report in Codecov by Sentry. |
| entry.insert(old.display_name.clone(), old.users); | ||
| } | ||
|
|
||
| if let Some(new) = new_map { | ||
| entry.insert(new.display_name, new.users); | ||
| entry.insert(new.display_name.clone(), new.users); |
| } | ||
| } | ||
|
|
||
| async fn get_users_with_display_name( |
There was a problem hiding this comment.
Can you add a brief doc comment explaining what it does please?
| Some(display_name.unwrap_or_else(|| event.user_id().localpart().to_owned())) | ||
| let Some(Ok(old_event)) = old_event.map(|r| r.deserialize()) else { return Ok(None) }; | ||
|
|
||
| if matches!(old_event.membership(), Join | Invite) { |
There was a problem hiding this comment.
Should we include Knock here too?
| use MembershipState::*; | ||
|
|
||
| let old_event = if let Some(m) = changes.state.get(room_id).and_then(|events| { | ||
| events.get(&StateEventType::RoomMember)?.get(member_event.state_key().as_str()) | ||
| events.get(&StateEventType::RoomMember)?.get(new_event.state_key().as_str()) |
There was a problem hiding this comment.
Could we store new_event.state_key() into a local variable named user_id, and reuse it all over the place in this function? I think it would make reading this code much simpler.
| // We don't allow other users to set the display name, so if we | ||
| // have a more trusted version of the display | ||
| // name use that. | ||
| // We don't allow other users to set the display name, so if we have a more |
There was a problem hiding this comment.
(Same question above for Join | Invite, do we need to add | Knock?)
| }) | ||
| } | ||
|
|
||
| async fn get( |
There was a problem hiding this comment.
While you're here, can you add a brief doc comment too please?
| @@ -154,60 +154,76 @@ impl AmbiguityCache { | |||
| self.changes.entry(room_id.to_owned()).or_default().insert(event_id, change); | |||
| } | |||
|
|
|||
| async fn get( | |||
| &mut self, | |||
| async fn get_old_display_name( | |||
This is in preparation of some higher level user display name handling, thus the rename of our
DisplayNametype as well.No functional changes here.