eframe: Better restore Window position on Mac when on secondary monitor (#3239)

crates/eframe/src/native/epi_integration.rs

@@ -140,10 +140,11 @@ pub fn window_builder<E>(
 
     let inner_size_points = if let Some(mut window_settings) = window_settings {
         // Restore pos/size from previous session
-        window_settings.clamp_to_sane_values(largest_monitor_point_size(event_loop));
-        #[cfg(windows)]
-        window_settings.clamp_window_to_sane_position(event_loop);
-        window_builder = window_settings.initialize_window(window_builder);
+
+        window_settings.clamp_size_to_sane_values(largest_monitor_point_size(event_loop));
+        window_settings.clamp_position_to_monitors(event_loop);
+
+        window_builder = window_settings.initialize_window_builder(window_builder);
         window_settings.inner_size_points()
     } else {
         if let Some(pos) = *initial_window_pos {
@@ -173,12 +174,14 @@ pub fn window_builder<E>(
             }
         }
     }
+
     window_builder
 }
 
 pub fn apply_native_options_to_window(
     window: &winit::window::Window,
     native_options: &crate::NativeOptions,
+    window_settings: Option<WindowSettings>,
 ) {
     use winit::window::WindowLevel;
     window.set_window_level(if native_options.always_on_top {
@@ -186,6 +189,10 @@ pub fn apply_native_options_to_window(
     } else {
         WindowLevel::Normal
     });
+
+    if let Some(window_settings) = window_settings {
+        window_settings.initialize_window(window);
+    }
 }
 
 fn largest_monitor_point_size<E>(event_loop: &EventLoopWindowTarget<E>) -> egui::Vec2 {

crates/eframe/src/native/run.rs

@@ -675,7 +675,11 @@ mod glow_integration {
             glutin_window_context.on_resume(event_loop)?;
 
             if let Some(window) = &glutin_window_context.window {
-                epi_integration::apply_native_options_to_window(window, native_options);
+                epi_integration::apply_native_options_to_window(
+                    window,
+                    native_options,
+                    window_settings,
+                );
             }
 
             let gl = unsafe {
@@ -1128,7 +1132,11 @@ mod wgpu_integration {
             let window_builder =
                 epi_integration::window_builder(event_loop, title, native_options, window_settings);
             let window = window_builder.build(event_loop)?;
-            epi_integration::apply_native_options_to_window(&window, native_options);
+            epi_integration::apply_native_options_to_window(
+                &window,
+                native_options,
+                window_settings,
+            );
             Ok(window)
         }
 

crates/egui-winit/src/window_settings.rs

@@ -1,11 +1,13 @@
 /// Can be used to store native window settings (position and size).
-#[derive(Clone, Copy, Debug)]
+#[derive(Clone, Copy, Debug, Default)]
 #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
+#[cfg_attr(feature = "serde", serde(default))]
 pub struct WindowSettings {
-    /// Position of window in physical pixels. This is either
-    /// the inner or outer position depending on the platform.
-    /// See [`winit::window::WindowBuilder::with_position`] for details.
-    position: Option<egui::Pos2>,
+    /// Position of window content in physical pixels.
+    inner_position_pixels: Option<egui::Pos2>,
+
+    /// Position of window frame/titlebar in physical pixels.
+    outer_position_pixels: Option<egui::Pos2>,
 
     fullscreen: bool,
 
@@ -16,22 +18,20 @@ pub struct WindowSettings {
 impl WindowSettings {
     pub fn from_display(window: &winit::window::Window) -> Self {
         let inner_size_points = window.inner_size().to_logical::<f32>(window.scale_factor());
-        let position = if cfg!(macos) {
-            // MacOS uses inner position when positioning windows.
-            window
-                .inner_position()
-                .ok()
-                .map(|p| egui::pos2(p.x as f32, p.y as f32))
-        } else {
-            // Other platforms use the outer position.
-            window
-                .outer_position()
-                .ok()
-                .map(|p| egui::pos2(p.x as f32, p.y as f32))
-        };
+
+        let inner_position_pixels = window
+            .inner_position()
+            .ok()
+            .map(|p| egui::pos2(p.x as f32, p.y as f32));
+
+        let outer_position_pixels = window
+            .outer_position()
+            .ok()
+            .map(|p| egui::pos2(p.x as f32, p.y as f32));
 
         Self {
-            position,
+            inner_position_pixels,
+            outer_position_pixels,
 
             fullscreen: window.fullscreen().is_some(),
 
@@ -46,19 +46,21 @@ impl WindowSettings {
         self.inner_size_points
     }
 
-    pub fn initialize_window(
+    pub fn initialize_window_builder(
         &self,
         mut window: winit::window::WindowBuilder,
     ) -> winit::window::WindowBuilder {
-        // If the app last ran on two monitors and only one is now connected, then
-        // the given position is invalid.
-        // If this happens on Mac, the window is clamped into valid area.
-        // If this happens on Windows, the clamping behavior is managed by the function
-        // clamp_window_to_sane_position.
-        if let Some(pos) = self.position {
+        // `WindowBuilder::with_position` expects inner position in Macos, and outer position elsewhere
+        // See [`winit::window::WindowBuilder::with_position`] for details.
+        let pos_px = if cfg!(target_os = "macos") {
+            self.inner_position_pixels
+        } else {
+            self.outer_position_pixels
+        };
+        if let Some(pos_px) = pos_px {
             window = window.with_position(winit::dpi::PhysicalPosition {
-                x: pos.x as f64,
-                y: pos.y as f64,
+                x: pos_px.x as f64,
+                y: pos_px.y as f64,
             });
         }
 
@@ -77,68 +79,103 @@ impl WindowSettings {
         }
     }
 
-    pub fn clamp_to_sane_values(&mut self, max_size: egui::Vec2) {
+    pub fn initialize_window(&self, window: &winit::window::Window) {
+        if cfg!(target_os = "macos") {
+            // Mac sometimes has problems restoring the window to secondary monitors
+            // using only `WindowBuilder::with_position`, so we need this extra step:
+            if let Some(pos) = self.outer_position_pixels {
+                window.set_outer_position(winit::dpi::PhysicalPosition { x: pos.x, y: pos.y });
+            }
+        }
+    }
+
+    pub fn clamp_size_to_sane_values(&mut self, largest_monitor_size_points: egui::Vec2) {
         use egui::NumExt as _;
 
         if let Some(size) = &mut self.inner_size_points {
-            // Prevent ridiculously small windows
+            // Prevent ridiculously small windows:
             let min_size = egui::Vec2::splat(64.0);
             *size = size.at_least(min_size);
-            *size = size.at_most(max_size);
+
+            // Make sure we don't try to create a window larger than the largest monitor
+            // because on Linux that can lead to a crash.
+            *size = size.at_most(largest_monitor_size_points);
         }
     }
 
-    pub fn clamp_window_to_sane_position<E>(
+    pub fn clamp_position_to_monitors<E>(
         &mut self,
         event_loop: &winit::event_loop::EventLoopWindowTarget<E>,
     ) {
-        if let (Some(position), Some(inner_size_points)) =
-            (&mut self.position, &self.inner_size_points)
-        {
-            let monitors = event_loop.available_monitors();
-            // default to primary monitor, in case the correct monitor was disconnected.
-            let mut active_monitor = if let Some(active_monitor) = event_loop
-                .primary_monitor()
-                .or_else(|| event_loop.available_monitors().next())
-            {
-                active_monitor
-            } else {
-                return; // no monitors 🤷
-            };
-            for monitor in monitors {
-                let monitor_x_range = (monitor.position().x - inner_size_points.x as i32)
-                    ..(monitor.position().x + monitor.size().width as i32);
-                let monitor_y_range = (monitor.position().y - inner_size_points.y as i32)
-                    ..(monitor.position().y + monitor.size().height as i32);
-
-                if monitor_x_range.contains(&(position.x as i32))
-                    && monitor_y_range.contains(&(position.y as i32))
-                {
-                    active_monitor = monitor;
-                }
-            }
+        // If the app last ran on two monitors and only one is now connected, then
+        // the given position is invalid.
+        // If this happens on Mac, the window is clamped into valid area.
+        // If this happens on Windows, the window becomes invisible to the user 🤦‍♂️
+        // So on Windows we clamp the position to the monitor it is on.
+        if !cfg!(target_os = "windows") {
+            return;
+        }
 
-            let mut inner_size_pixels = *inner_size_points * (active_monitor.scale_factor() as f32);
-            // Add size of title bar. This is 32 px by default in Win 10/11.
-            if cfg!(target_os = "windows") {
-                inner_size_pixels +=
-                    egui::Vec2::new(0.0, 32.0 * active_monitor.scale_factor() as f32);
-            }
-            let monitor_position = egui::Pos2::new(
-                active_monitor.position().x as f32,
-                active_monitor.position().y as f32,
-            );
-            let monitor_size = egui::Vec2::new(
-                active_monitor.size().width as f32,
-                active_monitor.size().height as f32,
-            );
-
-            // Window size cannot be negative or the subsequent `clamp` will panic.
-            let window_size = (monitor_size - inner_size_pixels).max(egui::Vec2::ZERO);
-            // To get the maximum position, we get the rightmost corner of the display, then
-            // subtract the size of the window to get the bottom right most value window.position
-            // can have.
-            *position = position.clamp(monitor_position, monitor_position + window_size);
+        let Some(inner_size_points) = self.inner_size_points else { return; };
+
+        if let Some(pos_px) = &mut self.inner_position_pixels {
+            clamp_pos_to_monitors(event_loop, inner_size_points, pos_px);
+        }
+        if let Some(pos_px) = &mut self.outer_position_pixels {
+            clamp_pos_to_monitors(event_loop, inner_size_points, pos_px);
+        }
+    }
+}
+
+fn clamp_pos_to_monitors<E>(
+    event_loop: &winit::event_loop::EventLoopWindowTarget<E>,
+    window_size_pts: egui::Vec2,
+    position_px: &mut egui::Pos2,
+) {
+    let monitors = event_loop.available_monitors();
+
+    // default to primary monitor, in case the correct monitor was disconnected.
+    let mut active_monitor = if let Some(active_monitor) = event_loop
+        .primary_monitor()
+        .or_else(|| event_loop.available_monitors().next())
+    {
+        active_monitor
+    } else {
+        return; // no monitors 🤷
+    };
+
+    for monitor in monitors {
+        let window_size_px = window_size_pts * (monitor.scale_factor() as f32);
+        let monitor_x_range = (monitor.position().x - window_size_px.x as i32)
+            ..(monitor.position().x + monitor.size().width as i32);
+        let monitor_y_range = (monitor.position().y - window_size_px.y as i32)
+            ..(monitor.position().y + monitor.size().height as i32);
+
+        if monitor_x_range.contains(&(position_px.x as i32))
+            && monitor_y_range.contains(&(position_px.y as i32))
+        {
+            active_monitor = monitor;
         }
     }
+
+    let mut window_size_px = window_size_pts * (active_monitor.scale_factor() as f32);
+    // Add size of title bar. This is 32 px by default in Win 10/11.
+    if cfg!(target_os = "windows") {
+        window_size_px += egui::Vec2::new(0.0, 32.0 * active_monitor.scale_factor() as f32);
+    }
+    let monitor_position = egui::Pos2::new(
+        active_monitor.position().x as f32,
+        active_monitor.position().y as f32,
+    );
+    let monitor_size_px = egui::Vec2::new(
+        active_monitor.size().width as f32,
+        active_monitor.size().height as f32,
+    );
+
+    // Window size cannot be negative or the subsequent `clamp` will panic.
+    let window_size = (monitor_size_px - window_size_px).max(egui::Vec2::ZERO);
+    // To get the maximum position, we get the rightmost corner of the display, then
+    // subtract the size of the window to get the bottom right most value window.position
+    // can have.
+    *position_px = position_px.clamp(monitor_position, monitor_position + window_size);
 }