From 0f2e67dd60a42adf57f64a86f07cc9792389ae76 Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Tue, 14 Jan 2025 14:46:39 +0000 Subject: [PATCH] Disambiguate displaynames (#2918) * Disambigute displaynames * Add test * fixup test functions * prettier * lint * Split displayname utils into own file and add tests. * Split out fixtures * Add more testcases for displayname calculation. * lint * Also listen for displayname changes. (I stand corrected!) * fix missing media tiles on missing member --- src/state/CallViewModel.test.ts | 160 +++++++++++++++++++++++++++++--- src/state/CallViewModel.ts | 107 ++++++++++++++------- src/state/MediaViewModel.ts | 40 +++----- src/tile/GridTile.tsx | 3 +- src/tile/SpotlightTile.tsx | 3 +- src/utils/displayname.test.ts | 118 +++++++++++++++++++++++ src/utils/displayname.ts | 81 ++++++++++++++++ src/utils/test-fixtures.ts | 45 ++++++++- src/utils/test.ts | 2 + 9 files changed, 482 insertions(+), 77 deletions(-) create mode 100644 src/utils/displayname.test.ts create mode 100644 src/utils/displayname.ts diff --git a/src/state/CallViewModel.test.ts b/src/state/CallViewModel.test.ts index 4ddee0c5b..78a0d7ceb 100644 --- a/src/state/CallViewModel.test.ts +++ b/src/state/CallViewModel.test.ts @@ -14,6 +14,7 @@ import { map, type Observable, of, + skip, switchMap, } from "rxjs"; import { type MatrixClient } from "matrix-js-sdk/src/matrix"; @@ -49,25 +50,39 @@ import { import { E2eeType } from "../e2ee/e2eeType"; import type { RaisedHandInfo } from "../reactions"; import { showNonMemberTiles } from "../settings/settings"; +import { + alice, + aliceDoppelganger, + aliceDoppelgangerId, + aliceDoppelgangerRtcMember, + aliceId, + aliceParticipant, + aliceRtcMember, + bob, + bobId, + bobRtcMember, + bobZeroWidthSpace, + bobZeroWidthSpaceId, + bobZeroWidthSpaceRtcMember, + daveRTL, + daveRTLId, + daveRTLRtcMember, + local, + localId, + localRtcMember, +} from "../utils/test-fixtures"; vi.mock("@livekit/components-core"); -const localRtcMember = mockRtcMembership("@carol:example.org", "CCCC"); -const aliceRtcMember = mockRtcMembership("@alice:example.org", "AAAA"); -const bobRtcMember = mockRtcMembership("@bob:example.org", "BBBB"); const daveRtcMember = mockRtcMembership("@dave:example.org", "DDDD"); -const alice = mockMatrixRoomMember(aliceRtcMember); -const bob = mockMatrixRoomMember(bobRtcMember); -const carol = mockMatrixRoomMember(localRtcMember); -const dave = mockMatrixRoomMember(daveRtcMember); +const carol = local; +const carolId = localId; +const dave = mockMatrixRoomMember(daveRtcMember, { rawDisplayName: "Dave" }); -const aliceId = `${alice.userId}:${aliceRtcMember.deviceId}`; -const bobId = `${bob.userId}:${bobRtcMember.deviceId}`; const daveId = `${dave.userId}:${daveRtcMember.deviceId}`; const localParticipant = mockLocalParticipant({ identity: "" }); -const aliceParticipant = mockRemoteParticipant({ identity: aliceId }); const aliceSharingScreen = mockRemoteParticipant({ identity: aliceId, isScreenShareEnabled: true, @@ -80,7 +95,9 @@ const bobSharingScreen = mockRemoteParticipant({ const daveParticipant = mockRemoteParticipant({ identity: daveId }); const roomMembers = new Map( - [alice, bob, carol, dave].map((p) => [p.userId, p]), + [alice, aliceDoppelganger, bob, bobZeroWidthSpace, carol, dave, daveRTL].map( + (p) => [p.userId, p], + ), ); export interface GridLayoutSummary { @@ -792,6 +809,127 @@ it("should show at least one tile per MatrixRTCSession", () => { }); }); +test("should disambiguate users with the same displayname", () => { + withTestScheduler(({ hot, expectObservable }) => { + const scenarioInputMarbles = "abcde"; + const expectedLayoutMarbles = "abcde"; + + withCallViewModel( + of([]), + hot(scenarioInputMarbles, { + a: [], + b: [aliceRtcMember], + c: [aliceRtcMember, aliceDoppelgangerRtcMember], + d: [aliceRtcMember, aliceDoppelgangerRtcMember, bobRtcMember], + e: [aliceDoppelgangerRtcMember, bobRtcMember], + }), + of(ConnectionState.Connected), + new Map(), + (vm) => { + // Skip the null state. + expectObservable(vm.memberDisplaynames$.pipe(skip(1))).toBe( + expectedLayoutMarbles, + { + // Carol has no displayname - So userId is used. + a: new Map([[carolId, carol.userId]]), + b: new Map([ + [carolId, carol.userId], + [aliceId, alice.rawDisplayName], + ]), + // The second alice joins. + c: new Map([ + [carolId, carol.userId], + [aliceId, "Alice (@alice:example.org)"], + [aliceDoppelgangerId, "Alice (@alice2:example.org)"], + ]), + // Bob also joins + d: new Map([ + [carolId, carol.userId], + [aliceId, "Alice (@alice:example.org)"], + [aliceDoppelgangerId, "Alice (@alice2:example.org)"], + [bobId, bob.rawDisplayName], + ]), + // Alice leaves, and the displayname should reset. + e: new Map([ + [carolId, carol.userId], + [aliceDoppelgangerId, "Alice"], + [bobId, bob.rawDisplayName], + ]), + }, + ); + }, + ); + }); +}); + +test("should disambiguate users with invisible characters", () => { + withTestScheduler(({ hot, expectObservable }) => { + const scenarioInputMarbles = "ab"; + const expectedLayoutMarbles = "ab"; + + withCallViewModel( + of([]), + hot(scenarioInputMarbles, { + a: [], + b: [bobRtcMember, bobZeroWidthSpaceRtcMember], + }), + of(ConnectionState.Connected), + new Map(), + (vm) => { + // Skip the null state. + expectObservable(vm.memberDisplaynames$.pipe(skip(1))).toBe( + expectedLayoutMarbles, + { + // Carol has no displayname - So userId is used. + a: new Map([[carolId, carol.userId]]), + // Both Bobs join, and should handle zero width hacks. + b: new Map([ + [carolId, carol.userId], + [bobId, `Bob (${bob.userId})`], + [bobZeroWidthSpaceId, `Bob (${bobZeroWidthSpace.userId})`], + ]), + }, + ); + }, + ); + }); +}); + +test("should strip RTL characters from displayname", () => { + withTestScheduler(({ hot, expectObservable }) => { + const scenarioInputMarbles = "ab"; + const expectedLayoutMarbles = "ab"; + + withCallViewModel( + of([]), + hot(scenarioInputMarbles, { + a: [], + b: [daveRtcMember, daveRTLRtcMember], + }), + of(ConnectionState.Connected), + new Map(), + (vm) => { + // Skip the null state. + expectObservable(vm.memberDisplaynames$.pipe(skip(1))).toBe( + expectedLayoutMarbles, + { + // Carol has no displayname - So userId is used. + a: new Map([[carolId, carol.userId]]), + // Both Dave's join. Since after stripping + b: new Map([ + [carolId, carol.userId], + // Not disambiguated + [daveId, "Dave"], + // This one is, since it's using RTL. + [daveRTLId, `evaD (${daveRTL.userId})`], + ]), + }, + ); + }, + ); + }); +}); + it("should rank raised hands above video feeds and below speakers and presenters", () => { withTestScheduler(({ schedule, expectObservable }) => { // There should always be one tile for each MatrixRTCSession diff --git a/src/state/CallViewModel.ts b/src/state/CallViewModel.ts index 0c3b80db3..a52fd9af4 100644 --- a/src/state/CallViewModel.ts +++ b/src/state/CallViewModel.ts @@ -1,5 +1,5 @@ /* -Copyright 2023, 2024 New Vector Ltd. +Copyright 2023, 2024, 2025 New Vector Ltd. SPDX-License-Identifier: AGPL-3.0-only Please see LICENSE in the repository root for full details. @@ -19,7 +19,8 @@ import { Track, } from "livekit-client"; import { - type Room as MatrixRoom, + RoomStateEvent, + type Room, type RoomMember, } from "matrix-js-sdk/src/matrix"; import { @@ -50,6 +51,7 @@ import { } from "rxjs"; import { logger } from "matrix-js-sdk/src/logger"; import { + type CallMembership, type MatrixRTCSession, MatrixRTCSessionEvent, } from "matrix-js-sdk/src/matrixrtc"; @@ -94,6 +96,7 @@ import { } from "../reactions"; import { observeSpeaker$ } from "./observeSpeaker"; import { shallowEquals } from "../utils/array"; +import { calculateDisplayName, shouldDisambiguate } from "../utils/displayname"; // How long we wait after a focus switch before showing the real participant // list again @@ -258,6 +261,7 @@ class UserMedia { participant: LocalParticipant | RemoteParticipant | undefined, encryptionSystem: EncryptionSystem, livekitRoom: LivekitRoom, + displayname$: Observable, handRaised$: Observable, reaction$: Observable, ) { @@ -270,6 +274,7 @@ class UserMedia { this.participant$.asObservable() as Observable, encryptionSystem, livekitRoom, + displayname$, handRaised$, reaction$, ); @@ -282,6 +287,7 @@ class UserMedia { >, encryptionSystem, livekitRoom, + displayname$, handRaised$, reaction$, ); @@ -333,6 +339,7 @@ class ScreenShare { participant: LocalParticipant | RemoteParticipant, encryptionSystem: EncryptionSystem, liveKitRoom: LivekitRoom, + displayname$: Observable, ) { this.participant$ = new BehaviorSubject(participant); @@ -342,6 +349,7 @@ class ScreenShare { this.participant$.asObservable(), encryptionSystem, liveKitRoom, + displayname$, participant.isLocal, ); } @@ -353,26 +361,26 @@ class ScreenShare { type MediaItem = UserMedia | ScreenShare; -function findMatrixRoomMember( - room: MatrixRoom, - id: string, -): RoomMember | undefined { - if (id === "local") - return room.getMember(room.client.getUserId()!) ?? undefined; - - const parts = id.split(":"); - // must be at least 3 parts because we know the first part is a userId which must necessarily contain a colon - if (parts.length < 3) { - logger.warn( - `Livekit participants ID (${id}) doesn't look like a userId:deviceId combination`, - ); - return undefined; - } +function getRoomMemberFromRtcMember( + rtcMember: CallMembership, + room: Room, +): { id: string; member: RoomMember | undefined } { + // WARN! This is not exactly the sender but the user defined in the state key. + // This will be available once we change to the new "member as object" format in the MatrixRTC object. + let id = rtcMember.sender + ":" + rtcMember.deviceId; - parts.pop(); - const userId = parts.join(":"); + if (!rtcMember.sender) { + return { id, member: undefined }; + } + if ( + rtcMember.sender === room.client.getUserId() && + rtcMember.deviceId === room.client.getDeviceId() + ) { + id = "local"; + } - return room.getMember(userId) ?? undefined; + const member = room.getMember(rtcMember.sender) ?? undefined; + return { id, member }; } // TODO: Move wayyyy more business logic from the call and lobby views into here @@ -456,6 +464,40 @@ export class CallViewModel extends ViewModel { }, ); + /** + * Displaynames for each member of the call. This will disambiguate + * any displaynames that clashes with another member. Only members + * joined to the call are considered here. + */ + public readonly memberDisplaynames$ = merge( + // Handle call membership changes. + fromEvent(this.matrixRTCSession, MatrixRTCSessionEvent.MembershipsChanged), + // Handle room membership changes (and displayname updates) + fromEvent(this.matrixRTCSession.room, RoomStateEvent.Members), + ).pipe( + startWith(null), + map(() => { + const displaynameMap = new Map(); + const { room, memberships } = this.matrixRTCSession; + + // We only consider RTC members for disambiguation as they are the only visible members. + for (const rtcMember of memberships) { + const matrixIdentifier = `${rtcMember.sender}:${rtcMember.deviceId}`; + const { member } = getRoomMemberFromRtcMember(rtcMember, room); + if (!member) { + logger.error("Could not find member for media id:", matrixIdentifier); + continue; + } + const disambiguate = shouldDisambiguate(member, memberships, room); + displaynameMap.set( + matrixIdentifier, + calculateDisplayName(member, disambiguate), + ); + } + return displaynameMap; + }), + ); + /** * List of MediaItems that we want to display */ @@ -485,25 +527,18 @@ export class CallViewModel extends ViewModel { ) => { const newItems = new Map( function* (this: CallViewModel): Iterable<[string, MediaItem]> { + const room = this.matrixRTCSession.room; // m.rtc.members are the basis for calculating what is visible in the call for (const rtcMember of this.matrixRTCSession.memberships) { - const room = this.matrixRTCSession.room; - // WARN! This is not exactly the sender but the user defined in the state key. - // This will be available once we change to the new "member as object" format in the MatrixRTC object. - let livekitParticipantId = - rtcMember.sender + ":" + rtcMember.deviceId; - + const { member, id: livekitParticipantId } = + getRoomMemberFromRtcMember(rtcMember, room); const matrixIdentifier = `${rtcMember.sender}:${rtcMember.deviceId}`; let participant: | LocalParticipant | RemoteParticipant | undefined = undefined; - if ( - rtcMember.sender === room.client.getUserId()! && - rtcMember.deviceId === room.client.getDeviceId() - ) { - livekitParticipantId = "local"; + if (livekitParticipantId === "local") { participant = localParticipant; } else { participant = remoteParticipants.find( @@ -511,7 +546,6 @@ export class CallViewModel extends ViewModel { ); } - const member = findMatrixRoomMember(room, livekitParticipantId); if (!member) { logger.error( "Could not find member for media id: ", @@ -544,6 +578,9 @@ export class CallViewModel extends ViewModel { participant, this.encryptionSystem, this.livekitRoom, + this.memberDisplaynames$.pipe( + map((m) => m.get(matrixIdentifier) ?? "[👻]"), + ), this.handsRaised$.pipe( map((v) => v[matrixIdentifier]?.time ?? null), ), @@ -564,6 +601,9 @@ export class CallViewModel extends ViewModel { participant, this.encryptionSystem, this.livekitRoom, + this.memberDisplaynames$.pipe( + map((m) => m.get(livekitParticipantId) ?? "[👻]"), + ), ), ]; } @@ -602,6 +642,9 @@ export class CallViewModel extends ViewModel { participant, this.encryptionSystem, this.livekitRoom, + this.memberDisplaynames$.pipe( + map((m) => m.get(participant.identity) ?? "[👻]"), + ), of(null), of(null), ), diff --git a/src/state/MediaViewModel.ts b/src/state/MediaViewModel.ts index 19a717868..5efc60f51 100644 --- a/src/state/MediaViewModel.ts +++ b/src/state/MediaViewModel.ts @@ -26,7 +26,7 @@ import { RoomEvent as LivekitRoomEvent, RemoteTrack, } from "livekit-client"; -import { type RoomMember, RoomMemberEvent } from "matrix-js-sdk/src/matrix"; +import { type RoomMember } from "matrix-js-sdk/src/matrix"; import { BehaviorSubject, type Observable, @@ -43,38 +43,15 @@ import { switchMap, throttleTime, } from "rxjs"; -import { useEffect } from "react"; import { ViewModel } from "./ViewModel"; -import { useReactiveState } from "../useReactiveState"; -import { alwaysShowSelf, showConnectionStats } from "../settings/settings"; +import { alwaysShowSelf } from "../settings/settings"; +import { showConnectionStats } from "../settings/settings"; import { accumulate } from "../utils/observable"; import { type EncryptionSystem } from "../e2ee/sharedKeyManagement"; import { E2eeType } from "../e2ee/e2eeType"; import { type ReactionOption } from "../reactions"; -// TODO: Move this naming logic into the view model -export function useDisplayName(vm: MediaViewModel): string { - const [displayName, setDisplayName] = useReactiveState( - () => vm.member?.rawDisplayName ?? "[👻]", - [vm.member], - ); - useEffect(() => { - if (vm.member) { - const updateName = (): void => { - setDisplayName(vm.member!.rawDisplayName); - }; - - vm.member!.on(RoomMemberEvent.Name, updateName); - return (): void => { - vm.member!.removeListener(RoomMemberEvent.Name, updateName); - }; - } - }, [vm.member, setDisplayName]); - - return displayName; -} - export function observeTrackReference$( participant$: Observable, source: Track.Source, @@ -280,6 +257,7 @@ abstract class BaseMediaViewModel extends ViewModel { audioSource: AudioSource, videoSource: VideoSource, livekitRoom: LivekitRoom, + public readonly displayname$: Observable, ) { super(); const audio$ = observeTrackReference$(participant$, audioSource).pipe( @@ -408,6 +386,7 @@ abstract class BaseUserMediaViewModel extends BaseMediaViewModel { participant$: Observable, encryptionSystem: EncryptionSystem, livekitRoom: LivekitRoom, + displayname$: Observable, public readonly handRaised$: Observable, public readonly reaction$: Observable, ) { @@ -419,6 +398,7 @@ abstract class BaseUserMediaViewModel extends BaseMediaViewModel { Track.Source.Microphone, Track.Source.Camera, livekitRoom, + displayname$, ); const media$ = participant$.pipe( @@ -450,6 +430,8 @@ abstract class BaseUserMediaViewModel extends BaseMediaViewModel { } /** + }, + }, * The local participant's user media. */ export class LocalUserMediaViewModel extends BaseUserMediaViewModel { @@ -483,6 +465,7 @@ export class LocalUserMediaViewModel extends BaseUserMediaViewModel { participant$: Observable, encryptionSystem: EncryptionSystem, livekitRoom: LivekitRoom, + displayname$: Observable, handRaised$: Observable, reaction$: Observable, ) { @@ -492,6 +475,7 @@ export class LocalUserMediaViewModel extends BaseUserMediaViewModel { participant$, encryptionSystem, livekitRoom, + displayname$, handRaised$, reaction$, ); @@ -574,6 +558,7 @@ export class RemoteUserMediaViewModel extends BaseUserMediaViewModel { participant$: Observable, encryptionSystem: EncryptionSystem, livekitRoom: LivekitRoom, + displayname$: Observable, handRaised$: Observable, reaction$: Observable, ) { @@ -583,6 +568,7 @@ export class RemoteUserMediaViewModel extends BaseUserMediaViewModel { participant$, encryptionSystem, livekitRoom, + displayname$, handRaised$, reaction$, ); @@ -637,6 +623,7 @@ export class ScreenShareViewModel extends BaseMediaViewModel { participant$: Observable, encryptionSystem: EncryptionSystem, livekitRoom: LivekitRoom, + displayname$: Observable, public readonly local: boolean, ) { super( @@ -647,6 +634,7 @@ export class ScreenShareViewModel extends BaseMediaViewModel { Track.Source.ScreenShareAudio, Track.Source.ScreenShare, livekitRoom, + displayname$, ); } } diff --git a/src/tile/GridTile.tsx b/src/tile/GridTile.tsx index 0d33ccd6c..16972481b 100644 --- a/src/tile/GridTile.tsx +++ b/src/tile/GridTile.tsx @@ -39,7 +39,6 @@ import { useObservableEagerState, useObservableState } from "observable-hooks"; import styles from "./GridTile.module.css"; import { type UserMediaViewModel, - useDisplayName, LocalUserMediaViewModel, type RemoteUserMediaViewModel, } from "../state/MediaViewModel"; @@ -323,7 +322,7 @@ export const GridTile = forwardRef( const ourRef = useRef(null); const ref = useMergedRefs(ourRef, theirRef); const media = useObservableEagerState(vm.media$); - const displayName = useDisplayName(media); + const displayName = useObservableEagerState(media.displayname$); if (media instanceof LocalUserMediaViewModel) { return ( diff --git a/src/tile/SpotlightTile.tsx b/src/tile/SpotlightTile.tsx index c72bad81d..70fc937ef 100644 --- a/src/tile/SpotlightTile.tsx +++ b/src/tile/SpotlightTile.tsx @@ -36,7 +36,6 @@ import { type MediaViewModel, ScreenShareViewModel, type UserMediaViewModel, - useDisplayName, } from "../state/MediaViewModel"; import { useInitial } from "../useInitial"; import { useMergedRefs } from "../useMergedRefs"; @@ -132,7 +131,7 @@ const SpotlightItem = forwardRef( ) => { const ourRef = useRef(null); const ref = useMergedRefs(ourRef, theirRef); - const displayName = useDisplayName(vm); + const displayName = useObservableEagerState(vm.displayname$); const video = useObservableEagerState(vm.video$); const unencryptedWarning = useObservableEagerState(vm.unencryptedWarning$); const encryptionStatus = useObservableEagerState(vm.encryptionStatus$); diff --git a/src/utils/displayname.test.ts b/src/utils/displayname.test.ts new file mode 100644 index 000000000..fce473fa7 --- /dev/null +++ b/src/utils/displayname.test.ts @@ -0,0 +1,118 @@ +/* +Copyright 2025 New Vector Ltd. + +SPDX-License-Identifier: AGPL-3.0-only +Please see LICENSE in the repository root for full details. +*/ + +import { describe, expect, test } from "vitest"; + +import { calculateDisplayName, shouldDisambiguate } from "./displayname"; +import { + alice, + aliceDoppelganger, + aliceDoppelgangerRtcMember, + aliceRtcMember, + bob, + bobRtcMember, + bobZeroWidthSpace, + bobZeroWidthSpaceRtcMember, + daveRTL, +} from "./test-fixtures"; +import { mockMatrixRoom } from "./test"; + +describe("shouldDisambiguate", () => { + test("should not disambiguate a solo member", () => { + const room = mockMatrixRoom({}); + expect(shouldDisambiguate(alice, [], room)).toEqual(false); + }); + test("should not disambiguate a member with an empty displayname", () => { + const room = mockMatrixRoom({ + getMember: (u) => + [alice, aliceDoppelganger].find((m) => m.userId === u) ?? null, + }); + expect( + shouldDisambiguate( + { rawDisplayName: "", userId: alice.userId }, + [aliceRtcMember, aliceDoppelgangerRtcMember], + room, + ), + ).toEqual(false); + }); + test("should disambiguate a member with RTL characters", () => { + const room = mockMatrixRoom({}); + expect(shouldDisambiguate(daveRTL, [], room)).toEqual(true); + }); + test("should disambiguate a member with a matching displayname", () => { + const room = mockMatrixRoom({ + getMember: (u) => + [alice, aliceDoppelganger].find((m) => m.userId === u) ?? null, + }); + expect( + shouldDisambiguate( + alice, + [aliceRtcMember, aliceDoppelgangerRtcMember], + room, + ), + ).toEqual(true); + expect( + shouldDisambiguate( + aliceDoppelganger, + [aliceRtcMember, aliceDoppelgangerRtcMember], + room, + ), + ).toEqual(true); + }); + test("should disambiguate a member with a matching displayname with hidden spaces", () => { + const room = mockMatrixRoom({ + getMember: (u) => + [bob, bobZeroWidthSpace].find((m) => m.userId === u) ?? null, + }); + expect( + shouldDisambiguate(bob, [bobRtcMember, bobZeroWidthSpaceRtcMember], room), + ).toEqual(true); + expect( + shouldDisambiguate( + bobZeroWidthSpace, + [bobRtcMember, bobZeroWidthSpaceRtcMember], + room, + ), + ).toEqual(true); + }); + test.for(["Alice @foo:bar", "@foo:b", "A@foo:lice", "A @f oo: ba r"])( + "should disambiguate a member with a displayname containing a mxid-like string '%s'", + (rawDisplayName) => { + const room = mockMatrixRoom({ + getMember: (u) => + [alice, aliceDoppelganger].find((m) => m.userId === u) ?? null, + }); + expect( + shouldDisambiguate({ rawDisplayName, userId: alice.userId }, [], room), + ).toEqual(true); + }, + ); +}); + +describe("calculateDisplayName", () => { + test.for<[{ rawDisplayName?: string; userId: string }, boolean, string]>([ + [alice, false, alice.rawDisplayName], + [alice, true, `${alice.rawDisplayName} (${alice.userId})`], + [alice, false, alice.rawDisplayName], + [{ rawDisplayName: "", userId: alice.userId }, false, alice.userId], + [ + { rawDisplayName: alice.userId, userId: alice.userId }, + false, + alice.userId, + ], + [bobZeroWidthSpace, false, "Bob"], + [ + { rawDisplayName: "\u200b\u200b\u200b", userId: alice.userId }, + false, + alice.userId, + ], + [daveRTL, false, "evaD"], + [daveRTL, true, `evaD (${daveRTL.userId})`], + ])("correctly calculates displayname", ([member, disambiguate, result]) => + expect(calculateDisplayName(member, disambiguate)).toEqual(result), + ); +}); diff --git a/src/utils/displayname.ts b/src/utils/displayname.ts new file mode 100644 index 000000000..63b54ebc6 --- /dev/null +++ b/src/utils/displayname.ts @@ -0,0 +1,81 @@ +/* +Copyright 2025 New Vector Ltd. + +SPDX-License-Identifier: AGPL-3.0-only +Please see LICENSE in the repository root for full details. +*/ + +import { + removeDirectionOverrideChars, + removeHiddenChars, +} from "matrix-js-sdk/src/utils"; + +import type { Room } from "matrix-js-sdk/src/matrix"; +import type { CallMembership } from "matrix-js-sdk/src/matrixrtc"; + +// Borrowed from https://github.com/matrix-org/matrix-js-sdk/blob/f10deb5ef2e8f061ff005af0476034382ea128ca/src/models/room-member.ts#L409 +export function shouldDisambiguate( + member: { rawDisplayName?: string; userId: string }, + memberships: CallMembership[], + room: Room, +): boolean { + const { rawDisplayName: displayName, userId } = member; + if (!displayName || displayName === userId) return false; + + // First check if the displayname is something we consider truthy + // after stripping it of zero width characters and padding spaces + const strippedDisplayName = removeHiddenChars(displayName); + if (!strippedDisplayName) return false; + + // Next check if the name contains something that look like a mxid + // If it does, it may be someone trying to impersonate someone else + // Show full mxid in this case + if (/@.+:.+/.test(displayName)) return true; + + // Also show mxid if the display name contains any LTR/RTL characters as these + // make it very difficult for us to find similar *looking* display names + // E.g "Mark" could be cloned by writing "kraM" but in RTL. + if (/[\u200E\u200F\u202A-\u202F]/.test(displayName)) return true; + + // Also show mxid if there are other people with the same or similar + // displayname, after hidden character removal. + return ( + memberships + .map((m) => m.sender && room.getMember(m.sender)) + // NOTE: We *should* have a room member for everyone. + .filter((m) => !!m) + .filter((m) => m.userId !== userId) + .some((m) => calculateDisplayName(m, false) === strippedDisplayName) + ); +} + +export function calculateDisplayName( + member: { rawDisplayName?: string; userId: string }, + disambiguate: boolean, +): string { + const { rawDisplayName: displayName, userId } = member; + if (!displayName || displayName === userId) return userId; + + const resultDisplayname = removeDirectionOverrideChars( + removeHiddenChars(displayName), + ); + + if (disambiguate) return resultDisplayname + " (" + userId + ")"; + + // First check if the displayname is something we consider truthy + // after stripping it of zero width characters and padding spaces + if (!removeHiddenChars(displayName)) return userId; + + // We always strip the direction override characters (LRO and RLO). + // These override the text direction for all subsequent characters + // in the paragraph so if display names contained these, they'd + // need to be wrapped in something to prevent this from leaking out + // (which we can do in HTML but not text) or we'd need to add + // control characters to the string to reset any overrides (eg. + // adding PDF characters at the end). As far as we can see, + // there should be no reason these would be necessary - rtl display + // names should flip into the correct direction automatically based on + // the characters, and you can still embed rtl in ltr or vice versa + // with the embed chars or marker chars. + return resultDisplayname; +} diff --git a/src/utils/test-fixtures.ts b/src/utils/test-fixtures.ts index a105b5f74..1172d111d 100644 --- a/src/utils/test-fixtures.ts +++ b/src/utils/test-fixtures.ts @@ -12,13 +12,50 @@ import { mockLocalParticipant, } from "./test"; +export const localRtcMember = mockRtcMembership("@carol:example.org", "CCCC"); +export const local = mockMatrixRoomMember(localRtcMember); +export const localParticipant = mockLocalParticipant({ identity: "" }); +export const localId = `${local.userId}:${localRtcMember.deviceId}`; + export const aliceRtcMember = mockRtcMembership("@alice:example.org", "AAAA"); -export const alice = mockMatrixRoomMember(aliceRtcMember); +export const alice = mockMatrixRoomMember(aliceRtcMember, { + rawDisplayName: "Alice", +}); export const aliceId = `${alice.userId}:${aliceRtcMember.deviceId}`; export const aliceParticipant = mockRemoteParticipant({ identity: aliceId }); -export const localRtcMember = mockRtcMembership("@carol:example.org", "CCCC"); -export const local = mockMatrixRoomMember(localRtcMember); -export const localParticipant = mockLocalParticipant({ identity: "" }); +export const aliceDoppelgangerRtcMember = mockRtcMembership( + "@alice2:example.org", + "AAAA", +); +export const aliceDoppelganger = mockMatrixRoomMember( + aliceDoppelgangerRtcMember, + { + rawDisplayName: "Alice", + }, +); +export const aliceDoppelgangerId = `${aliceDoppelganger.userId}:${aliceDoppelgangerRtcMember.deviceId}`; export const bobRtcMember = mockRtcMembership("@bob:example.org", "BBBB"); +export const bob = mockMatrixRoomMember(bobRtcMember, { + rawDisplayName: "Bob", +}); +export const bobId = `${bob.userId}:${bobRtcMember.deviceId}`; + +export const bobZeroWidthSpaceRtcMember = mockRtcMembership( + "@bob2:example.org", + "BBBB", +); +export const bobZeroWidthSpace = mockMatrixRoomMember( + bobZeroWidthSpaceRtcMember, + { + rawDisplayName: "Bo\u200bb", + }, +); +export const bobZeroWidthSpaceId = `${bobZeroWidthSpace.userId}:${bobZeroWidthSpaceRtcMember.deviceId}`; + +export const daveRTLRtcMember = mockRtcMembership("@dave2:example.org", "DDDD"); +export const daveRTL = mockMatrixRoomMember(daveRTLRtcMember, { + rawDisplayName: "\u200fevaD", +}); +export const daveRTLId = `${daveRTL.userId}:${daveRTLRtcMember.deviceId}`; diff --git a/src/utils/test.ts b/src/utils/test.ts index 41e85ba33..52f34a2ec 100644 --- a/src/utils/test.ts +++ b/src/utils/test.ts @@ -205,6 +205,7 @@ export async function withLocalMedia( kind: E2eeType.PER_PARTICIPANT, }, mockLivekitRoom({ localParticipant }), + of(roomMember.rawDisplayName ?? "nodisplayname"), of(null), of(null), ); @@ -243,6 +244,7 @@ export async function withRemoteMedia( kind: E2eeType.PER_PARTICIPANT, }, mockLivekitRoom({}, { remoteParticipants$: of([remoteParticipant]) }), + of(roomMember.rawDisplayName ?? "nodisplayname"), of(null), of(null), );