base: cache the computed display name and reuse that in the search filters #3516
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
That's new, the CI task is not looking at the right version of the code… |
I think we had it a couple of times, it might happen if the base branch of your PR is too far off from main on the repo. Try a rebase? |
Instead of calling it through `BaseRoomInfo::calculate_room_name`.
…rs, not joined+invited The previous code would use `ACTIVE` which means "either joined or invited" members, but the code thereafter considered this to be the number of joined members. Also replaced a call to `self.members()` (which does a lot of work under the hood) with a call to `self.joined_user_ids()`, since we only retrieve the `.len()` of the resulting vector.
bnjbvr
force-pushed
the
bnjbvr/cached-computed-display-name
branch
from
4b843e3 to
81d0e7c
Compare
Since this is a good place where to recompute the cached display name at start.
… computed_ in the cached one
bnjbvr
force-pushed
the
bnjbvr/cached-computed-display-name
branch
from
81d0e7c to
823226a
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3516 +/- ##
==========================================
+ Coverage 83.79% 83.83% +0.04%
==========================================
Files 254 254
Lines 25723 25739 +16
==========================================
+ Hits 21554 21579 +25
+ Misses 4169 4160 -9 ☔ View full report in Codecov by Sentry. |
… way The test looks at updates of `RoomInfo`s, but these depends on external factors like the server sending them in one part or multiple ones. Instead of trying to figure out which partial updates the server sent, wait for the stream of `RoomInfo`s to stabilize by trying to get the latest item from the stream, then wait up to N seconds for another item to show up, and continue as long as items come in. This should allow us to get rid of some code that was required to prevent flakey updates.
|
@poljar This is now ready for review \o/ |
poljar
reviewed
Jun 7, 2024
…he cached display name in RoomInfo So revert a few changes to make some functions async, etc.
…c methods It's not perfect, but it's honest work.
bnjbvr
force-pushed
the
bnjbvr/cached-computed-display-name
branch
from
51a599b to
5001b27
Compare
poljar
approved these changes
Jun 11, 2024
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Could be reviewed commit by commit. It gets messy only at the end. GLHF.
The computed display name is filled in two cases:
This means that the first time we see a room during a sync, we'll compute the display name twice 🤷
Recomputing on every sync might be a bit wasteful, but getting all the inputs right when computing the display name is a tad complicated, since this depends on the room summary, the members, the heroes, the room name event and the canonical alias event (and maybe other things I'm forgetting about right now). Alternatives involved: using a dirty flag (bleh), also keeping all the inputs along the cached output (so we could early return when the inputs haven't changed in
compute_display_name— could work, but seems frail), making the search filters async (nopenopenope).