ListItem 2.0 (part 2): introduce PropertyContent for two-column, property-like list items
#6174
Conversation
This file contains 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
abey79
commented
May 1, 2024
Comment on lines
+271
to
+276
| // Make a union of all (possibly) interactive elements | ||
| match (value_response, button_response) { | ||
| (Some(a), Some(b)) => Some(a | b), | ||
| (Some(a), None) | (None, Some(a)) => Some(a), | ||
| (None, None) => None, | ||
| } |
There was a problem hiding this comment.
#6182 removes the return value entirely.
abey79
commented
May 1, 2024
Comment on lines
+11
to
+12
| type PropertyValueFn<'a> = | ||
| dyn FnOnce(&ReUi, &mut egui::Ui, egui::style::WidgetVisuals) -> Option<egui::Response> + 'a; |
There was a problem hiding this comment.
#6182 removes the return value as well, as its used was only to be forwarded to ListItemContent::ui(), which no longer needs it.
5 tasks
emilk
reviewed
May 2, 2024
| let visual_size = egui::vec2(12.0, 8.0); // 12x7 in figma, but 12x8 looks _much_ better in epaint | ||
| let visual_rect = | ||
| egui::Align2::CENTER_CENTER.align_size_within_rect(visual_size, interact_rect); | ||
| fn toggle_switch_ui(ui: &mut egui::Ui, height: f32, on: &mut bool) -> egui::Response { |
There was a problem hiding this comment.
shouldn't all our toggle switches have the same height, everywhere?
There was a problem hiding this comment.
We don't use it at all in rerun yet, but at least in re_ui_example I used it twice in contexts that very specifically call for a different size:
|
All review comment (except toggle size) addressed in #6183 |
abey79
added a commit
that referenced
this pull request
May 2, 2024
### What This PR: - Implements column auto-sizing for `PropertyContent`. The current heuristics is rather simple and will likely need improvement when it's battle tested. Currently, the content requests a minimum width of 200px (there really isn't a way to properly deal with two-column-plus-button neatly all the way to 0 width), and allocate a maximum of 70% of the available width to the label column. - Improves the way `ListItem` handles content capturing the hover state (which happen for interactive content). There is no longer a requirement for `ListItemContent::ui()` to return an `Option<Response>`, which simplifies a bunch of things. - Updates `re_ui_example` accordingly Note: still no change applied to the actual viewer code base so far. - Part of #6075 - Follow-up to #6174 <img width="502" alt="image" src="https://github.com/rerun-io/rerun/assets/49431240/28322870-997c-4bf4-997f-d0e7c8da2da9"> ### Limitations and todo - Improved heuristics for column sizing, when we have a better idea of the need. - More helpers are needed for various kinds of values. - There can be only 0 or 1 action button. This should be extended by using a `…` button with some kind of popup with all available actions in a future PR. - Right gutter space is reserved for the action button even if no list item in scope use them. The `list_item_scope` could track this and skip reserving that space if it's never used (e.g. component list in entity path selection panel): #6179 ### Checklist * [x] I have read and agree to [Contributor Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and the [Code of Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md) * [x] I've included a screenshot or gif (if applicable) * [x] I have tested the web demo (if applicable): * Using examples from latest `main` build: [rerun.io/viewer](https://rerun.io/viewer/pr/6182?manifest_url=https://app.rerun.io/version/main/examples_manifest.json) * Using full set of examples from `nightly` build: [rerun.io/viewer](https://rerun.io/viewer/pr/6182?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json) * [x] The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG * [x] If applicable, add a new check to the [release checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)! - [PR Build Summary](https://build.rerun.io/pr/6182) - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html) To run all checks from `main`, comment on the PR with `@rerun-bot full-check`.
abey79
added a commit
that referenced
this pull request
May 2, 2024
abey79
added a commit
that referenced
this pull request
May 2, 2024
…ons when needed (#6183) ### What ☝🏻 This PR also slightly adjust the semantics of the `id` parameter of `list_item_scope()`. Now it just needs to be unique within the current UI. It also addresses review comments from #6161, #6174, and #6182. - Part of #6075 - Follow-up to #6182 - Fixes #6179  <br/> Note: the upper group of (nested) items (which makes use of action buttons) has no impact on the lower group, because both sub-groups are in a distinct `list_item_scope`. ### Checklist * [x] I have read and agree to [Contributor Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and the [Code of Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md) * [x] I've included a screenshot or gif (if applicable) * [x] I have tested the web demo (if applicable): * Using examples from latest `main` build: [rerun.io/viewer](https://rerun.io/viewer/pr/6183?manifest_url=https://app.rerun.io/version/main/examples_manifest.json) * Using full set of examples from `nightly` build: [rerun.io/viewer](https://rerun.io/viewer/pr/6183?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json) * [x] The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG * [x] If applicable, add a new check to the [release checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)! - [PR Build Summary](https://build.rerun.io/pr/6183) - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html) To run all checks from `main`, comment on the PR with `@rerun-bot full-check`.
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.
What
This PR introduced the
PropertyContent, an implementation ofListItemContentfor two-column list item, with a label and flexible "values". Currently only demonstrated inre_ui_example.What the "value" displays is delegated to a user-provided closure. However,
PropertyContentprovides helper for a few basic types: bool, text, color (both read-only and editable).ListItem2.0 (part 1): introduce content-genericListItemandLabelContentlegacy back-port #6161Export-1714484618553.mp4
Limitations and todo
…button with some kind of popup with all available actions in a future PR.list_item_scopecould track this and skip reserving that space if it's never used (e.g. component list in entity path selection panel):PropertyContent: do not allocate space for acton button if no action button is ever used in the scope #6179Checklist
mainbuild: rerun.io/viewernightlybuild: rerun.io/viewerTo run all checks from
main, comment on the PR with@rerun-bot full-check.