Skip to content

Commit

Permalink
fix automatic DM avatar with functional members (matrix-org#4017)
Browse files Browse the repository at this point in the history 
* 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]>
  • Loading branch information
@HarHarLinks @richvdh
HarHarLinks and richvdh authored Mar 13, 2024
1 parent 78d0594 commit 8e0ef5f
Show file tree
Hide file tree
Showing 2 changed files with 124 additions and 24 deletions.
74 changes: 73 additions & 1 deletion spec/unit/room.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2204,7 +2204,7 @@ describe("Room", function () {
});

describe("getAvatarFallbackMember", () => {
it("should should return undefined if the room isn't a 1:1", () => {
it("should return undefined if the room isn't a 1:1", () => {
const room = new Room(roomId, null!, userA);
room.currentState.setJoinedMemberCount(2);
room.currentState.setInvitedMemberCount(1);
Expand All @@ -2231,6 +2231,78 @@ describe("Room", function () {
});
expect(room.getAvatarFallbackMember()?.userId).toBe(userD);
});

it("should return undefined if the room is a 1:1 plus functional member", async function () {
const room = new Room(roomId, null!, userA);
await room.currentState.setStateEvents([
utils.mkMembership({
user: userA,
mship: "join",
room: roomId,
event: true,
name: "User A",
}),
utils.mkMembership({
user: userB,
mship: "join",
room: roomId,
event: true,
name: "User B",
}),
utils.mkEvent({
type: UNSTABLE_ELEMENT_FUNCTIONAL_USERS.name,
skey: "",
room: roomId,
event: true,
content: {
service_members: [userB],
},
}),
]);
expect(room.getAvatarFallbackMember()).toBeUndefined();
});

it("should pick nonfunctional member from summary heroes if room is a 1:1 plus functional member", async function () {
const room = new Room(roomId, null!, userA);
await room.currentState.setStateEvents([
utils.mkMembership({
user: userA,
mship: "join",
room: roomId,
event: true,
name: "User A",
}),
utils.mkMembership({
user: userB,
mship: "join",
room: roomId,
event: true,
name: "User B",
}),
utils.mkMembership({
user: userD,
mship: "join",
room: roomId,
event: true,
name: "User D",
}),
utils.mkEvent({
type: UNSTABLE_ELEMENT_FUNCTIONAL_USERS.name,
skey: "",
room: roomId,
event: true,
content: {
service_members: [userB],
},
}),
]);
room.setSummary({
"m.heroes": [userA, userD, userB],
"m.joined_member_count": 2,
"m.invited_member_count": 1,
});
expect(room.getAvatarFallbackMember()?.userId).toBe(userD);
});
});

describe("maySendMessage", function () {
Expand Down
74 changes: 51 additions & 23 deletions src/models/room.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -914,37 +914,69 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
return this.myUserId;
}

public getAvatarFallbackMember(): RoomMember | undefined {
const memberCount = this.getInvitedAndJoinedMemberCount();
if (memberCount > 2) {
return;
/**
* Gets the "functional members" in this room.
*
* Returns the list of userIDs from the `io.element.functional_members` event. Does not consider the
* current membership states of those users.
*
* @see https://github.com/element-hq/element-meta/blob/develop/spec/functional_members.md.
*/
private getFunctionalMembers(): string[] {
const mFunctionalMembers = this.currentState.getStateEvents(UNSTABLE_ELEMENT_FUNCTIONAL_USERS.name, "");
if (Array.isArray(mFunctionalMembers?.getContent().service_members)) {
return mFunctionalMembers!.getContent().service_members;
}
const hasHeroes = Array.isArray(this.summaryHeroes) && this.summaryHeroes.length;
return [];
}

public getAvatarFallbackMember(): RoomMember | undefined {
const functionalMembers = this.getFunctionalMembers();

// Only generate a fallback avatar if the conversation is with a single specific other user (a "DM").
let nonFunctionalMemberCount = 0;
this.getMembers()!.forEach((m) => {
if (m.membership !== "join" && m.membership !== "invite") return;
if (functionalMembers.includes(m.userId)) return;
nonFunctionalMemberCount++;
});
if (nonFunctionalMemberCount > 2) return;

// Prefer the list of heroes, if present. It should only include the single other user in the DM.
const nonFunctionalHeroes = this.summaryHeroes?.filter((h) => !functionalMembers.includes(h));
const hasHeroes = Array.isArray(nonFunctionalHeroes) && nonFunctionalHeroes.length;
if (hasHeroes) {
const availableMember = this.summaryHeroes!.map((userId) => {
return this.getMember(userId);
}).find((member) => !!member);
const availableMember = nonFunctionalHeroes
.map((userId) => {
return this.getMember(userId);
})
.find((member) => !!member);
if (availableMember) {
return availableMember;
}
}
const members = this.currentState.getMembers();
// could be different than memberCount
// as this includes left members
if (members.length <= 2) {
const availableMember = members.find((m) => {

// Consider *all*, including previous, members, to generate the avatar for DMs where the other user left.
// Needed to generate a matching avatar for rooms named "Empty Room (was Alice)".
const members = this.getMembers();
const nonFunctionalMembers = members?.filter((m) => !functionalMembers.includes(m.userId));
if (nonFunctionalMembers.length <= 2) {
const availableMember = nonFunctionalMembers.find((m) => {
return m.userId !== this.myUserId;
});
if (availableMember) {
return availableMember;
}
}
// if all else fails, try falling back to a user,
// and create a one-off member for it

// If all else failed, but the homeserver gave us heroes that previously could not be found in the room members,
// trust and try falling back to a hero, creating a one-off member for it
if (hasHeroes) {
const availableUser = this.summaryHeroes!.map((userId) => {
return this.client.getUser(userId);
}).find((user) => !!user);
const availableUser = nonFunctionalHeroes
.map((userId) => {
return this.client.getUser(userId);
})
.find((user) => !!user);
if (availableUser) {
const member = new RoomMember(this.roomId, availableUser.userId);
member.user = availableUser;
Expand Down Expand Up @@ -3351,11 +3383,7 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
let inviteJoinCount = joinedMemberCount + invitedMemberCount - 1;

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

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

0 comments on commit 8e0ef5f

Please sign in to comment.