Skip to content

⚠️ Close popup if Memory::keep_popup_open isn't called #5814

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Mar 20, 2025
Merged

⚠️ Close popup if Memory::keep_popup_open isn't called #5814

merged 3 commits into from
Mar 20, 2025

Conversation

juancampa
Copy link
Contributor

@juancampa juancampa commented Mar 16, 2025 •
edited by lucasmerlin
Loading

Breaking changes:

  • When using the Memory::popup state, it's now required to call keep_popup_open each frame or the popup will close.
  • Usually handled by the Popup struct, but required for custom popups using the state in Memory directly

If a popup is abandoned Memory::popup would remain Some. This is problematic if, for example, you have logic that checks is_any_popup_open.

This PR adds a new requirement for popups keeping their open state in Memory::popup. They must call Memory::keep_popup_open as long as they are being rendered. The recent changes in #5716 make this easy to implement.

Supersedes #4697 which had an awkward implementation

These two videos show a case where a context menu was open when the underlying widget got removed.

Before (any_popup_open remains true)
Screenshot 2025-03-16 at 18 22 50

After
Screenshot 2025-03-16 at 18 21 14

@juancampa
Copy link
Contributor Author

juancampa commented Mar 16, 2025 •
edited
Loading

cc @lucasmerlin who is actively working on popups

@github-actions GitHub Actions
Copy link

Preview available at https://egui-pr-preview.github.io/pr/5814-juanclean-up-abandoned-popups
Note that it might take a couple seconds for the update to show up after the preview_build workflow has completed.

@juancampa juancampa marked this pull request as ready for review March 16, 2025 22:46
@juancampa
Copy link
Contributor Author

@lucasmerlin, on a slight tangent, I noticed a couple of potential performance improvements to the new popup system while working on this btw:

  • The Tooltip struct contains a Popup which makes it relatively bulky struct (248 bytes). Since tooltips are commonly defined but rarely visible, I wonder if it's worth deferring the creation of Popup until the tooltip is actually visible.
  • Popup::from_response preemptively calls response.clicked_elsewhere which is not super heavy but will run a bunch of Rect::contains on every click.

Fixing the first one would make the second one a non-issue.

To illustrate this point with our application, these green rects show all the Popups created in one frame to handle tooltips. None of them visible or really doing anything.

Screenshot 2025-03-16 at 16 26 37

@lucasmerlin lucasmerlin added bug Something is broken egui labels Mar 17, 2025
lucasmerlin
lucasmerlin approved these changes Mar 17, 2025
View reviewed changes
Copy link
Collaborator

@lucasmerlin lucasmerlin left a comment

Choose a reason for hiding this comment

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

Nice, I think this makes sense.

@lucasmerlin
Copy link
Collaborator

lucasmerlin commented Mar 17, 2025 •
edited
Loading

About the optimization, best thing I can imagine that wouldn't make the api less ergonomic would be storing the Popup in a Option<Box<Popup>>, and then only setting it to some only if the popup is actually shown.

@juancampa
Copy link
Contributor Author

Feedback addressed 👍. This could be a breaking change for anyone using open_popup directly btw. Something to call out in the changelog.

About the optimization, best thing I can imagine that wouldn't make the api less ergonomic would be storing the Popup in a Option<Box<Popup>>, and then only setting it to some only if the popup is actually shown.

Makes sense. I mentioned it since there's been quite a bit of work recently to reduce the size of structs, so yeah this should help 👍

With regards to widget_clicked_elsewhere, would it make sense to use ui.ctx().get_response(widget_id) to get the widget's response only when needed? Similarly to how Ui::response works.

juancampa added a commit to membrane-io/egui that referenced this pull request Mar 17, 2025
@juancampa
[Upstream PR](emilk#5814) Clean up abandoned popups
204ee03
juancampa added a commit to membrane-io/egui that referenced this pull request Mar 17, 2025
@juancampa
[Upstream PR](emilk#5814) Clean up abandoned popups
8c7d751
@juancampa juancampa mentioned this pull request Mar 17, 2025
Closed
@lucasmerlin lucasmerlin changed the title Clean up abandoned popups ⚠️ Close popup if Memory::keep_popup_open isn't called Mar 18, 2025
@lucasmerlin
Copy link
Collaborator

Feedback addressed 👍. This could be a breaking change for anyone using open_popup directly btw. Something to call out in the changelog.

I've updated the title and description so we'll remember to adress it in the Changelog 👍🏻

With regards to widget_clicked_elsewhere, would it make sense to use ui.ctx().get_response(widget_id) to get the widget's response only when needed? Similarly to how Ui::response works.

Great idea!

emilk
emilk approved these changes Mar 18, 2025
View reviewed changes
crates/egui/src/memory/mod.rs
/// This is needed because in some cases popups can go away without `close_popup` being
/// called. For example, when a context menu is open and the underlying widget stops
/// being rendered.
pub fn keep_popup_open(&mut self, popup_id: Id) {
Copy link
Owner

Choose a reason for hiding this comment

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

I feel like there is a more elegant solution to this problem that I'm not seeing 😆

We could replace is_popup_open with show_popup_if_open(popup_id, closure), but that's a bit annoying too (especially when it comes to lifetimes and locks), so… no.

I'm also not in love with this function name, but again fail to come up with something better.

so… LGTM :)

@lucasmerlin lucasmerlin merged commit 77244cd into emilk:master Mar 20, 2025
25 checks passed
@lucasmerlin lucasmerlin mentioned this pull request Mar 20, 2025
Closed
@lucasmerlin lucasmerlin mentioned this pull request May 8, 2025
Closed
@emilk emilk mentioned this pull request Jul 10, 2025
Merged
@Omustardo Omustardo mentioned this pull request Jul 26, 2025
Closed
darkwater pushed a commit to darkwater/egui that referenced this pull request Aug 24, 2025
@juancampa @darkwater
⚠️ Close popup if Memory::keep_popup_open isn't called (emilk#5814)
3b17663 
Breaking changes:
- When using the Memory::popup state, it's now required to call
keep_popup_open each frame or the popup will close.
- Usually handled by the `Popup` struct, but required for custom popups
using the state in `Memory` directly

-----

If a popup is abandoned `Memory::popup` would remain `Some`. This is
problematic if, for example, you have logic that checks
`is_any_popup_open`.

This PR adds a new requirement for popups keeping their open state in
`Memory::popup`. They must call `Memory::keep_popup_open` as long as
they are being rendered. The recent changes in emilk#5716 make this easy to
implement.

Supersedes emilk#4697 which had an awkward implementation

These two videos show a case where a context menu was open when the
underlying widget got removed.

Before (`any_popup_open` remains `true`)
![Screenshot 2025-03-16 at 18 22
50](https://github.com/user-attachments/assets/22db64dd-e6f2-4501-9bda-39f470b9210c)

After
![Screenshot 2025-03-16 at 18 21
14](https://github.com/user-attachments/assets/bd4631b1-a0ad-4047-a14d-cd4999710e07)



* Closes emilk#3657
* [x] I have followed the instructions in the PR template
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilk emilk emilk approved these changes

@lucasmerlin lucasmerlin lucasmerlin approved these changes

Assignees
No one assigned
Labels
bug Something is broken egui
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

any_popup_open() remains true even when there are no popups
3 participants
@juancampa @lucasmerlin @emilk