From 714481073d4d68b7bd2e452fbcf688100723e837 Mon Sep 17 00:00:00 2001 From: Petros Amoiridis Date: Thu, 6 Nov 2025 03:22:49 +0200 Subject: [PATCH] Fix macOS new window stacking (#38683) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #36206 Disclaimer: I did use AI for help to end up with this proposed solution. 😅 ## Observed behavior of native apps on macOS (like Safari) I first did a quick research on how Safari behaves on macOS, and here's what I have found: 1. Safari seems to position new windows with an offset based on the currently active window 2. It keeps opening new windows with an offset until the new window cannot fit the display bounds horizontally, vertically or both. 3. When it cannot fit horizontally, the new window opens at x=0 (y=active window's y) 4. When it cannot fit vertically, the new window opens at y=0 (x=active window's x) 5. When it cannot fit both horizontally and vertically, the new window opens at x=0 and y=0 (top left). 6. At any moment if I activate a different Safari window, the next new window is offset off of that 7. If I resize the active window and open a new window, the new window has the same size as the active window So, I implemented the changes based on those observations. I am not sure if touching `gpui/src/window.rs` is the way to go. I am open to feedback and direction here. I am also not sure if making my changes platform (macOS) specific, is the right thing to do. I reckoned that Linux and Windows have different default behaviors, and the original issue mentioned macOS. But, likewise, I am open to take a different approach. ## Tests I haven't included tests for such change, as it seems to me a bit difficult to properly test this, other than just doing a manual integration test. But if you would want them for such a change, happy to try including them. ## Alternative approach I also did some research on macOS native APIs that we could use instead of trying to make the calculations ourselves, and I found `NSWindow.cascadeTopLeftFromPoint` which seems to be doing exactly what we want, and more. It probably takes more things into consideration and thus it is more robust. We could go down that road, and add it to `gpui/src/platform/mac/window.rs` and then use it for new window creation. Again, if that's what you would do yourselves, let me know and I can either change the implementation here, or open a new pull request and let you decide which one would you would like to pursue. ## Video showing the behavior https://github.com/user-attachments/assets/f802a864-7504-47ee-8c6b-8d9b55474899 🙇‍♂️ Release Notes: - Improved macOS new window stacking --- crates/gpui/src/window.rs | 99 +++++++++++++++++++++++++++++++-------- 1 file changed, 79 insertions(+), 20 deletions(-) diff --git a/crates/gpui/src/window.rs b/crates/gpui/src/window.rs index f6d9be68ab0773ca14e99a2f42bde3cb88d031c2..ef7a720e7f6f06f61a885424139b010e106fb341 100644 --- a/crates/gpui/src/window.rs +++ b/crates/gpui/src/window.rs @@ -918,26 +918,85 @@ pub(crate) struct ElementStateBox { } fn default_bounds(display_id: Option, cx: &mut App) -> Bounds { - const DEFAULT_WINDOW_OFFSET: Point = point(px(0.), px(35.)); - - // TODO, BUG: if you open a window with the currently active window - // on the stack, this will erroneously select the 'unwrap_or_else' - // code path - cx.active_window() - .and_then(|w| w.update(cx, |_, window, _| window.bounds()).ok()) - .map(|mut bounds| { - bounds.origin += DEFAULT_WINDOW_OFFSET; - bounds - }) - .unwrap_or_else(|| { - let display = display_id - .map(|id| cx.find_display(id)) - .unwrap_or_else(|| cx.primary_display()); - - display - .map(|display| display.default_bounds()) - .unwrap_or_else(|| Bounds::new(point(px(0.), px(0.)), DEFAULT_WINDOW_SIZE)) - }) + #[cfg(target_os = "macos")] + { + const CASCADE_OFFSET: f32 = 25.0; + + let display = display_id + .map(|id| cx.find_display(id)) + .unwrap_or_else(|| cx.primary_display()); + + let display_bounds = display + .as_ref() + .map(|d| d.bounds()) + .unwrap_or_else(|| Bounds::new(point(px(0.), px(0.)), DEFAULT_WINDOW_SIZE)); + + // TODO, BUG: if you open a window with the currently active window + // on the stack, this will erroneously select the 'unwrap_or_else' + // code path + let (base_origin, base_size) = cx + .active_window() + .and_then(|w| { + w.update(cx, |_, window, _| { + let bounds = window.bounds(); + (bounds.origin, bounds.size) + }) + .ok() + }) + .unwrap_or_else(|| { + let default_bounds = display + .as_ref() + .map(|d| d.default_bounds()) + .unwrap_or_else(|| Bounds::new(point(px(0.), px(0.)), DEFAULT_WINDOW_SIZE)); + (default_bounds.origin, default_bounds.size) + }); + + let cascade_offset = point(px(CASCADE_OFFSET), px(CASCADE_OFFSET)); + let proposed_origin = base_origin + cascade_offset; + let proposed_bounds = Bounds::new(proposed_origin, base_size); + + let display_right = display_bounds.origin.x + display_bounds.size.width; + let display_bottom = display_bounds.origin.y + display_bounds.size.height; + let window_right = proposed_bounds.origin.x + proposed_bounds.size.width; + let window_bottom = proposed_bounds.origin.y + proposed_bounds.size.height; + + let fits_horizontally = window_right <= display_right; + let fits_vertically = window_bottom <= display_bottom; + + let final_origin = match (fits_horizontally, fits_vertically) { + (true, true) => proposed_origin, + (false, true) => point(display_bounds.origin.x, base_origin.y), + (true, false) => point(base_origin.x, display_bounds.origin.y), + (false, false) => display_bounds.origin, + }; + + Bounds::new(final_origin, base_size) + } + + #[cfg(not(target_os = "macos"))] + { + const DEFAULT_WINDOW_OFFSET: Point = point(px(0.), px(35.)); + + // TODO, BUG: if you open a window with the currently active window + // on the stack, this will erroneously select the 'unwrap_or_else' + // code path + cx.active_window() + .and_then(|w| w.update(cx, |_, window, _| window.bounds()).ok()) + .map(|mut bounds| { + bounds.origin += DEFAULT_WINDOW_OFFSET; + bounds + }) + .unwrap_or_else(|| { + let display = display_id + .map(|id| cx.find_display(id)) + .unwrap_or_else(|| cx.primary_display()); + + display + .as_ref() + .map(|display| display.default_bounds()) + .unwrap_or_else(|| Bounds::new(point(px(0.), px(0.)), DEFAULT_WINDOW_SIZE)) + }) + } } impl Window {