Skip to content

Commit 8e0ef5f

Browse files
HarHarLinksrichvdh
HarHarLinks
and
richvdh
authored
fix automatic DM avatar with functional members (#4017)
* fix automatic DM avatar with functional members * update comments * lint * add tests for functional members * keep functional members out of the public API - remove public API for functional members, reverting most of 0ce2d82, f9b41f6, e65fb24 - remove tests for functional members public API c114bf5 - add shared functional members getter for both room name and avatar fallback generation * filter functional members from more candidates - remove from hero(es) - remove from previous members * add tests for fallback avatars with functional members * Add docstring for getFunctionalMembers Co-authored-by: Richard van der Hoff <[email protected]> * inline getInvitedAndJoinedFunctionalMemberCount * update comments for getAvatarFallbackMember * use correct list of heroes in getAvatarFallbackMember * remove redundant type annotation * optimize performance of invitedAndJoinedFunctionalMemberCount * calculate nonFunctionalMemberCount in one step instead of iterating redundantly * clean up functional member tests with review feedback * lint * Update src/models/room.ts Co-authored-by: Richard van der Hoff <[email protected]> * apply feedback about comments * non-functional per review, lint --------- Co-authored-by: Richard van der Hoff <[email protected]>
1 parent 78d0594 commit 8e0ef5f

File tree

2 files changed

+124
-24
lines changed

2 files changed

+124
-24
lines changed

‎spec/unit/room.spec.ts‎

Lines changed: 73 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2204,7 +2204,7 @@ describe("Room", function () {
22042204
});
22052205

22062206
describe("getAvatarFallbackMember", () => {
2207-
it("should should return undefined if the room isn't a 1:1", () => {
2207+
it("should return undefined if the room isn't a 1:1", () => {
22082208
const room = new Room(roomId, null!, userA);
22092209
room.currentState.setJoinedMemberCount(2);
22102210
room.currentState.setInvitedMemberCount(1);
@@ -2231,6 +2231,78 @@ describe("Room", function () {
22312231
});
22322232
expect(room.getAvatarFallbackMember()?.userId).toBe(userD);
22332233
});
2234+
2235+
it("should return undefined if the room is a 1:1 plus functional member", async function () {
2236+
const room = new Room(roomId, null!, userA);
2237+
await room.currentState.setStateEvents([
2238+
utils.mkMembership({
2239+
user: userA,
2240+
mship: "join",
2241+
room: roomId,
2242+
event: true,
2243+
name: "User A",
2244+
}),
2245+
utils.mkMembership({
2246+
user: userB,
2247+
mship: "join",
2248+
room: roomId,
2249+
event: true,
2250+
name: "User B",
2251+
}),
2252+
utils.mkEvent({
2253+
type: UNSTABLE_ELEMENT_FUNCTIONAL_USERS.name,
2254+
skey: "",
2255+
room: roomId,
2256+
event: true,
2257+
content: {
2258+
service_members: [userB],
2259+
},
2260+
}),
2261+
]);
2262+
expect(room.getAvatarFallbackMember()).toBeUndefined();
2263+
});
2264+
2265+
it("should pick nonfunctional member from summary heroes if room is a 1:1 plus functional member", async function () {
2266+
const room = new Room(roomId, null!, userA);
2267+
await room.currentState.setStateEvents([
2268+
utils.mkMembership({
2269+
user: userA,
2270+
mship: "join",
2271+
room: roomId,
2272+
event: true,
2273+
name: "User A",
2274+
}),
2275+
utils.mkMembership({
2276+
user: userB,
2277+
mship: "join",
2278+
room: roomId,
2279+
event: true,
2280+
name: "User B",
2281+
}),
2282+
utils.mkMembership({
2283+
user: userD,
2284+
mship: "join",
2285+
room: roomId,
2286+
event: true,
2287+
name: "User D",
2288+
}),
2289+
utils.mkEvent({
2290+
type: UNSTABLE_ELEMENT_FUNCTIONAL_USERS.name,
2291+
skey: "",
2292+
room: roomId,
2293+
event: true,
2294+
content: {
2295+
service_members: [userB],
2296+
},
2297+
}),
2298+
]);
2299+
room.setSummary({
2300+
"m.heroes": [userA, userD, userB],
2301+
"m.joined_member_count": 2,
2302+
"m.invited_member_count": 1,
2303+
});
2304+
expect(room.getAvatarFallbackMember()?.userId).toBe(userD);
2305+
});
22342306
});
22352307

22362308
describe("maySendMessage", function () {

‎src/models/room.ts‎

Lines changed: 51 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -914,37 +914,69 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
914914
return this.myUserId;
915915
}
916916

917-
public getAvatarFallbackMember(): RoomMember | undefined {
918-
const memberCount = this.getInvitedAndJoinedMemberCount();
919-
if (memberCount > 2) {
920-
return;
917+
/**
918+
* Gets the "functional members" in this room.
919+
*
920+
* Returns the list of userIDs from the `io.element.functional_members` event. Does not consider the
921+
* current membership states of those users.
922+
*
923+
* @see https://github.com/element-hq/element-meta/blob/develop/spec/functional_members.md.
924+
*/
925+
private getFunctionalMembers(): string[] {
926+
const mFunctionalMembers = this.currentState.getStateEvents(UNSTABLE_ELEMENT_FUNCTIONAL_USERS.name, "");
927+
if (Array.isArray(mFunctionalMembers?.getContent().service_members)) {
928+
return mFunctionalMembers!.getContent().service_members;
921929
}
922-
const hasHeroes = Array.isArray(this.summaryHeroes) && this.summaryHeroes.length;
930+
return [];
931+
}
932+
933+
public getAvatarFallbackMember(): RoomMember | undefined {
934+
const functionalMembers = this.getFunctionalMembers();
935+
936+
// Only generate a fallback avatar if the conversation is with a single specific other user (a "DM").
937+
let nonFunctionalMemberCount = 0;
938+
this.getMembers()!.forEach((m) => {
939+
if (m.membership !== "join" && m.membership !== "invite") return;
940+
if (functionalMembers.includes(m.userId)) return;
941+
nonFunctionalMemberCount++;
942+
});
943+
if (nonFunctionalMemberCount > 2) return;
944+
945+
// Prefer the list of heroes, if present. It should only include the single other user in the DM.
946+
const nonFunctionalHeroes = this.summaryHeroes?.filter((h) => !functionalMembers.includes(h));
947+
const hasHeroes = Array.isArray(nonFunctionalHeroes) && nonFunctionalHeroes.length;
923948
if (hasHeroes) {
924-
const availableMember = this.summaryHeroes!.map((userId) => {
925-
return this.getMember(userId);
926-
}).find((member) => !!member);
949+
const availableMember = nonFunctionalHeroes
950+
.map((userId) => {
951+
return this.getMember(userId);
952+
})
953+
.find((member) => !!member);
927954
if (availableMember) {
928955
return availableMember;
929956
}
930957
}
931-
const members = this.currentState.getMembers();
932-
// could be different than memberCount
933-
// as this includes left members
934-
if (members.length <= 2) {
935-
const availableMember = members.find((m) => {
958+
959+
// Consider *all*, including previous, members, to generate the avatar for DMs where the other user left.
960+
// Needed to generate a matching avatar for rooms named "Empty Room (was Alice)".
961+
const members = this.getMembers();
962+
const nonFunctionalMembers = members?.filter((m) => !functionalMembers.includes(m.userId));
963+
if (nonFunctionalMembers.length <= 2) {
964+
const availableMember = nonFunctionalMembers.find((m) => {
936965
return m.userId !== this.myUserId;
937966
});
938967
if (availableMember) {
939968
return availableMember;
940969
}
941970
}
942-
// if all else fails, try falling back to a user,
943-
// and create a one-off member for it
971+
972+
// If all else failed, but the homeserver gave us heroes that previously could not be found in the room members,
973+
// trust and try falling back to a hero, creating a one-off member for it
944974
if (hasHeroes) {
945-
const availableUser = this.summaryHeroes!.map((userId) => {
946-
return this.client.getUser(userId);
947-
}).find((user) => !!user);
975+
const availableUser = nonFunctionalHeroes
976+
.map((userId) => {
977+
return this.client.getUser(userId);
978+
})
979+
.find((user) => !!user);
948980
if (availableUser) {
949981
const member = new RoomMember(this.roomId, availableUser.userId);
950982
member.user = availableUser;
@@ -3351,11 +3383,7 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
33513383
let inviteJoinCount = joinedMemberCount + invitedMemberCount - 1;
33523384

33533385
// get service members (e.g. helper bots) for exclusion
3354-
let excludedUserIds: string[] = [];
3355-
const mFunctionalMembers = this.currentState.getStateEvents(UNSTABLE_ELEMENT_FUNCTIONAL_USERS.name, "");
3356-
if (Array.isArray(mFunctionalMembers?.getContent().service_members)) {
3357-
excludedUserIds = mFunctionalMembers!.getContent().service_members;
3358-
}
3386+
const excludedUserIds = this.getFunctionalMembers();
33593387

33603388
// get members that are NOT ourselves and are actually in the room.
33613389
let otherNames: string[] = [];

0 commit comments

Comments
 (0)