-
Notifications
You must be signed in to change notification settings - Fork 265
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
feat(ui): Add m.room.tombstone
to the room list required_state
#4211
base: main
Are you sure you want to change the base?
Conversation
This patch adds the `m.room.tombstone` state event to the list of events in `required_state` used by the `RoomListService`. The goal is to offer the possibility for the consumers to know whether a room has been tombstoned or not.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4211 +/- ##
==========================================
- Coverage 84.89% 84.87% -0.02%
==========================================
Files 272 272
Lines 29142 29142
==========================================
- Hits 24739 24734 -5
- Misses 4403 4408 +5 ☔ View full report in Codecov by Sentry. |
Echoing my comments from the Matrix Rust SDK room here, as requested: I tested this PR out in Robrix, but unfortunately was not able to get any tombstoned info at all. Here's what I'm doing, just in case I missed a key step. When I receive a diff with a new room from the room list service, i then attempt to call several different tombstone-related functions (which I think are all doing mostly the same thing under the hood):
i know for a fact that I do have tombstoned rooms on the account that I'm testing with, so I'm expecting them to have tombstoned room state. Here's a side-by-side example showing how both rooms exist as separate rooms in the same session: the new version of Element Web/Desktop on the left, and the old tombstoned one on the right. If I understand tombstoning correctly, I'd expect the old version of the room (on the right) to have a tombstone state event. |
What's the state of this PR? Do we need this or do we expect users to enable tombstone events themselves?
Are you sure that the server sent the event to you? As far as I can see, we don't treat those events any differently from other important room state events, if we receive it we put it in the cache. |
Hi @poljar, thanks for the response. I would argue that tombstone state should be a part of the sync service's required state, since without it, the client may accidentally display multiple old tombstoned rooms as separate unrelated rooms without being able to properly determine that they're replacements of one another.
I'm not 100% sure, no. I think that's part of the problem -- even with this change, I don't get any tombstone events from any rooms. How might I accurately determine whether I've received the tombstone events from the server? (besides using the existing SDK APIs for this, like https://matrix-org.github.io/matrix-rust-sdk/matrix_sdk/struct.BaseRoom.html#method.tombstone) |
I like to use |
@erikjohnston Do you know if sliding sync on synapse filters out the |
I wonder if this can be solved by element-hq/synapse#17540. |
This patch adds the
m.room.tombstone
state event to the list of events inrequired_state
used by theRoomListService
. The goal is to offer the possibility for the consumers to know whether a room has been tombstoned or not.