From 114460c1de6a7d879b5a2e6c3dd9ea401f3a7b17 Mon Sep 17 00:00:00 2001 From: mkalte Date: Tue, 22 Apr 2025 11:38:17 +0200 Subject: [PATCH] Change popup memory to be per-viewport (#6753) Starting with 77244cd4c5ebb924beaec8f89604a2881106b27c the popup open-state is cleaned up per memory pass. This becomes problematic for implementations that share memory between viewports (i.e. all of them, as far as i understand it), because each viewport gets a context pass, and thus a memory pass, which cleans out popup open state. To illustrate my issue, i have modifed the multiple viewport example to include a popup menu for the labels: https://gist.github.com/mkalte666/4ecd6b658003df4c6d532ae2060c7595 (changes not included in this pr). Then, when i try to use the popups, i get this: https://github.com/user-attachments/assets/7d04b872-5396-4823-bf30-824122925528 Immediate viewports just break popup handling in general, while deferred viewports kinda work, or dont. In this example ill be honest, it kind of still did, sometimes. In my more complex app with multiple viewports (where i encountered this first) it also just broke - even when just showing root and one nother. Probably to do with the order wgpu vs glow draws the viewports? Im not sure. In any case: This commit adds `Memory::popup` (now `Memory::popups`) to the per-viewport state of memory, including viewport aware cleanup, and adding it to the list of things cleaned if a viewport goes away. This results in the expected behavior: https://github.com/user-attachments/assets/fd1f74b7-d3b2-4edc-8dc4-2ad3cfa2160e I will note that with this, changing focus does not cause a popup to be closed, which is consistent with current behavior on a single app. Hope this helps ~Malte * Closes * [x] I have followed the instructions in the PR template * [x] ~~I have run check.sh locally~~ CI on the fork, including checks, went through. --- crates/egui/src/memory/mod.rs | 47 ++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 20 deletions(-) diff --git a/crates/egui/src/memory/mod.rs b/crates/egui/src/memory/mod.rs index 0f588ca5..b8cd782c 100644 --- a/crates/egui/src/memory/mod.rs +++ b/crates/egui/src/memory/mod.rs @@ -87,15 +87,6 @@ pub struct Memory { #[cfg_attr(feature = "persistence", serde(skip))] pub(crate) viewport_id: ViewportId, - /// Which popup-window is open (if any)? - /// Could be a combo box, color picker, menu, etc. - /// Optionally stores the position of the popup (usually this would be the position where - /// the user clicked). - /// 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, - #[cfg_attr(feature = "persistence", serde(skip))] everything_is_visible: bool, @@ -116,6 +107,15 @@ pub struct Memory { #[cfg_attr(feature = "persistence", serde(skip))] pub(crate) focus: ViewportIdMap, + + /// Which popup-window is open on a viewport (if any)? + /// Could be a combo box, color picker, menu, etc. + /// Optionally stores the position of the popup (usually this would be the position where + /// the user clicked). + /// 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))] + popups: ViewportIdMap, } impl Default for Memory { @@ -130,7 +130,7 @@ impl Default for Memory { viewport_id: Default::default(), areas: Default::default(), to_global: Default::default(), - popup: Default::default(), + popups: Default::default(), everything_is_visible: Default::default(), add_fonts: Default::default(), }; @@ -790,6 +790,7 @@ impl Memory { // Cleanup self.interactions.retain(|id, _| viewports.contains(id)); self.areas.retain(|id, _| viewports.contains(id)); + self.popups.retain(|id, _| viewports.contains(id)); self.areas.entry(self.viewport_id).or_default(); @@ -809,11 +810,11 @@ impl Memory { self.focus_mut().end_pass(used_ids); // Clean up abandoned popups. - if let Some(popup) = &mut self.popup { + if let Some(popup) = self.popups.get_mut(&self.viewport_id) { if popup.open_this_frame { popup.open_this_frame = false; } else { - self.popup = None; + self.popups.remove(&self.viewport_id); } } } @@ -1107,19 +1108,23 @@ impl OpenPopup { impl Memory { /// Is the given popup open? pub fn is_popup_open(&self, popup_id: Id) -> bool { - self.popup.is_some_and(|state| state.id == popup_id) || self.everything_is_visible() + self.popups + .get(&self.viewport_id) + .is_some_and(|state| state.id == popup_id) + || self.everything_is_visible() } /// Is any popup open? pub fn any_popup_open(&self) -> bool { - self.popup.is_some() || self.everything_is_visible() + self.popups.contains_key(&self.viewport_id) || self.everything_is_visible() } /// 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(OpenPopup::new(popup_id, None)); + self.popups + .insert(self.viewport_id, OpenPopup::new(popup_id, None)); } /// Popups must call this every frame while open. @@ -1128,7 +1133,7 @@ impl Memory { /// 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 let Some(state) = self.popups.get_mut(&self.viewport_id) { if state.id == popup_id { state.open_this_frame = true; } @@ -1137,18 +1142,20 @@ impl Memory { /// Open the popup and remember its position. pub fn open_popup_at(&mut self, popup_id: Id, pos: impl Into>) { - self.popup = Some(OpenPopup::new(popup_id, pos.into())); + self.popups + .insert(self.viewport_id, OpenPopup::new(popup_id, pos.into())); } /// Get the position for this popup. pub fn popup_position(&self, id: Id) -> Option { - self.popup + self.popups + .get(&self.viewport_id) .and_then(|state| if state.id == id { state.pos } else { None }) } /// Close any currently open popup. pub fn close_all_popups(&mut self) { - self.popup = None; + self.popups.clear(); } /// Close the given popup, if it is open. @@ -1156,7 +1163,7 @@ impl Memory { /// See also [`Self::close_all_popups`] if you want to close any / all currently open popups. pub fn close_popup(&mut self, popup_id: Id) { if self.is_popup_open(popup_id) { - self.popup = None; + self.popups.remove(&self.viewport_id); } }