-
Notifications
You must be signed in to change notification settings - Fork 41
Collapsible small state #118 2 #637
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
Open
alanpoon
wants to merge
27
commits into
project-robius:main
Choose a base branch
from
alanpoon:collapsible_small_state_#118_2
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Collapsible small state #118 2 #637
alanpoon
wants to merge
27
commits into
project-robius:main
from
alanpoon:collapsible_small_state_#118_2
+1,504
−10
Conversation
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
kevinaboos
requested changes
Dec 30, 2025
Member
kevinaboos
left a comment
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.
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.
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.