Collapsible small state #118 2#637
Closed
alanpoon wants to merge 32 commits intoproject-robius:mainfrom
Closed
Conversation
kevinaboos
requested changes
Dec 30, 2025
Member
kevinaboos
left a comment
There was a problem hiding this comment.
Thanks Alan, I appreciate your work on this!
I have a few high-level comments that apply to the general design of this PR:
- There is already an existing data structure designed to support this particular use case:
RangeMap, which is much easier to use and more efficient for lookups than a vector of ranges.- This is particularly useful here because there are going to be many small ranges of state events, and we should prioritize fast lookup of a given index over everything else.
- The crate is also called
rangemap, see docs here: https://docs.rs/rangemap/latest/rangemap/map/struct.RangeMap.html. I have used it in previous projects, and it works very well. - As a bonus, I think you'll find that using
rangemapwill allow you to simplify a lot of the code in thesmall_state_group_manager, which is currently too complicated to easily understand & maintain.
- When you create a new range of grouped SmallStateEvent items, you are doing it incrementally in very small batches by looking at only the previous item and the next item (L4014-4015 of room_screen.rs). While it is typically best to do things incrementally, especially when drawing, this is one rare case where it is worse to do so, and it also happens to be incorrect. There are two reasons for this:
- There is a fixed overhead cost of creating a group, and also a fixed overhead when looking up whether an item index is already part of a group. Instead of creating and updating the group gradually, it is much faster to iterate over all previous/next events to find the entire range of SmallStateEvents that can be combined into one group, and then just create that group all at once.
- Doing it incrementally will also lead to visually incorrect/misleading content. If you only consider the item right before and after a given event, then the summary text that you generate for those events will be based on how far the user has scrolled (which timeline items are being drawn) rather than the actual full content of the timeline. This will result in a strange user experience, in which the user will see changes to the small state group summary based on how far up or down they have scrolled, which can generally be considered incorrect.
- Using
!is_appendto determine if the indices need to be updated is not always correct, unfortunately. That boolean merely indicates that if it is true, a given update only resulted in appending new items to the existing set. It does not work in the opposite way in all cases.- Instead, you need to use the combination of
clear_cacheandchanged_indicesto determine which indices have changed. Ifclear_cacheis true, then all indices have changed, which most frequently occurs due to back pagination. Ifclear_cacheis false, then you can look atchanged_indicesto see the range of indices that need to be redrawn, and you'll want to pass that range into the small state event group manager so that it can invalidate any cached group summaries whose ranges intersect with that changed range.
- Instead, you need to use the combination of
- The UI design for collapsing/expanding a grouped summary event is okay, in that it works, but it's a bit confusing and non-obvious. I'm not sure why it strikes me as hard to find/use, but I bet that many users will not notice it or not realize what it is for. On a similar note, the feedback we've gotten from users/designers who have looked at Robrix is that there is a lack of consistent UI design across different elements, and this is one example of that.
- Thus, to be consistent, I propose that you use the same UI design for the list of link previews beneath a message. Not only will that be more consistent, but it's also easier to see a larger button that is underneath an event summary than a small triangle on the far right side.
afbac0d to
d792363
Compare
Member
|
As of #662, redacted messages use the regular |
Contributor
Author
|
Closed, in favor of #667. |
This file contains hidden or 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
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.
Fixes #118
Screen.Recording.2025-12-05.at.8.01.07.PM.mov
As it is very difficult to add the collapsible header as a separate item into the portal list, the small state event contains the collapsible header. Extra logic is used to decide to show the small state event's body or the collapsible header.