Fix AppKit screen coordinate conversion leading to wrong window bounds (#2856)

Fixes https://linear.app/zed-industries/issue/Z-1510/join-project-notification-takes-up-full-screen-on-a-second-monitor There were multiple mistakes in the positioning of Zed's notification windows, one of which lead to the notifications taking up the full screen on secondary displays 😱 . * Wrong sign for the vertical padding (moving the window *upward* instead of downward) * Using the screen's full frame instead of its "visible frame" (which accounts for app menu bar) * Wrong coordinate translation between our coordinates and AppKit's coordinates. Regardless of which display a given window appears on, the coordinate translation needs to use the height of the *main* display. Release Notes: - Fixed a bug where call notifications were accidentally full-screen on all displays except the main display.
2023-08-17 16:24:46 -07:00 · 2023-08-17 16:24:46 -07:00 · 802911d742
commit 802911d742
parent 6d3518cb50 cd2ef784ea
10 changed files with 130 additions and 119 deletions
--- a/crates/collab_ui/src/collab_ui.rs
+++ b/crates/collab_ui/src/collab_ui.rs
@ -9,8 +9,16 @@ mod sharing_status_indicator;

 use call::{ActiveCall, Room};
 pub use collab_titlebar_item::CollabTitlebarItem;
-use gpui::{actions, AppContext, Task};
-use std::sync::Arc;
+use gpui::{
+    actions,
+    geometry::{
+        rect::RectF,
+        vector::{vec2f, Vector2F},
+    },
+    platform::{Screen, WindowBounds, WindowKind, WindowOptions},
+    AppContext, Task,
+};
+use std::{rc::Rc, sync::Arc};
 use util::ResultExt;
 use workspace::AppState;

@ -88,3 +96,29 @@ pub fn toggle_deafen(_: &ToggleDeafen, cx: &mut AppContext) {
            .log_err();
    }
 }
+
+fn notification_window_options(
+    screen: Rc<dyn Screen>,
+    window_size: Vector2F,
+) -> WindowOptions<'static> {
+    const NOTIFICATION_PADDING: f32 = 16.;
+
+    let screen_bounds = screen.content_bounds();
+    WindowOptions {
+        bounds: WindowBounds::Fixed(RectF::new(
+            screen_bounds.upper_right()
+                + vec2f(
+                    -NOTIFICATION_PADDING - window_size.x(),
+                    NOTIFICATION_PADDING,
+                ),
+            window_size,
+        )),
+        titlebar: None,
+        center: false,
+        focus: false,
+        show: true,
+        kind: WindowKind::PopUp,
+        is_movable: false,
+        screen: Some(screen),
+    }
+}
--- a/crates/collab_ui/src/incoming_call_notification.rs
+++ b/crates/collab_ui/src/incoming_call_notification.rs
@ -1,14 +1,14 @@
-use std::sync::{Arc, Weak};
-
+use crate::notification_window_options;
 use call::{ActiveCall, IncomingCall};
 use client::proto;
 use futures::StreamExt;
 use gpui::{
    elements::*,
-    geometry::{rect::RectF, vector::vec2f},
-    platform::{CursorStyle, MouseButton, WindowBounds, WindowKind, WindowOptions},
+    geometry::vector::vec2f,
+    platform::{CursorStyle, MouseButton},
    AnyElement, AppContext, Entity, View, ViewContext, WindowHandle,
 };
+use std::sync::{Arc, Weak};
 use util::ResultExt;
 use workspace::AppState;

@ -23,31 +23,16 @@ pub fn init(app_state: &Arc<AppState>, cx: &mut AppContext) {
            }

            if let Some(incoming_call) = incoming_call {
-                const PADDING: f32 = 16.;
                let window_size = cx.read(|cx| {
                    let theme = &theme::current(cx).incoming_call_notification;
                    vec2f(theme.window_width, theme.window_height)
                });

                for screen in cx.platform().screens() {
-                    let screen_bounds = screen.bounds();
-                    let window = cx.add_window(
-                        WindowOptions {
-                            bounds: WindowBounds::Fixed(RectF::new(
-                                screen_bounds.upper_right()
-                                    - vec2f(PADDING + window_size.x(), PADDING),
-                                window_size,
-                            )),
-                            titlebar: None,
-                            center: false,
-                            focus: false,
-                            show: true,
-                            kind: WindowKind::PopUp,
-                            is_movable: false,
-                            screen: Some(screen),
-                        },
-                        |_| IncomingCallNotification::new(incoming_call.clone(), app_state.clone()),
-                    );
+                    let window = cx
+                        .add_window(notification_window_options(screen, window_size), |_| {
+                            IncomingCallNotification::new(incoming_call.clone(), app_state.clone())
+                        });

                    notification_windows.push(window);
                }
--- a/crates/collab_ui/src/project_shared_notification.rs
+++ b/crates/collab_ui/src/project_shared_notification.rs
@ -1,10 +1,11 @@
+use crate::notification_window_options;
 use call::{room, ActiveCall};
 use client::User;
 use collections::HashMap;
 use gpui::{
    elements::*,
-    geometry::{rect::RectF, vector::vec2f},
-    platform::{CursorStyle, MouseButton, WindowBounds, WindowKind, WindowOptions},
+    geometry::vector::vec2f,
+    platform::{CursorStyle, MouseButton},
    AppContext, Entity, View, ViewContext,
 };
 use std::sync::{Arc, Weak};
@ -20,35 +21,19 @@ pub fn init(app_state: &Arc<AppState>, cx: &mut AppContext) {
            project_id,
            worktree_root_names,
        } => {
-            const PADDING: f32 = 16.;
            let theme = &theme::current(cx).project_shared_notification;
            let window_size = vec2f(theme.window_width, theme.window_height);

            for screen in cx.platform().screens() {
-                let screen_bounds = screen.bounds();
-                let window = cx.add_window(
-                    WindowOptions {
-                        bounds: WindowBounds::Fixed(RectF::new(
-                            screen_bounds.upper_right() - vec2f(PADDING + window_size.x(), PADDING),
-                            window_size,
-                        )),
-                        titlebar: None,
-                        center: false,
-                        focus: false,
-                        show: true,
-                        kind: WindowKind::PopUp,
-                        is_movable: false,
-                        screen: Some(screen),
-                    },
-                    |_| {
+                let window =
+                    cx.add_window(notification_window_options(screen, window_size), |_| {
                        ProjectSharedNotification::new(
                            owner.clone(),
                            *project_id,
                            worktree_root_names.clone(),
                            app_state.clone(),
                        )
-                    },
-                );
+                    });
                notification_windows
                    .entry(*project_id)
                    .or_insert(Vec::new())
--- a/crates/gpui/src/platform.rs
+++ b/crates/gpui/src/platform.rs
@ -135,6 +135,7 @@ pub trait InputHandler {
 pub trait Screen: Debug {
    fn as_any(&self) -> &dyn Any;
    fn bounds(&self) -> RectF;
+    fn content_bounds(&self) -> RectF;
    fn display_uuid(&self) -> Option<Uuid>;
 }

--- a/crates/gpui/src/platform/mac/geometry.rs
+++ b/crates/gpui/src/platform/mac/geometry.rs
@ -3,10 +3,7 @@ use cocoa::{
    foundation::{NSPoint, NSRect},
 };
 use objc::{msg_send, sel, sel_impl};
-use pathfinder_geometry::{
-    rect::RectF,
-    vector::{vec2f, Vector2F},
-};
+use pathfinder_geometry::vector::{vec2f, Vector2F};

 ///! Macos screen have a y axis that goings up from the bottom of the screen and
 ///! an origin at the bottom left of the main display.
@ -15,6 +12,7 @@ pub trait Vector2FExt {
    /// Converts self to an NSPoint with y axis pointing up.
    fn to_screen_ns_point(&self, native_window: id, window_height: f64) -> NSPoint;
 }
+
 impl Vector2FExt for Vector2F {
    fn to_screen_ns_point(&self, native_window: id, window_height: f64) -> NSPoint {
        unsafe {
@ -25,16 +23,13 @@ impl Vector2FExt for Vector2F {
 }

 pub trait NSRectExt {
-    fn to_rectf(&self) -> RectF;
+    fn size_vec(&self) -> Vector2F;
    fn intersects(&self, other: Self) -> bool;
 }

 impl NSRectExt for NSRect {
-    fn to_rectf(&self) -> RectF {
-        RectF::new(
-            vec2f(self.origin.x as f32, self.origin.y as f32),
-            vec2f(self.size.width as f32, self.size.height as f32),
-        )
+    fn size_vec(&self) -> Vector2F {
+        vec2f(self.size.width as f32, self.size.height as f32)
    }

    fn intersects(&self, other: Self) -> bool {
--- a/crates/gpui/src/platform/mac/screen.rs
+++ b/crates/gpui/src/platform/mac/screen.rs
@ -1,21 +1,19 @@
-use std::{any::Any, ffi::c_void};
-
+use super::ns_string;
 use crate::platform;
 use cocoa::{
    appkit::NSScreen,
    base::{id, nil},
-    foundation::{NSArray, NSDictionary},
+    foundation::{NSArray, NSDictionary, NSPoint, NSRect, NSSize},
 };
 use core_foundation::{
    number::{kCFNumberIntType, CFNumberGetValue, CFNumberRef},
    uuid::{CFUUIDGetUUIDBytes, CFUUIDRef},
 };
 use core_graphics::display::CGDirectDisplayID;
-use pathfinder_geometry::rect::RectF;
+use pathfinder_geometry::{rect::RectF, vector::vec2f};
+use std::{any::Any, ffi::c_void};
 use uuid::Uuid;

-use super::{geometry::NSRectExt, ns_string};
-
 #[link(name = "ApplicationServices", kind = "framework")]
 extern "C" {
    pub fn CGDisplayCreateUUIDFromDisplayID(display: CGDirectDisplayID) -> CFUUIDRef;
@ -27,29 +25,58 @@ pub struct Screen {
 }

 impl Screen {
+    /// Get the screen with the given UUID.
    pub fn find_by_id(uuid: Uuid) -> Option<Self> {
+        Self::all().find(|screen| platform::Screen::display_uuid(screen) == Some(uuid))
+    }
+
+    /// Get the primary screen - the one with the menu bar, and whose bottom left
+    /// corner is at the origin of the AppKit coordinate system.
+    fn primary() -> Self {
+        Self::all().next().unwrap()
+    }
+
+    pub fn all() -> impl Iterator<Item = Self> {
        unsafe {
            let native_screens = NSScreen::screens(nil);
-            (0..NSArray::count(native_screens))
-                .into_iter()
-                .map(|ix| Screen {
-                    native_screen: native_screens.objectAtIndex(ix),
-                })
-                .find(|screen| platform::Screen::display_uuid(screen) == Some(uuid))
+            (0..NSArray::count(native_screens)).map(move |ix| Screen {
+                native_screen: native_screens.objectAtIndex(ix),
+            })
        }
    }

-    pub fn all() -> Vec<Self> {
-        let mut screens = Vec::new();
-        unsafe {
-            let native_screens = NSScreen::screens(nil);
-            for ix in 0..NSArray::count(native_screens) {
-                screens.push(Screen {
-                    native_screen: native_screens.objectAtIndex(ix),
-                });
-            }
-        }
-        screens
+    /// Convert the given rectangle in screen coordinates from GPUI's
+    /// coordinate system to the AppKit coordinate system.
+    ///
+    /// In GPUI's coordinates, the origin is at the top left of the primary screen, with
+    /// the Y axis pointing downward. In the AppKit coordindate system, the origin is at the
+    /// bottom left of the primary screen, with the Y axis pointing upward.
+    pub(crate) fn screen_rect_to_native(rect: RectF) -> NSRect {
+        let primary_screen_height = unsafe { Self::primary().native_screen.frame().size.height };
+        NSRect::new(
+            NSPoint::new(
+                rect.origin_x() as f64,
+                primary_screen_height - rect.origin_y() as f64 - rect.height() as f64,
+            ),
+            NSSize::new(rect.width() as f64, rect.height() as f64),
+        )
+    }
+
+    /// Convert the given rectangle in screen coordinates from the AppKit
+    /// coordinate system to GPUI's coordinate system.
+    ///
+    /// In GPUI's coordinates, the origin is at the top left of the primary screen, with
+    /// the Y axis pointing downward. In the AppKit coordindate system, the origin is at the
+    /// bottom left of the primary screen, with the Y axis pointing upward.
+    pub(crate) fn screen_rect_from_native(rect: NSRect) -> RectF {
+        let primary_screen_height = unsafe { Self::primary().native_screen.frame().size.height };
+        RectF::new(
+            vec2f(
+                rect.origin.x as f32,
+                (primary_screen_height - rect.origin.y - rect.size.height) as f32,
+            ),
+            vec2f(rect.size.width as f32, rect.size.height as f32),
+        )
    }
 }

@ -108,9 +135,10 @@ impl platform::Screen for Screen {
    }

    fn bounds(&self) -> RectF {
-        unsafe {
-            let frame = self.native_screen.frame();
-            frame.to_rectf()
-        }
+        unsafe { Self::screen_rect_from_native(self.native_screen.frame()) }
+    }
+
+    fn content_bounds(&self) -> RectF {
+        unsafe { Self::screen_rect_from_native(self.native_screen.visibleFrame()) }
    }
 }
--- a/crates/gpui/src/platform/mac/window.rs
+++ b/crates/gpui/src/platform/mac/window.rs
@ -368,32 +368,20 @@ impl WindowState {
                return WindowBounds::Fullscreen;
            }

-            let window_frame = self.frame();
-            let screen_frame = self.native_window.screen().visibleFrame().to_rectf();
-            if window_frame.size() == screen_frame.size() {
+            let frame = self.frame();
+            let screen_size = self.native_window.screen().visibleFrame().size_vec();
+            if frame.size() == screen_size {
                WindowBounds::Maximized
            } else {
-                WindowBounds::Fixed(window_frame)
+                WindowBounds::Fixed(frame)
            }
        }
    }

-    // Returns the window bounds in window coordinates
    fn frame(&self) -> RectF {
        unsafe {
-            let screen_frame = self.native_window.screen().visibleFrame();
-            let window_frame = NSWindow::frame(self.native_window);
-            RectF::new(
-                vec2f(
-                    window_frame.origin.x as f32,
-                    (screen_frame.size.height - window_frame.origin.y - window_frame.size.height)
-                        as f32,
-                ),
-                vec2f(
-                    window_frame.size.width as f32,
-                    window_frame.size.height as f32,
-                ),
-            )
+            let frame = NSWindow::frame(self.native_window);
+            Screen::screen_rect_from_native(frame)
        }
    }

@ -480,21 +468,12 @@ impl MacWindow {
                    native_window.setFrame_display_(screen.visibleFrame(), YES);
                }
                WindowBounds::Fixed(rect) => {
-                    let screen_frame = screen.visibleFrame();
-                    let ns_rect = NSRect::new(
-                        NSPoint::new(
-                            rect.origin_x() as f64,
-                            screen_frame.size.height
-                                - rect.origin_y() as f64
-                                - rect.height() as f64,
-                        ),
-                        NSSize::new(rect.width() as f64, rect.height() as f64),
-                    );
-
-                    if ns_rect.intersects(screen_frame) {
-                        native_window.setFrame_display_(ns_rect, YES);
+                    let bounds = Screen::screen_rect_to_native(rect);
+                    let screen_bounds = screen.visibleFrame();
+                    if bounds.intersects(screen_bounds) {
+                        native_window.setFrame_display_(bounds, YES);
                    } else {
-                        native_window.setFrame_display_(screen_frame, YES);
+                        native_window.setFrame_display_(screen_bounds, YES);
                    }
                }
            }
--- a/crates/gpui/src/platform/test.rs
+++ b/crates/gpui/src/platform/test.rs
@ -250,6 +250,10 @@ impl super::Screen for Screen {
        RectF::new(Vector2F::zero(), Vector2F::new(1920., 1080.))
    }

+    fn content_bounds(&self) -> RectF {
+        RectF::new(Vector2F::zero(), Vector2F::new(1920., 1080.))
+    }
+
    fn display_uuid(&self) -> Option<uuid::Uuid> {
        Some(uuid::Uuid::new_v4())
    }
--- a/crates/terminal/Cargo.toml
+++ b/crates/terminal/Cargo.toml
@ -16,7 +16,7 @@ db = { path = "../db" }
 theme = { path = "../theme" }
 util = { path = "../util" }

-alacritty_terminal = { git = "https://github.com/zed-industries/alacritty", rev = "f6d001ba8080ebfab6822106a436c64b677a44d5" }
+alacritty_terminal = { git = "https://github.com/zed-industries/alacritty", rev = "33306142195b354ef3485ca2b1d8a85dfc6605ca" }
 procinfo = { git = "https://github.com/zed-industries/wezterm", rev = "5cd757e5f2eb039ed0c6bb6512223e69d5efc64d", default-features = false }
 smallvec.workspace = true
 smol.workspace = true