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
Changes from 4 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
24 changes: 7 additions & 17 deletions src/models/room.ts
Original file line number Diff line number Diff line change
Expand Up @@ -932,25 +932,15 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {

public getAvatarFallbackMember(): RoomMember | undefined {
const functionalMembers = this.getFunctionalMembers();
// Optimizing for performance, we iterate over all members outermost, as is it the longest of the 3 lists we are comparing.
const nonFunctionalMemberCount = this.getMembers()!.reduce((count, m) => {
if (
m.membership &&
// We care most about big rooms where also possibly lots of users may have left,
// so we first do the O(2) membership check, as it is definitely cheap and might remove candidates before the next step.
["join", "invite"].includes(m.membership) &&
// Then we do the O(functionalMembers.length) functionalMember check, which is probably usually still cheap.
!functionalMembers.includes(m.userId)
) {
return count + 1;
}
return count;
}, 0);

// 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
if (nonFunctionalMemberCount > 2) {
return;
}
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));
Expand Down
Loading