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 1: Basic scaffolding and simple actions #5163

Merged
merged 7 commits into from Feb 15, 2024
Merged

Conversation

abey79
Copy link
Contributor

@abey79 abey79 commented Feb 9, 2024

What

This PR is the first of a series to implement context menus in the Blueprint Tree. Here we setup the basic scaffolding and implement some basic actions:

  • Add SV
  • Add container
  • Show/hide
  • Remove
image

The selection is currently ignore and the actions only apply to the clicked item. TODO in future PRs:

  • improved (multi-)selection handling
  • 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 12, 2024
@abey79 abey79 changed the title Add contextual menu for common operations in the Blueprint tree UI Context Menu 1: Basic scaffolding and basic actions Feb 13, 2024
@abey79 abey79 changed the title Context Menu 1: Basic scaffolding and basic actions Context Menu 1: Basic scaffolding and simple actions Feb 13, 2024
@abey79 abey79 marked this pull request as ready for review February 13, 2024 10:38
@abey79 abey79 marked this pull request as draft February 13, 2024 15:49
@abey79 abey79 marked this pull request as ready for review February 14, 2024 10:38
@Wumpf Wumpf self-requested a review February 14, 2024 11:20
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.

The context menu allows you now to hide the top level container, something that isn't possible via a button and has no visual feedback.

More shockingly, you can also remove it and then not add a new one anymore 😱

@abey79
Copy link
Contributor Author

abey79 commented Feb 14, 2024

The context menu allows you now to hide the top level container, something that isn't possible via a button and has no visual feedback.

More shockingly, you can also remove it and then not add a new one anymore 😱

🤦🏻

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.

Great start for context menus! I like the overall structure; left only a few thoughts, can go in as-is imho :)


/// Trait for things that can populate a context menu
trait ContextMenuItem {
//TODO(ab): should probably return `egui::WidgetText` instead
Copy link
Member

Choose a reason for hiding this comment

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

so.. why not? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That comment is actually wrong. The end-game is to return a ListItem (with all the fanciness that comes with it: nicer look, icon, etc.). Both ListItem and the egui context menu code needs a bit of a overhaul for this to work though, so it'll come as a follow-up.

}

// ================================================================================================
// Utility items
Copy link
Member

Choose a reason for hiding this comment

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

guess it's a matter of taste but I would have started a subfolder for context menu with modules for each of these item categories
It looks pretty much like this gonna be another thing where one dynamically registers elements, so might as well already structure it like one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'll do that in a follow up PR to avoid too many rebasing annoying with my existing follow-up.

/// Trait for things that can populate a context menu
trait ContextMenuItem {
//TODO(ab): should probably return `egui::WidgetText` instead
fn label(&self, _ctx: &ViewerContext<'_>, _viewport_blueprint: &ViewportBlueprint) -> String {
Copy link
Member

Choose a reason for hiding this comment

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

Just a label is a bit little though? Should add icons in a follow-up pr so we can show the familiar space view and layout icons - they are much faster to parse than text

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup—see my other comment.

@abey79 abey79 merged commit 4dc75b8 into main Feb 15, 2024
42 checks passed
@abey79 abey79 deleted the antoine/context-menu branch February 15, 2024 10:03
abey79 added a commit that referenced this pull request Feb 16, 2024
### What

* Follow up to #5163
* Part of #4823

<img width="260" alt="image"
src="https://github.com/rerun-io/rerun/assets/49431240/f911f4ca-a33e-4e7a-85a9-80f126148be4">
<br/>
<br/>

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

* [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/5205/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/5205/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/5205/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/5205)
- [Docs
preview](https://rerun.io/preview/291a52d41703e7e70f1de7d905754ff6ac46cdcb/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/291a52d41703e7e70f1de7d905754ff6ac46cdcb/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.

None yet

2 participants