Skip to content

Unify menus with tooltips/popups #4607

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

Closed
emilk opened this issue Jun 3, 2024 · 5 comments · Fixed by #5716 · May be fixed by #4669 or #5758
Closed

Unify menus with tooltips/popups #4607

emilk opened this issue Jun 3, 2024 · 5 comments · Fixed by #5716 · May be fixed by #4669 or #5758
Assignees
Labels
feature New feature or request refactor rerun Desired for Rerun.io

Comments

@emilk
Copy link
Owner

emilk commented Jun 3, 2024

egui menus (Ui::menu_button) and context menus (Response::context_menu) are implemented using the egui::menu module.

There are also tooltips (Response::on_hover_ui etc) and ComboBox popups, both implemented using egui::containers::popup.

It would be nice to unify these systems.

Things I would like to accomplish:

  • Being able to open a popup on:
    • hover (tooltip)
    • click (menu)
    • right-click or long-press (context menu)
  • Nest sub-menus everywhere (currently not possible in tooltips)
  • Call ui.close_menu to close a tooltip and popup (currently only works for menus)

At the same time it would be great to take a naming pass on all these things, and look up what they are commonly called in other ui libraries.

@Umatriz
Copy link
Contributor

Umatriz commented Jun 7, 2024

I would like to try to implement this. But I also want to be able to change the way egui::containers::popups are closed. Because for now if I add any kind of checkboxed into it or anything else that requires some kind of interaction the popup will immediately close as soon as I change something (mark a checkbox or press a button). I think it's better to let user to take control of it. Simitarly to the ui.menu_button and it's ui.close_menu.

@Umatriz
Copy link
Contributor

Umatriz commented Jun 7, 2024

I think I will make a PR and write all my thoughts about this isssue.

@emilk
Copy link
Owner Author

emilk commented Jun 7, 2024

Nice! I suspect there is a lot in menu.rs that can be simplified too

@emilk emilk added this to the Next Major Release milestone Jun 19, 2024
@emilk emilk added the rerun Desired for Rerun.io label Jun 19, 2024
Wumpf added a commit to rerun-io/rerun that referenced this issue Jun 20, 2024
### What

Previously, we had ui for adding component overrides, under an
experimental feature everywhere except the time series view + some
hardcoded ui for manipulating overrides (those that were
`EntityProperties` in 0.16).

This PR removes both the component override ui and the hardcoded
property ui in favor of a new overview + override ui showing all
currently used values by all active visualizers + ability to add/remove
visualizers.


https://github.com/rerun-io/rerun/assets/1220815/425a9406-9eb2-4319-846b-fe0d0eee1a42

---

Noteworthy "sideeffects":
* `ComponentUiCallback` now uses raw arrow arrays, allowing it to show
fallbacks
* kills lots'a old code :)
* `VisualizerQueryInfo::queried` keeps now archetype component ordering
instead of alphabetical (not in above video yet)
* lots of inconsistency in our data flow are now visible

---

Planned direct follow-ups:
* allow various reset / override menus 
* particularly important: remove override, add (single) override on
multi-value
* Would be great to do this with a menu button, will have to be a
context menu until emilk/egui#4607 is handled
* allow expanding to see the override/store/default/fallback stack
* original plan was to skip on this, but the method has all the data
layed out neatly now, so might as well!
* get the "Add" button integrated into the top (unless this is hard?)
*  show better docs: #6556
* Find a better name for the "blueprint section" below

---

Also needed but not planned short term:
* inspect multivalues
* we could do so for store multivalues already, but everything else we
can't yet select, so let's tackle this later
* ui polish

---

Part of:
* #4888

### 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/6599?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/6599?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/6599)
- [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`.
emilk pushed a commit that referenced this issue Jun 27, 2024
This PR adds `PopupCloseBehavior` to improve state of the
<#4607>

`PopupCloseBehavior` determines when popup will be closed.
- `CloseOnClick` popup will be closed if the click happens anywhere even
in the popup's body
- `CloseOnClickAway` popup will be closed if the click happens somewhere
else but in the popup's body.

It also adds a test in the demo app which contains several popups
examples.

---

My ideas about <#4607> is to make
every tooltip and popup a menu. So it will provide more control over
popups and tooltips (you will be able to close a popup by calling
something similar to the `ui.close_menu` if you need to). You won't need
to manually handle it's opening. And also will allow to have multiple
popups opened. That means you can have a popup inside a popup. And it
will also lead to the easier creation of the popups. (should we create a
tracking issue to track changes because to me it seems like a huge
amount of changes to be done?)

---

- Improvements on <#4607>
hacknus pushed a commit to hacknus/egui that referenced this issue Oct 30, 2024
This PR adds `PopupCloseBehavior` to improve state of the
<emilk#4607>

`PopupCloseBehavior` determines when popup will be closed.
- `CloseOnClick` popup will be closed if the click happens anywhere even
in the popup's body
- `CloseOnClickAway` popup will be closed if the click happens somewhere
else but in the popup's body.

It also adds a test in the demo app which contains several popups
examples.

---

My ideas about <emilk#4607> is to make
every tooltip and popup a menu. So it will provide more control over
popups and tooltips (you will be able to close a popup by calling
something similar to the `ui.close_menu` if you need to). You won't need
to manually handle it's opening. And also will allow to have multiple
popups opened. That means you can have a popup inside a popup. And it
will also lead to the easier creation of the popups. (should we create a
tracking issue to track changes because to me it seems like a huge
amount of changes to be done?)

---

- Improvements on <emilk#4607>
@emilk emilk added this to egui Nov 20, 2024
@emilk emilk moved this to Backlog in egui Nov 20, 2024
@emilk emilk moved this from Backlog to Ready in egui Dec 11, 2024
@emilk emilk removed this from the Next Major Release milestone Dec 11, 2024
@xangelix
Copy link
Contributor

xangelix commented Jan 6, 2025

Would greatly appreciate some condition flexibility for the helpers on Response, e.g. #3452

There are also a few additional spots in the code base that should probably be merged into whatever solution is chosen for this: e.g. https://github.com/emilk/egui/blob/master/crates/egui/src/widgets/color_picker.rs#L498

@lucasmerlin lucasmerlin moved this from Next up to In progress in egui Feb 10, 2025
@lucasmerlin lucasmerlin self-assigned this Feb 10, 2025
@lucasmerlin
Copy link
Collaborator

lucasmerlin commented Feb 11, 2025

My plan is:

  • add new Popup wich handles the differnet popup / tooltips sizing and positioning options and the popup open state
  • add new Tooltip struct which builds on top of Popup and handles all the tooltip logic
  • add some way to open multiple / nested popups
  • update the menu to use Popup and make it work in any popup / anywhere
  • use the new structs in the existing functions and mark some of them as deprecated
  • clean up all the different helper fns on Response (inconsistent naming, some take &self, some self)
    • should they talke &self or self?

I also tried writing a test for popups that uses memory.everything_is_visible in the demo app but it was so chaotic that I don't think such a test has a lot of value. I'll rather write some more specific tests for the new popup.

We should also pay attantion that the new popup/tooltip/menu plays together nicely with the modal. Maybe the open state handling could even be shared between popups and modals? Seems like it should be pretty similar

lucasmerlin added a commit that referenced this issue Feb 18, 2025
This introduces new `Tooltip` and `Popup` structs that unify and extend
the old popups and tooltips.

`Popup` handles the positioning and optionally stores state on whether
the popup is open (for click based popups like `ComboBox`, menus,
context menus).
`Tooltip` is based on `Popup` and handles state of whether the tooltip
should be shown (which turns out to be quite complex to handles all the
edge cases).

Both `Popup` and `Tooltip` can easily be constructed from a `Response`
and then customized via builder methods.

This also introduces `PositionAlign`, for aligning something outside of
a `Rect` (in contrast to `Align2` for aligning inside a `Rect`). But I
don't like the name, any suggestions? Inspired by [mui's tooltip
positioning](https://mui.com/material-ui/react-tooltip/#positioned-tooltips).

* Part of #4607 
* [x] I have followed the instructions in the PR template

TODOs:
- [x] Automatic tooltip positioning based on available space
- [x] Review / fix / remove all code TODOs 
- [x] ~Update the helper fns on `Response` to be consistent in naming
and parameters (Some use tooltip, some hover_ui, some take &self, some
take self)~ actually, I think the naming and parameter make sense on
second thought
- [x] Make sure all old code is marked deprecated

For discussion during review:
- the following check in `show_tooltip_for` still necessary?:
```rust
     let is_touch_screen = ctx.input(|i| i.any_touches());
     let allow_placing_below = !is_touch_screen; // There is a finger below. TODO: Needed?
```
@github-project-automation github-project-automation bot moved this from In progress to Done in egui Mar 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request refactor rerun Desired for Rerun.io
Projects
Status: Done
4 participants