fix automatic DM avatar with functional members #4017

HarHarLinks · 2024-01-19T00:22:16Z

needed by: matrix-org/matrix-react-sdk#12157

Checklist

Tests written for new code (and old code if feasible)
Linter and other CI checks pass
Sign-off given on the changes (see CONTRIBUTING.md)

Signed-off-by: Kim Brose [email protected]

richvdh

I'm generally uncomfortable with adding all these methods to the public API, for two reasons:

A lot of them are just one-line wrappers for other methods. It's a lot of clutter in the API, making it hard to find the important methods
"functional members" are an Element extension, and baking lots of support for them into the js-sdk seems like a bit of a conflation of concerns.

I would be inclined to limit the scope of this change to getAvatarFallbackMember (possibly with a helper method), without adding new public methods.

src/models/room.ts

- 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

- remove from hero(es) - remove from previous members

HarHarLinks · 2024-02-26T21:02:44Z

so 25fdc18 ended up reverting almost all previous changes. would you like me to squash it into all the previous ones? it should still be good to review (actually better to review in its entirety instead of incrementally) and may also be squashed when merging.

HarHarLinks · 2024-02-27T11:33:35Z

(not technically in scope of this Pr since it's about the avatar only, but)

with functional members, I can e.g. have a 1:1 DM between alice and frank ("Empty Room") but still search for Frank by displayname (that does not at all match their MXID) in spotlight and will still find the DM. maybe that is intentional? not sure how it does that.

richvdh · 2024-02-28T11:53:28Z

Don't worry about squashing, I'll just review it as-is, and we'll squash on merge.

src/models/room.ts

spec/unit/room.spec.ts

richvdh · 2024-02-28T12:20:13Z

spec/unit/room.spec.ts

+                }),
+            ]);
+            room.currentState.markOutOfBandMembersStarted();
+            room.currentState.setOutOfBandMembers([


why do we use ...OutOfBandMembers for this test rather than setStateEvents? Maybe we need a separate test to check the out-of-band-members case?

Because I copied it from the test above 🙃 I'm not sure what the difference is in the first place. What are they, and do I need to do something about them?

I wish I knew... let's ignore them for now

Co-authored-by: Richard van der Hoff <[email protected]>

instead of iterating redundantly

richvdh

Looking much better now, nearly there!

src/models/room.ts

Co-authored-by: Richard van der Hoff <[email protected]>

HarHarLinks · 2024-03-13T11:03:18Z

Thank you for taking the time to work on this with me!

richvdh

Thank you!

richvdh · 2024-03-13T16:57:19Z

src/models/room.ts

-            return;
-        }
+        let nonFunctionalMemberCount = 0;
+        this.getMembers()!.forEach((m) => {


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

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.

* v31.5.0-rc.0 * Don't re-fetch thread root if we already have it (matrix-org#4088) The root event of a thread used to arrive with the pagination request, but this was unspecced and so got changed to simply fetch the root event. In many (almost all) cases this shouldn't be necessary because the thread should already have its root event: re-use it if it's already there. This is only in pagination, so there's no reason to believe that the root event would have changed and needs to be re-fetched. This removes a number of duplicate calls to the /event/ endpoint from the tests. * Fix race condition with sliding sync extensions (matrix-org#4089) * Fix race condition with sliding sync extensions * Fix types on sliding-sync spec test * Prettier fixes * Add `.m.rule.is_room_mention` push rule to DEFAULT_OVERRIDE_RULES (matrix-org#4100) * Add intentional mentions push rules to DEFAULT_OVERRIDE_RULES Signed-off-by: Michael Telatynski <[email protected]> * Iterate Signed-off-by: Michael Telatynski <[email protected]> * Iterate Signed-off-by: Michael Telatynski <[email protected]> * Iterate Signed-off-by: Michael Telatynski <[email protected]> * Iterate Signed-off-by: Michael Telatynski <[email protected]> --------- Signed-off-by: Michael Telatynski <[email protected]> * Export types describing all specced media event formats (matrix-org#4092) * Export types describing all specced media event formats Signed-off-by: Michael Telatynski <[email protected]> * Iterate PR Signed-off-by: Michael Telatynski <[email protected]> * Move types to a dedicated export Signed-off-by: Michael Telatynski <[email protected]> * Iterate Signed-off-by: Michael Telatynski <[email protected]> * Add readme entry Signed-off-by: Michael Telatynski <[email protected]> --------- Signed-off-by: Michael Telatynski <[email protected]> * Update dependency typedoc to v0.25.11 (matrix-org#4102) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * v31.5.0 * Resetting package fields for development * Temporarily disable broken step in the release process Signed-off-by: Michael Telatynski <[email protected]> * fix automatic DM avatar with functional members (matrix-org#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]> --------- Signed-off-by: Michael Telatynski <[email protected]> Co-authored-by: RiotRobot <[email protected]> Co-authored-by: David Baker <[email protected]> Co-authored-by: Daniel Salinas <[email protected]> Co-authored-by: Michael Telatynski <[email protected]> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Kim Brose <[email protected]> Co-authored-by: Richard van der Hoff <[email protected]>

github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Jan 19, 2024

HarHarLinks mentioned this pull request Jan 19, 2024

fix automatic DM avatar with functional members matrix-org/matrix-react-sdk#12157

Merged

3 tasks

HarHarLinks force-pushed the fix-functional-members-avatar branch from 5ddccdd to efcbe4f Compare February 6, 2024 17:37

HarHarLinks marked this pull request as ready for review February 16, 2024 16:51

HarHarLinks requested a review from a team as a code owner February 16, 2024 16:51

HarHarLinks requested review from t3chguy and robintown February 16, 2024 16:51

HarHarLinks added 4 commits February 16, 2024 17:51

fix automatic DM avatar with functional members

0ce2d82

update comments

f9b41f6

lint

e65fb24

add tests for functional members

c114bf5

HarHarLinks force-pushed the fix-functional-members-avatar branch from efcbe4f to c114bf5 Compare February 16, 2024 16:51

richvdh requested changes Feb 23, 2024

View reviewed changes

src/models/room.ts Outdated Show resolved Hide resolved

Merge branch 'develop' into fix-functional-members-avatar

92aef67

github-actions bot deployed to PR Documentation Preview February 26, 2024 13:35 View deployment

HarHarLinks added 4 commits February 26, 2024 21:40

filter functional members from more candidates

d6f83d3

- remove from hero(es) - remove from previous members

add tests for fallback avatars with functional members

657f1bc

Merge branch 'develop' into fix-functional-members-avatar

9234e82

HarHarLinks requested a review from richvdh February 26, 2024 21:03

richvdh added T-Task Tasks for the team like planning and removed T-Task Tasks for the team like planning labels Feb 28, 2024

richvdh requested changes Feb 28, 2024

View reviewed changes

richvdh added the T-Enhancement label Feb 28, 2024

HarHarLinks and others added 3 commits February 28, 2024 18:02

Add docstring for getFunctionalMembers

5794809

Co-authored-by: Richard van der Hoff <[email protected]>

inline getInvitedAndJoinedFunctionalMemberCount

0789a23

update comments for getAvatarFallbackMember

b2b5b18

HarHarLinks added 6 commits February 28, 2024 18:28

use correct list of heroes in getAvatarFallbackMember

fa4666c

remove redundant type annotation

404d671

optimize performance of invitedAndJoinedFunctionalMemberCount

0f6ab78

calculate nonFunctionalMemberCount in one step

8db6b47

instead of iterating redundantly

clean up functional member tests with review feedback

66157c4

lint

684dcac

HarHarLinks requested a review from richvdh March 5, 2024 14:53

richvdh requested changes Mar 6, 2024

View reviewed changes

src/models/room.ts Show resolved Hide resolved

src/models/room.ts Outdated Show resolved Hide resolved

src/models/room.ts Outdated Show resolved Hide resolved

github-actions bot deployed to PR Documentation Preview March 6, 2024 21:27 View deployment

HarHarLinks and others added 4 commits March 12, 2024 23:51

Update src/models/room.ts

0ea0d50

Co-authored-by: Richard van der Hoff <[email protected]>

apply feedback about comments

efca7ba

non-functional per review, lint

882f625

Merge branch 'develop' into fix-functional-members-avatar

5f8a985

HarHarLinks requested a review from richvdh March 12, 2024 23:04

richvdh approved these changes Mar 13, 2024

View reviewed changes

richvdh enabled auto-merge March 13, 2024 16:57

github-actions bot deployed to PR Documentation Preview March 13, 2024 16:58 View deployment

richvdh added this pull request to the merge queue Mar 13, 2024

Merged via the queue into matrix-org:develop with commit 8e0ef5f Mar 13, 2024
20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix automatic DM avatar with functional members #4017

fix automatic DM avatar with functional members #4017

HarHarLinks commented Jan 19, 2024 •

edited by richvdh

Loading

richvdh left a comment

HarHarLinks commented Feb 26, 2024 •

edited

Loading

HarHarLinks commented Feb 27, 2024 •

edited

Loading

richvdh commented Feb 28, 2024

richvdh Feb 28, 2024

HarHarLinks Feb 28, 2024

richvdh Feb 29, 2024

richvdh left a comment

HarHarLinks commented Mar 13, 2024

richvdh left a comment

richvdh Mar 13, 2024

HarHarLinks Mar 13, 2024 •

edited

Loading

fix automatic DM avatar with functional members #4017

fix automatic DM avatar with functional members #4017

Conversation

HarHarLinks commented Jan 19, 2024 • edited by richvdh Loading

Checklist

richvdh left a comment

Choose a reason for hiding this comment

HarHarLinks commented Feb 26, 2024 • edited Loading

HarHarLinks commented Feb 27, 2024 • edited Loading

richvdh commented Feb 28, 2024

richvdh Feb 28, 2024

Choose a reason for hiding this comment

HarHarLinks Feb 28, 2024

Choose a reason for hiding this comment

richvdh Feb 29, 2024

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

HarHarLinks commented Mar 13, 2024

richvdh left a comment

Choose a reason for hiding this comment

richvdh Mar 13, 2024

Choose a reason for hiding this comment

HarHarLinks Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

HarHarLinks commented Jan 19, 2024 •

edited by richvdh

Loading

HarHarLinks commented Feb 26, 2024 •

edited

Loading

HarHarLinks commented Feb 27, 2024 •

edited

Loading

HarHarLinks Mar 13, 2024 •

edited

Loading