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

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](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 https://github.com/emilk/egui/issues/3657
* [x] I have followed the instructions in the PR template
This commit is contained in:
Juan Campa 2025-03-20 05:51:42 -04:00 committed by GitHub
parent b0bbca4e69
commit 77244cd4c5
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 56 additions and 6 deletions

View File

@ -530,7 +530,9 @@ impl<'a> Popup<'a> {
Some(SetOpenCommand::Toggle) => {
mem.toggle_popup(id);
}
None => {}
None => {
mem.keep_popup_open(id);
}
});
}

View File

@ -94,7 +94,7 @@ pub struct Memory {
/// If position is [`None`], the popup position will be calculated based on some configuration
/// (e.g. relative to some other widget).
#[cfg_attr(feature = "persistence", serde(skip))]
popup: Option<(Id, Option<Pos2>)>,
popup: Option<OpenPopup>,
#[cfg_attr(feature = "persistence", serde(skip))]
everything_is_visible: bool,
@ -807,6 +807,15 @@ impl Memory {
self.caches.update();
self.areas_mut().end_pass();
self.focus_mut().end_pass(used_ids);
// Clean up abandoned popups.
if let Some(popup) = &mut self.popup {
if popup.open_this_frame {
popup.open_this_frame = false;
} else {
self.popup = None;
}
}
}
pub(crate) fn set_viewport_id(&mut self, viewport_id: ViewportId) {
@ -1068,13 +1077,37 @@ impl Memory {
}
}
/// State of an open popup.
#[derive(Clone, Copy, Debug)]
struct OpenPopup {
/// Id of the popup.
id: Id,
/// Optional position of the popup.
pos: Option<Pos2>,
/// Whether this popup was still open this frame. Otherwise it's considered abandoned and `Memory::popup` will be cleared.
open_this_frame: bool,
}
impl OpenPopup {
/// Create a new `OpenPopup`.
fn new(id: Id, pos: Option<Pos2>) -> Self {
Self {
id,
pos,
open_this_frame: true,
}
}
}
/// ## Popups
/// Popups are things like combo-boxes, color pickers, menus etc.
/// Only one can be open at a time.
impl Memory {
/// Is the given popup open?
pub fn is_popup_open(&self, popup_id: Id) -> bool {
self.popup.is_some_and(|(id, _)| id == popup_id) || self.everything_is_visible()
self.popup.is_some_and(|state| state.id == popup_id) || self.everything_is_visible()
}
/// Is any popup open?
@ -1083,19 +1116,34 @@ impl Memory {
}
/// Open the given popup and close all others.
///
/// Note that you must call `keep_popup_open` on subsequent frames as long as the popup is open.
pub fn open_popup(&mut self, popup_id: Id) {
self.popup = Some((popup_id, None));
self.popup = Some(OpenPopup::new(popup_id, None));
}
/// Popups must call this every frame while open.
///
/// 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) {
if let Some(state) = self.popup.as_mut() {
if state.id == popup_id {
state.open_this_frame = true;
}
}
}
/// Open the popup and remember its position.
pub fn open_popup_at(&mut self, popup_id: Id, pos: impl Into<Option<Pos2>>) {
self.popup = Some((popup_id, pos.into()));
self.popup = Some(OpenPopup::new(popup_id, pos.into()));
}
/// Get the position for this popup.
pub fn popup_position(&self, id: Id) -> Option<Pos2> {
self.popup
.and_then(|(popup_id, pos)| if popup_id == id { pos } else { None })
.and_then(|state| if state.id == id { state.pos } else { None })
}
/// Close any currently open popup.