Skip to content

Conversation

@alanpoon
Copy link
Contributor

@alanpoon alanpoon commented Dec 5, 2025

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.

@alanpoon alanpoon self-assigned this Dec 5, 2025
@alanpoon alanpoon added the waiting-on-author This issue is waiting on the original author for a response label Dec 5, 2025
@alanpoon alanpoon added waiting-on-review This issue is waiting to be reviewed and removed waiting-on-author This issue is waiting on the original author for a response labels Dec 25, 2025
Copy link
Member

@kevinaboos kevinaboos left a 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:

  1. 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 rangemap will allow you to simplify a lot of the code in the small_state_group_manager, which is currently too complicated to easily understand & maintain.
  2. 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:
    1. 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.
    2. 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.
  3. Using !is_append to 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_cache and changed_indices to determine which indices have changed. If clear_cache is true, then all indices have changed, which most frequently occurs due to back pagination. If clear_cache is false, then you can look at changed_indices to 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.
  4. 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.

@kevinaboos kevinaboos added waiting-on-author This issue is waiting on the original author for a response and removed waiting-on-review This issue is waiting to be reviewed labels Dec 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting-on-author This issue is waiting on the original author for a response

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Collapsible/expandable sequence of small events

2 participants