Fix macOS new window stacking (#38683)

Petros Amoiridis created 2 months ago

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

Change summary

crates/gpui/src/window.rs | 99 ++++++++++++++++++++++++++++++++--------
1 file changed, 79 insertions(+), 20 deletions(-)

Detailed changes

crates/gpui/src/window.rs

@@ -918,26 +918,85 @@ pub(crate) struct ElementStateBox {
 }
 
 fn default_bounds(display_id: Option<DisplayId>, cx: &mut App) -> Bounds<Pixels> {
-    const DEFAULT_WINDOW_OFFSET: Point<Pixels> = 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<Pixels> = 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 {