From 92ddc4c797ca7f292270d21b0414d6cf5198f19e Mon Sep 17 00:00:00 2001 From: Robin Date: Sun, 9 Nov 2025 01:16:39 -0500 Subject: [PATCH] Fix avatar reactivity, simplify display names tracking --- .../remoteMembers/MatrixLivekitMembers.ts | 68 ++++++++++--------- .../remoteMembers/displayname.test.ts | 59 ++++++++-------- .../remoteMembers/displayname.ts | 20 ++---- 3 files changed, 72 insertions(+), 75 deletions(-) diff --git a/src/state/CallViewModel/remoteMembers/MatrixLivekitMembers.ts b/src/state/CallViewModel/remoteMembers/MatrixLivekitMembers.ts index 4aaaadd4..764862f2 100644 --- a/src/state/CallViewModel/remoteMembers/MatrixLivekitMembers.ts +++ b/src/state/CallViewModel/remoteMembers/MatrixLivekitMembers.ts @@ -13,15 +13,15 @@ import { type LivekitTransport, type CallMembership, } from "matrix-js-sdk/lib/matrixrtc"; -import { combineLatest, filter, map } from "rxjs"; +import { combineLatest, filter, fromEvent, map, startWith } from "rxjs"; // eslint-disable-next-line rxjs/no-internal import { type NodeStyleEventEmitter } from "rxjs/internal/observable/fromEvent"; -import { type Room as MatrixRoom } from "matrix-js-sdk"; +import { RoomStateEvent, type Room as MatrixRoom } from "matrix-js-sdk"; import { logger } from "matrix-js-sdk/lib/logger"; import { type Behavior } from "../../Behavior"; import { type IConnectionManager } from "./ConnectionManager"; -import { Epoch, mapEpoch, type ObservableScope } from "../../ObservableScope"; +import { Epoch, type ObservableScope } from "../../ObservableScope"; import { memberDisplaynames$ } from "./displayname"; import { type Connection } from "./Connection"; import { generateItemsWithEpoch } from "../../../utils/observable"; @@ -82,14 +82,17 @@ export function createMatrixLivekitMembers$({ const displaynameMap$ = memberDisplaynames$( scope, matrixRoom, - membershipsWithTransport$.pipe(mapEpoch((v) => v.map((v) => v.membership))), + scope.behavior( + membershipsWithTransport$.pipe( + map((ms) => ms.value.map((m) => m.membership)), + ), + ), ); return scope.behavior( combineLatest([ membershipsWithTransport$, connectionManager.connectionManagerData$, - displaynameMap$, ]).pipe( filter((values) => values.every((value) => value.epoch === values[0].epoch), @@ -98,15 +101,11 @@ export function createMatrixLivekitMembers$({ ([ { value: membershipsWithTransports, epoch }, { value: managerData }, - { value: displaynames }, ]) => - new Epoch( - [membershipsWithTransports, managerData, displaynames] as const, - epoch, - ), + new Epoch([membershipsWithTransports, managerData] as const, epoch), ), generateItemsWithEpoch( - function* ([membershipsWithTransports, managerData, displaynames]) { + function* ([membershipsWithTransports, managerData]) { for (const { membership, transport } of membershipsWithTransports) { // TODO! cannot use membership.membershipID yet, Currently its hardcoded by the jwt service to const participantId = /*membership.membershipID*/ `${membership.userId}:${membership.deviceId}`; @@ -116,35 +115,42 @@ export function createMatrixLivekitMembers$({ : []; const participant = participants.find((p) => p.identity == participantId) ?? null; - // This makes sense to add to the js-sdk callMembership (we only need the avatar so probably the call memberhsip just should aquire the avatar) - const member = matrixRoom.getMember(membership.userId); const connection = transport ? managerData.getConnectionForTransport(transport) : undefined; - let displayName = displaynames.get(membership.userId); - if (displayName === undefined) { - logger.warn(`No display name for user ${membership.userId}`); - displayName = ""; - } - yield { keys: [participantId, membership.userId], - data: { - membership, - participant, - connection, - displayName, - mxcAvatarUrl: member?.getMxcAvatarUrl(), - }, + data: { membership, participant, connection }, }; } }, - (scope, data$, participantId, userId) => ({ - participantId, - userId, - ...scope.splitBehavior(data$), - }), + (scope, data$, participantId, userId) => { + const member = matrixRoom.getMember(userId); + return { + participantId, + userId, + ...scope.splitBehavior(data$), + displayName$: scope.behavior( + displaynameMap$.pipe( + map((displayNames) => { + const name = displayNames.get(userId); + if (name === undefined) { + logger.warn(`No display name for user ${userId}`); + return ""; + } + return name; + }), + ), + ), + mxcAvatarUrl$: scope.behavior( + fromEvent(matrixRoom, RoomStateEvent.Members).pipe( + startWith(undefined), + map(() => member?.getMxcAvatarUrl()), + ), + ), + }; + }, ), ), ); diff --git a/src/state/CallViewModel/remoteMembers/displayname.test.ts b/src/state/CallViewModel/remoteMembers/displayname.test.ts index efbe1235..60a29a18 100644 --- a/src/state/CallViewModel/remoteMembers/displayname.test.ts +++ b/src/state/CallViewModel/remoteMembers/displayname.test.ts @@ -13,9 +13,8 @@ import { RoomStateEvent, } from "matrix-js-sdk"; import EventEmitter from "events"; -import { map } from "rxjs"; -import { ObservableScope, trackEpoch } from "../../ObservableScope.ts"; +import { ObservableScope } from "../../ObservableScope.ts"; import type { Room as MatrixRoom } from "matrix-js-sdk/lib/models/room"; import { mockCallMembership, withTestScheduler } from "../../../utils/test.ts"; import { memberDisplaynames$ } from "./displayname.ts"; @@ -86,16 +85,16 @@ afterEach(() => { // TODO this is a regression, now there the own user is not always in the map. Ask Timo if fine test.skip("should always have our own user", () => { - withTestScheduler(({ cold, schedule, expectObservable }) => { + withTestScheduler(({ behavior, expectObservable }) => { const dn$ = memberDisplaynames$( testScope, mockMatrixRoom, - cold("a", { + behavior("a", { a: [], - }).pipe(trackEpoch()), + }), ); - expectObservable(dn$.pipe(map((e) => e.value))).toBe("a", { + expectObservable(dn$).toBe("a", { a: new Map([ ["@local:example.com", "@local:example.com"], ]), @@ -116,19 +115,19 @@ function setUpBasicRoom(): void { test("should get displayName for users", () => { setUpBasicRoom(); - withTestScheduler(({ cold, schedule, expectObservable }) => { + withTestScheduler(({ behavior, expectObservable }) => { const dn$ = memberDisplaynames$( testScope, mockMatrixRoom, - cold("a", { + behavior("a", { a: [ mockCallMembership("@alice:example.com", "DEVICE1"), mockCallMembership("@bob:example.com", "DEVICE1"), ], - }).pipe(trackEpoch()), + }), ); - expectObservable(dn$.pipe(map((e) => e.value))).toBe("a", { + expectObservable(dn$).toBe("a", { a: new Map([ // ["@local:example.com", "it's a me"], ["@alice:example.com", "Alice"], @@ -139,18 +138,18 @@ test("should get displayName for users", () => { }); test("should use userId if no display name", () => { - withTestScheduler(({ cold, schedule, expectObservable }) => { + withTestScheduler(({ behavior, expectObservable }) => { setUpBasicRoom(); const dn$ = memberDisplaynames$( testScope, mockMatrixRoom, - cold("a", { + behavior("a", { a: [mockCallMembership("@no-name:foo.bar", "D000")], - }).pipe(trackEpoch()), + }), ); - expectObservable(dn$.pipe(map((e) => e.value))).toBe("a", { + expectObservable(dn$).toBe("a", { a: new Map([ // ["@local:example.com", "it's a me"], ["@no-name:foo.bar", "@no-name:foo.bar"], @@ -160,13 +159,13 @@ test("should use userId if no display name", () => { }); test("should disambiguate users with same display name", () => { - withTestScheduler(({ cold, schedule, expectObservable }) => { + withTestScheduler(({ behavior, expectObservable }) => { setUpBasicRoom(); const dn$ = memberDisplaynames$( testScope, mockMatrixRoom, - cold("a", { + behavior("a", { a: [ mockCallMembership("@bob:example.com", "DEVICE1"), mockCallMembership("@bob:example.com", "DEVICE2"), @@ -174,10 +173,10 @@ test("should disambiguate users with same display name", () => { mockCallMembership("@carl:example.com", "C000"), mockCallMembership("@evil:example.com", "E000"), ], - }).pipe(trackEpoch()), + }), ); - expectObservable(dn$.pipe(map((e) => e.value))).toBe("a", { + expectObservable(dn$).toBe("a", { a: new Map([ // ["@local:example.com", "it's a me"], ["@bob:example.com", "Bob (@bob:example.com)"], @@ -191,22 +190,22 @@ test("should disambiguate users with same display name", () => { }); test("should disambiguate when needed", () => { - withTestScheduler(({ cold, schedule, expectObservable }) => { + withTestScheduler(({ behavior, expectObservable }) => { setUpBasicRoom(); const dn$ = memberDisplaynames$( testScope, mockMatrixRoom, - cold("ab", { + behavior("ab", { a: [mockCallMembership("@bob:example.com", "DEVICE1")], b: [ mockCallMembership("@bob:example.com", "DEVICE1"), mockCallMembership("@bob:foo.bar", "BOB000"), ], - }).pipe(trackEpoch()), + }), ); - expectObservable(dn$.pipe(map((e) => e.value))).toBe("ab", { + expectObservable(dn$).toBe("ab", { a: new Map([ // ["@local:example.com", "it's a me"], ["@bob:example.com", "Bob"], @@ -221,22 +220,22 @@ test("should disambiguate when needed", () => { }); test.skip("should keep disambiguated name when other leave", () => { - withTestScheduler(({ cold, schedule, expectObservable }) => { + withTestScheduler(({ behavior, expectObservable }) => { setUpBasicRoom(); const dn$ = memberDisplaynames$( testScope, mockMatrixRoom, - cold("ab", { + behavior("ab", { a: [ mockCallMembership("@bob:example.com", "DEVICE1"), mockCallMembership("@bob:foo.bar", "BOB000"), ], b: [mockCallMembership("@bob:example.com", "DEVICE1")], - }).pipe(trackEpoch()), + }), ); - expectObservable(dn$.pipe(map((e) => e.value))).toBe("ab", { + expectObservable(dn$).toBe("ab", { a: new Map([ // ["@local:example.com", "it's a me"], ["@bob:example.com", "Bob (@bob:example.com)"], @@ -251,18 +250,18 @@ test.skip("should keep disambiguated name when other leave", () => { }); test("should disambiguate on name change", () => { - withTestScheduler(({ cold, schedule, expectObservable }) => { + withTestScheduler(({ behavior, schedule, expectObservable }) => { setUpBasicRoom(); const dn$ = memberDisplaynames$( testScope, mockMatrixRoom, - cold("a", { + behavior("a", { a: [ mockCallMembership("@bob:example.com", "B000"), mockCallMembership("@carl:example.com", "C000"), ], - }).pipe(trackEpoch()), + }), ); schedule("-a", { @@ -271,7 +270,7 @@ test("should disambiguate on name change", () => { }, }); - expectObservable(dn$.pipe(map((e) => e.value))).toBe("ab", { + expectObservable(dn$).toBe("ab", { a: new Map([ // ["@local:example.com", "it's a me"], ["@bob:example.com", "Bob"], diff --git a/src/state/CallViewModel/remoteMembers/displayname.ts b/src/state/CallViewModel/remoteMembers/displayname.ts index f56bc253..901f3613 100644 --- a/src/state/CallViewModel/remoteMembers/displayname.ts +++ b/src/state/CallViewModel/remoteMembers/displayname.ts @@ -6,20 +6,14 @@ Please see LICENSE in the repository root for full details. */ import { type RoomMember, RoomStateEvent } from "matrix-js-sdk"; -import { - combineLatest, - fromEvent, - map, - type Observable, - startWith, -} from "rxjs"; +import { combineLatest, fromEvent, map, startWith } from "rxjs"; import { type CallMembership } from "matrix-js-sdk/lib/matrixrtc"; import { logger } from "matrix-js-sdk/lib/logger"; import { type Room as MatrixRoom } from "matrix-js-sdk/lib/matrix"; // eslint-disable-next-line rxjs/no-internal import { type NodeStyleEventEmitter } from "rxjs/internal/observable/fromEvent"; -import { Epoch, type ObservableScope } from "../../ObservableScope"; +import { type ObservableScope } from "../../ObservableScope"; import { calculateDisplayName, shouldDisambiguate, @@ -49,8 +43,8 @@ export const memberDisplaynames$ = ( scope: ObservableScope, matrixRoom: Pick & NodeStyleEventEmitter, // roomMember$: Behavior>; - memberships$: Observable>, -): Behavior>> => + memberships$: Behavior, +): Behavior> => scope.behavior( combineLatest([ // Handle call membership changes @@ -59,8 +53,7 @@ export const memberDisplaynames$ = ( fromEvent(matrixRoom, RoomStateEvent.Members).pipe(startWith(null)), // TODO: do we need: pauseWhen(this.pretendToBeDisconnected$), ]).pipe( - map(([epochMemberships, _displayNames]) => { - const { epoch, value: memberships } = epochMemberships; + map(([memberships, _displayNames]) => { const displaynameMap = new Map(); const room = matrixRoom; @@ -77,8 +70,7 @@ export const memberDisplaynames$ = ( calculateDisplayName(member, disambiguate), ); } - return new Epoch(displaynameMap, epoch); + return displaynameMap; }), ), - new Epoch(new Map()), );