-
Notifications
You must be signed in to change notification settings - Fork 246
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
room list service: always request the room name event #3424
room list service: always request the room name event #3424
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3424 +/- ##
==========================================
- Coverage 82.98% 82.97% -0.02%
==========================================
Files 247 247
Lines 25019 25012 -7
==========================================
- Hits 20763 20754 -9
- Misses 4256 4258 +2 ☔ View full report in Codecov by Sentry. |
@@ -1194,7 +1194,7 @@ mod tests { | |||
|
|||
// When I send sliding sync response containing an invited room | |||
let mut room = v4::SlidingSyncRoom::new(); | |||
set_room_invited(&mut room, user_id); | |||
set_room_invited(&mut room, user_id, user_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This look like a bit weird, inviter
and invitee
are distinct users, no? (repeated 3 times, line 1221 and 1244). So maybe create a distinct user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh this set_room_invited
is a test function, and I've checked that all those tests don't actually care about the inviter/invitee distinction, since they observe other side-effects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, thanks for checking!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
I confirm that Also avatar of room where user is invited are correctly rendered. |
…w name event If we want to be able to note the absence of a room name, we shouldn't take the name returned by the server (which may be computed, and thus always be non-null). This way, we can expose both the name contained in the m.room.name event as well as the computed name everywhere. A consequence of this is that the room list service must now ask for the m.room.name event as part of the required state for every room.
…it's always provided It's always provided in the `avatar` response in the field, which sliding sync's using by default, so there's no need to request the `m.room.avatar` event too.
b212613
to
85e1b1f
Compare
And don't rely on the
name
field in the response as provided by the server. The name displayed by the server was before saved as them.room.name
event for that room, which is not the right semantics when the name has been computed by the server. The computation of a name should happen client-side, so as to be able to translate it according to the user's language.Instead, this PR makes it so that we ignore the
name
field from the sliding sync response, and instead request them.room.name
event for all the rooms in the room list service.Conversely, the
m.room.avatar
is always set to theavatar
field from a room sub-part of a response, and it's likely more precise than the actual content of them.room.avatar
(it will user the other user avatar for a DM room). We could also compute that client-side, but since the server does it for us, and its default choice is Good™, we use that instead. As a result, the state event was requested but always unused, so we can stop requesting it explicitly in the room list / notification services.