Disambiguate displaynames #2918
Open
+176
−32
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
toger5
reviewed
Jan 3, 2025
| */ | ||
| public readonly memberDisplaynames$ = fromEvent( | ||
| this.matrixRTCSession, | ||
| MatrixRTCSessionEvent.MembershipsChanged, |
There was a problem hiding this comment.
Do we emit this if a display name changes?
Very minor but otherwise we will not update the memberDisplaynames if a user changes their name.
Comment on lines
+533
to
+544
| const rawDisplayName = member?.rawDisplayName; | ||
| if ( | ||
| shouldDisambiguate(rawDisplayName, member.userId, memberships, room) | ||
| ) { | ||
| displaynameMap.set( | ||
| livekitParticipantId, | ||
| `${member.rawDisplayName} (${member.userId})`, | ||
| ); | ||
| } else { | ||
| displaynameMap.set(livekitParticipantId, member.rawDisplayName); | ||
| } | ||
| } |
There was a problem hiding this comment.
Only this logic is really new. The rest is duplicated from mediaItems$ right?
I think one of the following could work:
- create a fn:
getRoomMemberFor(rtcMember)and use this here and for mediaItems. (It also would need to return the participant? - create a BehaviorSubject for
memberDisplaynames$and update it when updating themediaItems$
| @@ -54,15 +55,23 @@ vi.mock("@livekit/components-core"); | |||
|
|
|||
| const localRtcMember = mockRtcMembership("@carol:example.org", "CCCC"); | |||
| const aliceRtcMember = mockRtcMembership("@alice:example.org", "AAAA"); | |||
| const aliceDopplegangerRtcMember = mockRtcMembership( | |||
There was a problem hiding this comment.
Suggested change
| const aliceDopplegangerRtcMember = mockRtcMembership( | |
| const aliceDoppelgangerRtcMember = mockRtcMembership( |
| const bobRtcMember = mockRtcMembership("@bob:example.org", "BBBB"); | ||
| const daveRtcMember = mockRtcMembership("@dave:example.org", "DDDD"); | ||
|
|
||
| const alice = mockMatrixRoomMember(aliceRtcMember); | ||
| const alice = mockMatrixRoomMember(aliceRtcMember, { rawDisplayName: "Alice" }); | ||
| const aliceDoppleganger = mockMatrixRoomMember(aliceDopplegangerRtcMember, { |
There was a problem hiding this comment.
Suggested change
| const aliceDoppleganger = mockMatrixRoomMember(aliceDopplegangerRtcMember, { | |
| const aliceDoppelganger = mockMatrixRoomMember(aliceDopplegangerRtcMember, { |
There was a problem hiding this comment.
It makes more sense if you change/refactor this in an ide. All occurances of Doppleganger
| b: [aliceRtcMember], | ||
| c: [aliceRtcMember, aliceDopplegangerRtcMember], | ||
| d: [aliceRtcMember, aliceDopplegangerRtcMember, bobRtcMember], | ||
| e: [aliceDopplegangerRtcMember, bobRtcMember], |
There was a problem hiding this comment.
How hard would it be to add the other cases: LTR/RTL and display name formatted like an impersonation?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #2746
This adds a new observer for member displaynames and updates usages of the old hook to use the view model. I'm currently copying the
shouldDisambiguatefunction from Element Web as we want to operate on call members, not all room members (and the changes felt too specific to us to incorporate upstream).Either way, this works quite nicely and responsively.
That said, maybe this would be good to be included in
MatrixRTCSession?