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

Context menu 2: add support for multiple selection #5205

Merged
merged 6 commits into from
Feb 16, 2024

Conversation

abey79
Copy link
Member

@abey79 abey79 commented Feb 15, 2024

What

image

This PR adds support for multiple selection with context menus in blueprint view.

The selection logic is as follows:

  • If the (right-)clicked item is part of the selection, the context menu actions apply to the entire selection.
  • Otherwise, the selection is set to the clicked item, and the context menu actions apply to that item only.

This PR doesn't introduce any new action, but adjust the existing ones to handle multi-selection:

  • If only space views and/or containers are selected, the visibility toggle and Remove actions are offered. The visible toggle will offer to show if one or more items are currently hidden, and hide if all are currently visible. If the root container is part of the multi-selection, its is ignored by those actions.
  • The "Add SV/Container" actions are offered only if exactly one container item is selected.

TODO in future PR:

  • additional actions
  • entity level manipulations
  • use ListItem instead of label/WidgetText
  • better folder structure
  • release checklist update

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
  • If applicable, add a new check to the release checklist!

@abey79 abey79 added ui concerns graphical user interface include in changelog labels Feb 15, 2024
@teh-cmc teh-cmc self-requested a review February 16, 2024 08:05
@teh-cmc
Copy link
Member

teh-cmc commented Feb 16, 2024

Is there something we can do to prevent the hover pop up in that case?
image

@teh-cmc
Copy link
Member

teh-cmc commented Feb 16, 2024

Something super weird happens sometimes that i cannot quite explain in words:

24-02-16_09.14.37.patched.mp4

@teh-cmc
Copy link
Member

teh-cmc commented Feb 16, 2024

If you do an heterogeneous selection and then right-click, the context menu pops up in the form of an empty bubble for a frame, it's a bit jarring:

24-02-16_09.30.38.patched.mp4

Copy link
Member

@teh-cmc teh-cmc left a comment

Choose a reason for hiding this comment

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

I had no idea all of these things existed 😄 pretty exciting what one can do in the blueprint panel these days 👀

See my other comments for some usage bugs I've found.
I don't think any of them are blocking.

The only one I'm worried is the only where the selection seems to be changing on its own somehow? That might hide a real nasty thing.

I do think the whole context menu thing really really needs a dedicated check in the checklist though (emphasis on dedicated: keep checks as atomic as possible!).

crates/re_viewport/src/context_menu.rs Outdated Show resolved Hide resolved
crates/re_viewport/src/context_menu.rs Outdated Show resolved Hide resolved
@abey79
Copy link
Member Author

abey79 commented Feb 16, 2024

Is there something we can do to prevent the hover pop up in that case?
image

On my list of things to fix in follow-up PRs. I guess we can remove these one entirely and they serve no purpose. The ones that actually are useful is entities, so I will need to address that properly.

@emilk any input on how to deal with hover tooltip vs. context menu conflict?

@abey79
Copy link
Member Author

abey79 commented Feb 16, 2024

The only one I'm worried is the only where the selection seems to be changing on its own somehow? That might hide a real nasty thing.

Yeah that's ugly. Looking into it.

I do think the whole context menu thing really really needs a dedicated check in the checklist though (emphasis on dedicated: keep checks as atomic as possible!).

I originally intended to add this in some later wrap-up PR but I guess I'll introduce it already now. It'll be easier to keep track of the various edge cases as I pour more stuff into those context menu.

@abey79
Copy link
Member Author

abey79 commented Feb 16, 2024

If you do an heterogeneous selection and then right-click, the context menu pops up in the form of an empty bubble for a frame, it's a bit jarring:

For this one, I opted to show an explicitly empty context menu:

image

Slightly less surprising that showing nothing, which would have the additional technical draw back to require computing the list of possible actions for every list item drawn, _every_frame. Currently, this happens only once, and only when a context menu is actually displayed.

…ion, show an empty context menu with a note instead of flashing a tiny bubble.
@abey79
Copy link
Member Author

abey79 commented Feb 16, 2024

The only one I'm worried is the only where the selection seems to be changing on its own somehow? That might hide a real nasty thing.

Yeah that's ugly. Looking into it.

Fixed. Thanks @emilk for the help.

@abey79 abey79 merged commit d7196c8 into main Feb 16, 2024
40 checks passed
@abey79 abey79 deleted the antoine/cm2-multi-selection branch February 16, 2024 14:09
abey79 added a commit that referenced this pull request Feb 16, 2024
### What

* Follow-up to #5205
* Part of #4823


![Export-1708013884748](https://github.com/rerun-io/rerun/assets/49431240/c3e6485a-48c6-4f38-b5c2-907f4c546346)

<br/>
<br/>

This PR adds a new "Move to new container" context menu action, that
applies when one or more space views and/or containers are selected. The
new container is created at the location of the clicked item, and all
the selected items are moved into it.

**Note**: one very often runs into this while using this feature:
- #5208

TODO in future PR:
- additional actions
- entity level manipulations
- use ListItem instead of label/WidgetText
- better folder structure
- release checklist update

### 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 newly built examples:
[app.rerun.io](https://app.rerun.io/pr/5210/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/5210/index.html?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[app.rerun.io](https://app.rerun.io/pr/5210/index.html?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/5210)
- [Docs
preview](https://rerun.io/preview/d842739a5e559177ddc2b0f682b58df6a40076b3/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/d842739a5e559177ddc2b0f682b58df6a40076b3/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
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