Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix automatic DM avatar with functional members #4017

Merged
merged 22 commits into from
Mar 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
0ce2d82
fix automatic DM avatar with functional members
HarHarLinks Jan 19, 2024
f9b41f6
update comments
HarHarLinks Feb 6, 2024
e65fb24
lint
HarHarLinks Feb 6, 2024
c114bf5
add tests for functional members
HarHarLinks Feb 6, 2024
92aef67
Merge branch 'develop' into fix-functional-members-avatar
richvdh Feb 26, 2024
25fdc18
keep functional members out of the public API
HarHarLinks Feb 26, 2024
d6f83d3
filter functional members from more candidates
HarHarLinks Feb 26, 2024
657f1bc
add tests for fallback avatars with functional members
HarHarLinks Feb 26, 2024
9234e82
Merge branch 'develop' into fix-functional-members-avatar
HarHarLinks Feb 26, 2024
5794809
Add docstring for getFunctionalMembers
HarHarLinks Feb 28, 2024
0789a23
inline getInvitedAndJoinedFunctionalMemberCount
HarHarLinks Feb 28, 2024
b2b5b18
update comments for getAvatarFallbackMember
HarHarLinks Feb 28, 2024
fa4666c
use correct list of heroes in getAvatarFallbackMember
HarHarLinks Feb 28, 2024
404d671
remove redundant type annotation
HarHarLinks Feb 28, 2024
0f6ab78
optimize performance of invitedAndJoinedFunctionalMemberCount
HarHarLinks Feb 28, 2024
8db6b47
calculate nonFunctionalMemberCount in one step
HarHarLinks Feb 28, 2024
66157c4
clean up functional member tests with review feedback
HarHarLinks Feb 28, 2024
684dcac
lint
HarHarLinks Feb 28, 2024
0ea0d50
Update src/models/room.ts
HarHarLinks Mar 12, 2024
efca7ba
apply feedback about comments
HarHarLinks Mar 12, 2024
882f625
non-functional per review, lint
HarHarLinks Mar 12, 2024
5f8a985
Merge branch 'develop' into fix-functional-members-avatar
HarHarLinks Mar 12, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 73 additions & 1 deletion spec/unit/room.spec.ts
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
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").
HarHarLinks marked this conversation as resolved.
Show resolved Hide resolved
let nonFunctionalMemberCount = 0;
this.getMembers()!.forEach((m) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd still prefer for (const m of this.getMembers()!) { ... } (and continue instead of return), but this is fine.

Copy link
Contributor Author

@HarHarLinks HarHarLinks Mar 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean. I'm just not that firm in all the possible JS syntaxes. I feel like the major part of the legibility improvement was the restructured function body I changed on your suggestion. Your call.

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
Loading