Skip to content
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

Filter entities in the UI (part 2): Introduce entity filtering in the time panel #8654

Merged
merged 10 commits into from
Jan 13, 2025

Conversation

abey79
Copy link
Member

@abey79 abey79 commented Jan 10, 2025

Related

What

This PR introduces the filter widget to the time panel. In doing so, it deals with several shenanigans:

  • The item collapsedness must not be shared between the unfiltered view, and across different filtering sessions
    Note: the collapsed state being persisted, we are now accumulating collapsedness state for every single filter session in the persistant storage. This is not ideal, but addressing that would required rolling our own collapsed state, which would leak all over the place (list item and many of its users).
  • The collapse/expend all context menu must be made aware of what precedes.
  • The time panel filter state must be invalidated upon switching active recording (otherwise it's weird UX).
  • The streams tree UI area needs to have a minimum width otherwise search icon placement/layout breaks.

And of course the actual filtering logic must be implement. For now, the semantics are: "one of the entity part must contain the query string in its entirety". This is very (too) basic and will evolve in future PR (see #8586 (comment)).

streams_tree_filter.mp4

Next step

Major next steps:

  • have filters in more places (primarily blueprint tree)
  • make the filter semantics more useful

@abey79 abey79 added ui concerns graphical user interface do-not-merge Do not merge this PR include in changelog labels Jan 10, 2025
Copy link

github-actions bot commented Jan 10, 2025

Web viewer built successfully. If applicable, you should also test it:

  • I have tested the web viewer
Result Commit Link Manifest
9b1d3db https://rerun.io/viewer/pr/8654 +nightly +main

Note: This comment is updated whenever you push a commit.

@Wumpf Wumpf self-requested a review January 10, 2025 18:26
@abey79 abey79 force-pushed the antoine/filt2-streams-tree branch from 5553c9a to 1a57030 Compare January 11, 2025 12:07
@abey79 abey79 force-pushed the antoine/filt1-filter-widget branch 2 times, most recently from e0fa976 to 96535ff Compare January 13, 2025 09:03
@abey79 abey79 force-pushed the antoine/filt2-streams-tree branch from 1a57030 to d8d5ec4 Compare January 13, 2025 09:03
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, get to have this in

Unfortunately makes an existing perf issue more prominent: We try to show the histogram for everything that's expanded and once there's a search we expand everything. We need to figure out how to disable the histogram showing code but this isn't an issue introduced here.

Comment on lines 201 to 205
// Invalidate the filter widget if the store id has changed.
if self.filter_state_store_id != Some(entity_db.store_id()) {
self.filter_state = Default::default();
self.filter_state_store_id = Some(entity_db.store_id());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels wrong: Can we keep the id on the RecordingConfig so that we don't loose the filter when changing recording?
If a user wants to go back and forth between two similar recording this lack of state keeping can be very annoying.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I added that precisely because I wanted to disable the filter upon recording change, because it was extremely annoying to have it. Now that you mention it, I see how it could be useful to have it.

My suggestion would be to use the app id instead. Makes sense to you?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yeah that sounds great, then the filter sticks with the different recordings within the same app 💡

crates/viewer/re_time_panel/src/lib.rs Outdated Show resolved Hide resolved
@abey79 abey79 force-pushed the antoine/filt1-filter-widget branch from 96535ff to 9a0a88e Compare January 13, 2025 14:23
@abey79 abey79 force-pushed the antoine/filt2-streams-tree branch 2 times, most recently from d73b490 to 7b01985 Compare January 13, 2025 14:29
Base automatically changed from antoine/filt1-filter-widget to main January 13, 2025 15:51
@abey79 abey79 force-pushed the antoine/filt2-streams-tree branch from cbda2d2 to 9b1d3db Compare January 13, 2025 15:52
@abey79 abey79 removed the do-not-merge Do not merge this PR label Jan 13, 2025
@abey79 abey79 merged commit 7ddc480 into main Jan 13, 2025
33 of 34 checks passed
@abey79 abey79 deleted the antoine/filt2-streams-tree branch January 13, 2025 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
include in changelog ui concerns graphical user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants