Fix closing of viewports (#3591)

This ensures the closed viewport gets a close-event, and that it and the
parent viewport gets repainting, allowing the event to be registered.

crates/eframe/src/native/epi_integration.rs

@@ -194,7 +194,7 @@ impl EpiIntegration {
 
     #[cfg(feature = "accesskit")]
     pub fn init_accesskit<E: From<egui_winit::accesskit_winit::ActionRequestEvent> + Send>(
-        &mut self,
+        &self,
         egui_winit: &mut egui_winit::State,
         window: &winit::window::Window,
         event_loop_proxy: winit::event_loop::EventLoopProxy<E>,
@@ -231,9 +231,10 @@ impl EpiIntegration {
 
         match event {
             WindowEvent::CloseRequested => {
-                log::debug!("Received WindowEvent::CloseRequested");
-                self.close = app.on_close_event() && viewport_id == ViewportId::ROOT;
-                log::debug!("App::on_close_event returned {}", self.close);
+                if viewport_id == ViewportId::ROOT {
+                    self.close = app.on_close_event();
+                    log::debug!("App::on_close_event returned {}", self.close);
+                }
             }
             WindowEvent::Destroyed => {
                 log::debug!("Received WindowEvent::Destroyed");
@@ -255,7 +256,7 @@ impl EpiIntegration {
             _ => {}
         }
 
-        egui_winit.on_window_event(&self.egui_ctx, event, viewport_id)
+        egui_winit.on_window_event(&self.egui_ctx, event)
     }
 
     pub fn pre_update(&mut self) {
@@ -281,7 +282,7 @@ impl EpiIntegration {
                 viewport_ui_cb(egui_ctx);
             } else {
                 // Root viewport
-                if egui_ctx.input(|i| i.viewport().close_requested) {
+                if egui_ctx.input(|i| i.viewport().close_requested()) {
                     self.close = app.on_close_event();
                     log::debug!("App::on_close_event returned {}", self.close);
                 }

crates/eframe/src/native/glow_integration.rs

@@ -208,7 +208,7 @@ impl GlowWinitApp {
         let system_theme =
             winit_integration::system_theme(&glutin.window(ViewportId::ROOT), &self.native_options);
 
-        let mut integration = EpiIntegration::new(
+        let integration = EpiIntegration::new(
             &glutin.window(ViewportId::ROOT),
             system_theme,
             &self.app_name,
@@ -656,12 +656,8 @@ impl GlowWinitRunning {
     ) -> EventResult {
         crate::profile_function!(egui_winit::short_window_event_description(event));
 
-        let viewport_id = self
-            .glutin
-            .borrow()
-            .viewport_from_window
-            .get(&window_id)
-            .copied();
+        let mut glutin = self.glutin.borrow_mut();
+        let viewport_id = glutin.viewport_from_window.get(&window_id).copied();
 
         // On Windows, if a window is resized by the user, it should repaint synchronously, inside the
         // event handler.
@@ -680,8 +676,7 @@ impl GlowWinitRunning {
 
         match event {
             winit::event::WindowEvent::Focused(new_focused) => {
-                self.glutin.borrow_mut().focused_viewport =
-                    new_focused.then(|| viewport_id).flatten();
+                glutin.focused_viewport = new_focused.then(|| viewport_id).flatten();
             }
 
             winit::event::WindowEvent::Resized(physical_size) => {
@@ -691,7 +686,7 @@ impl GlowWinitRunning {
                 if 0 < physical_size.width && 0 < physical_size.height {
                     if let Some(viewport_id) = viewport_id {
                         repaint_asap = true;
-                        self.glutin.borrow_mut().resize(viewport_id, *physical_size);
+                        glutin.resize(viewport_id, *physical_size);
                     }
                 }
             }
@@ -699,45 +694,59 @@ impl GlowWinitRunning {
             winit::event::WindowEvent::ScaleFactorChanged { new_inner_size, .. } => {
                 if let Some(viewport_id) = viewport_id {
                     repaint_asap = true;
-                    self.glutin
-                        .borrow_mut()
-                        .resize(viewport_id, **new_inner_size);
+                    glutin.resize(viewport_id, **new_inner_size);
                 }
             }
 
             winit::event::WindowEvent::CloseRequested => {
-                let is_root = viewport_id == Some(ViewportId::ROOT);
-                if is_root && self.integration.should_close() {
-                    log::debug!("Received WindowEvent::CloseRequested");
+                if viewport_id == Some(ViewportId::ROOT) && self.integration.should_close() {
+                    log::debug!(
+                        "Received WindowEvent::CloseRequested for main viewport - shutting down."
+                    );
                     return EventResult::Exit;
                 }
+
+                log::debug!("Received WindowEvent::CloseRequested for viewport {viewport_id:?}");
+
+                if let Some(viewport_id) = viewport_id {
+                    if let Some(viewport) = glutin.viewports.get_mut(&viewport_id) {
+                        // Tell viewport it should close:
+                        viewport.info.events.push(egui::ViewportEvent::Close);
+
+                        // We may need to repaint both us and our parent to close the window,
+                        // and perhaps twice (once to notice the close-event, once again to enforce it).
+                        // `request_repaint_of` does a double-repaint though:
+                        self.integration.egui_ctx.request_repaint_of(viewport_id);
+                        self.integration
+                            .egui_ctx
+                            .request_repaint_of(viewport.ids.parent);
+                    }
+                }
             }
 
             _ => {}
         }
 
-        let event_response = 'res: {
-            if let Some(viewport_id) = viewport_id {
-                let mut glutin = self.glutin.borrow_mut();
-                if let Some(viewport) = glutin.viewports.get_mut(&viewport_id) {
-                    break 'res self.integration.on_window_event(
-                        self.app.as_mut(),
-                        event,
-                        viewport.egui_winit.as_mut().unwrap(),
-                        viewport.ids.this,
-                    );
-                }
-            }
+        if self.integration.should_close() {
+            return EventResult::Exit;
+        }
 
-            EventResponse {
-                consumed: false,
-                repaint: false,
-            }
+        let mut event_response = EventResponse {
+            consumed: false,
+            repaint: false,
         };
+        if let Some(viewport_id) = viewport_id {
+            if let Some(viewport) = glutin.viewports.get_mut(&viewport_id) {
+                event_response = self.integration.on_window_event(
+                    self.app.as_mut(),
+                    event,
+                    viewport.egui_winit.as_mut().unwrap(),
+                    viewport.ids.this,
+                );
+            }
+        }
 
-        if self.integration.should_close() {
-            EventResult::Exit
-        } else if event_response.repaint {
+        if event_response.repaint {
             if repaint_asap {
                 EventResult::RepaintNow(window_id)
             } else {

crates/eframe/src/native/run.rs

@@ -147,7 +147,6 @@ fn run_and_return(
                 }
             }
             EventResult::RepaintNext(window_id) => {
-                log::trace!("Repaint caused by {}", short_event_description(&event));
                 windows_next_repaint_times.insert(window_id, Instant::now());
             }
             EventResult::RepaintAt(window_id, repaint_time) => {

crates/eframe/src/native/wgpu_integration.rs

@@ -162,7 +162,7 @@ impl WgpuWinitApp {
         let wgpu_render_state = painter.render_state();
 
         let system_theme = winit_integration::system_theme(&window, &self.native_options);
-        let mut integration = EpiIntegration::new(
+        let integration = EpiIntegration::new(
             &window,
             system_theme,
             &self.app_name,
@@ -697,40 +697,57 @@ impl WgpuWinitRunning {
                 if let (Some(width), Some(height), Some(viewport_id)) = (
                     NonZeroU32::new(new_inner_size.width),
                     NonZeroU32::new(new_inner_size.height),
-                    shared.viewport_from_window.get(&window_id).copied(),
+                    viewport_id,
                 ) {
                     repaint_asap = true;
                     shared.painter.on_window_resized(viewport_id, width, height);
                 }
             }
 
-            winit::event::WindowEvent::CloseRequested if integration.should_close() => {
-                log::debug!("Received WindowEvent::CloseRequested");
-                return EventResult::Exit;
+            winit::event::WindowEvent::CloseRequested => {
+                if viewport_id == Some(ViewportId::ROOT) && integration.should_close() {
+                    log::debug!(
+                        "Received WindowEvent::CloseRequested for main viewport - shutting down."
+                    );
+                    return EventResult::Exit;
+                }
+
+                log::debug!("Received WindowEvent::CloseRequested for viewport {viewport_id:?}");
+
+                if let Some(viewport_id) = viewport_id {
+                    if let Some(viewport) = shared.viewports.get_mut(&viewport_id) {
+                        // Tell viewport it should close:
+                        viewport.info.events.push(egui::ViewportEvent::Close);
+
+                        // We may need to repaint both us and our parent to close the window,
+                        // and perhaps twice (once to notice the close-event, once again to enforce it).
+                        // `request_repaint_of` does a double-repaint though:
+                        integration.egui_ctx.request_repaint_of(viewport_id);
+                        integration.egui_ctx.request_repaint_of(viewport.ids.parent);
+                    }
+                }
             }
 
             _ => {}
         };
 
-        let event_response = viewport_id.and_then(|viewport_id| {
-            shared.viewports.get_mut(&viewport_id).and_then(|viewport| {
-                viewport.egui_winit.as_mut().map(|egui_winit| {
-                    integration.on_window_event(app.as_mut(), event, egui_winit, viewport_id)
+        let event_response = viewport_id
+            .and_then(|viewport_id| {
+                shared.viewports.get_mut(&viewport_id).and_then(|viewport| {
+                    viewport.egui_winit.as_mut().map(|egui_winit| {
+                        integration.on_window_event(app.as_mut(), event, egui_winit, viewport_id)
+                    })
                 })
             })
-        });
+            .unwrap_or_default();
 
         if integration.should_close() {
             EventResult::Exit
-        } else if let Some(event_response) = event_response {
-            if event_response.repaint {
-                if repaint_asap {
-                    EventResult::RepaintNow(window_id)
-                } else {
-                    EventResult::RepaintNext(window_id)
-                }
+        } else if event_response.repaint {
+            if repaint_asap {
+                EventResult::RepaintNow(window_id)
             } else {
-                EventResult::Wait
+                EventResult::RepaintNext(window_id)
             }
         } else {
             EventResult::Wait

crates/egui-winit/src/lib.rs

@@ -38,6 +38,7 @@ pub fn screen_size_in_pixels(window: &Window) -> egui::Vec2 {
 // ----------------------------------------------------------------------------
 
 #[must_use]
+#[derive(Clone, Copy, Debug, Default)]
 pub struct EventResponse {
     /// If true, egui consumed this event, i.e. wants exclusive use of this event
     /// (e.g. a mouse click on an egui window, or entering text into a text field).
@@ -237,7 +238,6 @@ impl State {
         &mut self,
         egui_ctx: &egui::Context,
         event: &winit::event::WindowEvent<'_>,
-        viewport_id: ViewportId,
     ) -> EventResponse {
         crate::profile_function!(short_window_event_description(event));
 
@@ -421,15 +421,11 @@ impl State {
             }
 
             // Things that may require repaint:
-            WindowEvent::CloseRequested => {
-                if let Some(viewport_info) = self.egui_input.viewports.get_mut(&viewport_id) {
-                    viewport_info.close_requested = true;
-                }
-                EventResponse {
-                    consumed: true,
-                    repaint: true,
-                }
-            }
+            WindowEvent::CloseRequested => EventResponse {
+                consumed: true,
+                repaint: true,
+            },
+
             WindowEvent::CursorEntered { .. }
             | WindowEvent::Destroyed
             | WindowEvent::Occluded(_)
@@ -1053,7 +1049,7 @@ pub fn process_viewport_commands(
     for command in commands {
         match command {
             ViewportCommand::Close => {
-                info.close_requested = true;
+                info.events.push(egui::ViewportEvent::Close);
             }
             ViewportCommand::StartDrag => {
                 // If `is_viewport_focused` is not checked on x11 the input will be permanently taken until the app is killed!
@@ -1143,7 +1139,7 @@ pub fn process_viewport_commands(
                 egui::viewport::WindowLevel::AlwaysOnTop => WindowLevel::AlwaysOnTop,
                 egui::viewport::WindowLevel::Normal => WindowLevel::Normal,
             }),
-            ViewportCommand::WindowIcon(icon) => {
+            ViewportCommand::Icon(icon) => {
                 window.set_window_icon(icon.map(|icon| {
                     winit::window::Icon::from_rgba(icon.rgba.clone(), icon.width, icon.height)
                         .expect("Invalid ICON data!")

crates/egui/src/context.rs

@@ -2563,8 +2563,13 @@ impl Context {
     ///
     /// This lets you affect another viewport, e.g. resizing its window.
     pub fn send_viewport_cmd_to(&self, id: ViewportId, command: ViewportCommand) {
-        self.write(|ctx| ctx.viewport_for(id).commands.push(command));
         self.request_repaint_of(id);
+
+        if command.requires_parent_repaint() {
+            self.request_repaint_of(self.parent_viewport_id());
+        }
+
+        self.write(|ctx| ctx.viewport_for(id).commands.push(command));
     }
 
     /// Show a deferred viewport, creating a new native window, if possible.

crates/egui/src/data/input.rs

@@ -163,6 +163,18 @@ impl RawInput {
     }
 }
 
+/// An input event from the backend into egui, about a specific [viewport](crate::viewport).
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
+pub enum ViewportEvent {
+    /// The user clicked the close-button on the window, or similar.
+    ///
+    /// It is up to the user to react to this by _not_ showing the viewport in the next frame in the parent viewport.
+    ///
+    /// This even will wake up both the child and parent viewport.
+    Close,
+}
+
 /// Information about the current viewport,
 /// given as input each frame.
 ///
@@ -176,9 +188,7 @@ pub struct ViewportInfo {
     /// Name of the viewport, if known.
     pub title: Option<String>,
 
-    /// The user requested the viewport should close,
-    /// e.g. by pressing the close button in the window decoration.
-    pub close_requested: bool,
+    pub events: Vec<ViewportEvent>,
 
     /// Number of physical pixels per ui point.
     pub pixels_per_point: f32,
@@ -212,15 +222,17 @@ pub struct ViewportInfo {
 }
 
 impl ViewportInfo {
-    pub fn take(&mut self) -> Self {
-        core::mem::take(self)
+    pub fn close_requested(&self) -> bool {
+        self.events
+            .iter()
+            .any(|&event| event == ViewportEvent::Close)
     }
 
     pub fn ui(&self, ui: &mut crate::Ui) {
         let Self {
             parent,
             title,
-            close_requested,
+            events,
             pixels_per_point,
             monitor_size,
             inner_rect,
@@ -240,8 +252,8 @@ impl ViewportInfo {
             ui.label(opt_as_str(title));
             ui.end_row();
 
-            ui.label("Close requested:");
-            ui.label(close_requested.to_string());
+            ui.label("Events:");
+            ui.label(format!("{events:?}"));
             ui.end_row();
 
             ui.label("Pixels per point:");

crates/egui/src/viewport.rs

@@ -369,7 +369,7 @@ impl ViewportBuilder {
     /// The default icon is a white `e` on a black background (for "egui" or "eframe").
     /// If you prefer the OS default, set this to `None`.
     #[inline]
-    pub fn with_window_icon(mut self, icon: impl Into<Arc<IconData>>) -> Self {
+    pub fn with_icon(mut self, icon: impl Into<Arc<IconData>>) -> Self {
         self.icon = Some(icon.into());
         self
     }
@@ -641,7 +641,7 @@ impl ViewportBuilder {
             };
 
             if is_new {
-                commands.push(ViewportCommand::WindowIcon(Some(new_icon.clone())));
+                commands.push(ViewportCommand::Icon(Some(new_icon.clone())));
                 self.icon = Some(new_icon.clone());
             }
         }
@@ -765,7 +765,9 @@ pub enum ResizeDirection {
     SouthWest,
 }
 
-/// You can send a [`ViewportCommand`] to the viewport with [`Context::send_viewport_cmd`].
+/// An output [viewport](crate::viewport)-command from egui to the backend, e.g. to change the window title or size.
+///
+///  You can send a [`ViewportCommand`] to the viewport with [`Context::send_viewport_cmd`].
 ///
 /// See [`crate::viewport`] for how to build new viewports (native windows).
 ///
@@ -842,7 +844,7 @@ pub enum ViewportCommand {
     WindowLevel(WindowLevel),
 
     /// The the window icon.
-    WindowIcon(Option<Arc<IconData>>),
+    Icon(Option<Arc<IconData>>),
 
     IMEPosition(Pos2),
     IMEAllowed(bool),
@@ -903,6 +905,11 @@ impl ViewportCommand {
             }
         })
     }
+
+    /// This command requires the parent viewport to repaint.
+    pub fn requires_parent_repaint(&self) -> bool {
+        self == &Self::Close
+    }
 }
 
 /// Describes a viewport, i.e. a native window.

crates/egui_demo_lib/src/demo/extra_viewport.rs

@@ -66,7 +66,7 @@ fn viewport_content(ui: &mut egui::Ui, ctx: &egui::Context, open: &mut bool) {
         }
     });
 
-    if ui.input(|i| i.viewport().close_requested) {
+    if ui.input(|i| i.viewport().close_requested()) {
         *open = false;
     }
 }

crates/egui_glow/src/winit.rs

@@ -53,8 +53,7 @@ impl EguiGlow {
     }
 
     pub fn on_window_event(&mut self, event: &winit::event::WindowEvent<'_>) -> EventResponse {
-        self.egui_winit
-            .on_window_event(&self.egui_ctx, event, ViewportId::ROOT)
+        self.egui_winit.on_window_event(&self.egui_ctx, event)
     }
 
     /// Call [`Self::paint`] later to paint.

examples/multiple_viewports/src/main.rs

@@ -65,10 +65,9 @@ impl eframe::App for MyApp {
                         ui.label("Hello from immediate viewport");
                     });
 
-                    if ctx.input(|i| i.viewport().close_requested) {
+                    if ctx.input(|i| i.viewport().close_requested()) {
                         // Tell parent viewport that we should not show next frame:
                         self.show_immediate_viewport = false;
-                        ctx.request_repaint(); // make sure there is a next frame
                     }
                 },
             );
@@ -76,12 +75,12 @@ impl eframe::App for MyApp {
 
         if self.show_deferred_viewport.load(Ordering::Relaxed) {
             let show_deferred_viewport = self.show_deferred_viewport.clone();
-            ctx.show_viewport_immediate(
+            ctx.show_viewport_deferred(
                 egui::ViewportId::from_hash_of("deferred_viewport"),
                 egui::ViewportBuilder::default()
                     .with_title("Deferred Viewport")
                     .with_inner_size([200.0, 100.0]),
-                |ctx, class| {
+                move |ctx, class| {
                     assert!(
                         class == egui::ViewportClass::Deferred,
                         "This egui backend doesn't support multiple viewports"
@@ -90,10 +89,9 @@ impl eframe::App for MyApp {
                     egui::CentralPanel::default().show(ctx, |ui| {
                         ui.label("Hello from deferred viewport");
                     });
-                    if ctx.input(|i| i.viewport().close_requested) {
+                    if ctx.input(|i| i.viewport().close_requested()) {
                         // Tell parent to close us.
                         show_deferred_viewport.store(false, Ordering::Relaxed);
-                        ctx.request_repaint(); // make sure there is a next frame
                     }
                 },
             );