ListItem 2.0 (part 3): PropertyContent column auto-sizing
#6182
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
emilk
approved these changes
May 2, 2024
Comment on lines
+33
to
+36
| /// Maximum desired column width, to be updated this frame. | ||
| /// | ||
| /// The semantics are exactly the same as for `left_column_width`. | ||
| max_desired_left_column_width: f32, |
There was a problem hiding this comment.
Does this every shrink, or does it only ever grow?
There was a problem hiding this comment.
It's reset to -inf each frame by the scope, and only grows during the frame according the needed width.
| // from real-world usage. | ||
| state | ||
| .max_desired_left_column_width | ||
| .at_most(0.7 * ui.max_rect().width()), |
There was a problem hiding this comment.
I think there should be a minimum width we don't go below (e.g. 32 points), and force the parent scope to have scroll bars instead
Suggested change
| .at_most(0.7 * ui.max_rect().width()), | |
| .at_most(0.7 * ui.max_rect().width()), | |
| .at_least(MIN_LEFT_COLUMN_WIDTH) |
There was a problem hiding this comment.
I agree in principle, but suggest postponing any complexity-inducing improvement to when we actually reach into real world issues. I find very hard to predict how things will break exactly.
abey79
added a commit
that referenced
this pull request
May 2, 2024
|
All review comment addressed in #6183 (except the suggested postpone ☝🏻) |
abey79
added a commit
that referenced
this pull request
May 2, 2024
…property-like list items (#6174) ### What This PR introduced the `PropertyContent`, an implementation of `ListItemContent` for two-column list item, with a label and flexible "values". Currently only demonstrated in `re_ui_example`. What the "value" displays is delegated to a user-provided closure. However, `PropertyContent` provides helper for a few basic types: bool, text, color (both read-only and editable). - Part of #6075 - Follow-up to #6161 https://github.com/rerun-io/rerun/assets/49431240/bf94871a-63d5-46fa-94fe-a9adf720cdb8 ### Limitations and todo - Columns are fixed size at 50%. They will be made smart in [the next PR.](#6182) - 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/6174?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/6174?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/6174) - [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`.
The current heuristics simply caps the label column to be 70%
abey79
force-pushed
the
antoine/li3-smart-columns
branch
from
cd2acf6 to
6228024
Compare
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:
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.ListItemhandles content capturing the hover state (which happen for interactive content). There is no longer a requirement forListItemContent::ui()to return anOption<Response>, which simplifies a bunch of things.re_ui_exampleaccordinglyNote: still no change applied to the actual viewer code base so far.
ListItem2.0 (part 2): introducePropertyContentfor two-column, property-like list items #6174Limitations 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.