From 77244cd4c5ebb924beaec8f89604a2881106b27c Mon Sep 17 00:00:00 2001
From: Juan Campa <juancampa@gmail.com>
Date: Thu, 20 Mar 2025 05:51:42 -0400
Subject: [PATCH] =?UTF-8?q?=E2=9A=A0=EF=B8=8F=20Close=20popup=20if=20`Memo?=
 =?UTF-8?q?ry::keep=5Fpopup=5Fopen`=20isn't=20called=20(#5814)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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
---
 crates/egui/src/containers/popup.rs |  4 +-
 crates/egui/src/memory/mod.rs       | 58 ++++++++++++++++++++++++++---
 2 files changed, 56 insertions(+), 6 deletions(-)

diff --git a/crates/egui/src/containers/popup.rs b/crates/egui/src/containers/popup.rs
index 7c72d2dd..7159a052 100644
--- a/crates/egui/src/containers/popup.rs
+++ b/crates/egui/src/containers/popup.rs
@@ -530,7 +530,9 @@ impl<'a> Popup<'a> {
                 Some(SetOpenCommand::Toggle) => {
                     mem.toggle_popup(id);
                 }
-                None => {}
+                None => {
+                    mem.keep_popup_open(id);
+                }
             });
         }
 
diff --git a/crates/egui/src/memory/mod.rs b/crates/egui/src/memory/mod.rs
index c68ba4da..0f588ca5 100644
--- a/crates/egui/src/memory/mod.rs
+++ b/crates/egui/src/memory/mod.rs
@@ -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.