From d6c7dc2e82a65ec668cd684eb191313e77671df9 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Mon, 17 Mar 2025 09:18:14 +0100 Subject: [PATCH] macOS: Store UUID in MonitorHandle instead of CGDirectDisplayID The monitor UUID is what actually represents the monitor, CGDirectDisplayID is closer in correspondence to a specific framebuffer. --- src/changelog/unreleased.md | 3 +- src/platform_impl/apple/appkit/ffi.rs | 2 + src/platform_impl/apple/appkit/monitor.rs | 150 ++++++++++++------ .../apple/appkit/window_delegate.rs | 3 +- 4 files changed, 105 insertions(+), 53 deletions(-) diff --git a/src/changelog/unreleased.md b/src/changelog/unreleased.md index 63d9856d0b..7fb1724712 100644 --- a/src/changelog/unreleased.md +++ b/src/changelog/unreleased.md @@ -248,10 +248,11 @@ changelog entry. - On macOS, fixed the scancode conversion for `IntlBackslash`. - On macOS, fixed redundant `SurfaceResized` event at window creation. - On Windows, fixed ~500 ms pause when clicking the title bar during continuous redraw. -- On macos, `WindowExtMacOS::set_simple_fullscreen` now honors `WindowExtMacOS::set_borderless_game` +- On macOS, `WindowExtMacOS::set_simple_fullscreen` now honors `WindowExtMacOS::set_borderless_game` - On X11 and Wayland, fixed pump_events with `Some(Duration::Zero)` blocking with `Wait` polling mode - On macOS, fixed `run_app_on_demand` returning without closing open windows. - On Wayland, fixed a crash when consequently calling `set_cursor_grab` without pointer focus. - On Wayland, ensure that external event loop is woken-up when using pump_events and integrating via `FD`. - On Wayland, apply fractional scaling to custom cursors. - On macOS, fixed `VideoMode::refresh_rate_millihertz` for fractional refresh rates. +- On macOS, store monitor handle to avoid panics after going in/out of sleep. diff --git a/src/platform_impl/apple/appkit/ffi.rs b/src/platform_impl/apple/appkit/ffi.rs index b35fc84cc9..a42be14af5 100644 --- a/src/platform_impl/apple/appkit/ffi.rs +++ b/src/platform_impl/apple/appkit/ffi.rs @@ -24,6 +24,8 @@ pub const kIO30BitDirectPixels: &str = "--RRRRRRRRRRGGGGGGGGGGBBBBBBBBBB"; #[link(name = "ApplicationServices", kind = "framework")] extern "C" { pub fn CGDisplayCreateUUIDFromDisplayID(display: CGDirectDisplayID) -> *mut CFUUID; + + pub fn CGDisplayGetDisplayIDFromUUID(uuid: &CFUUID) -> CGDirectDisplayID; } #[link(name = "CoreGraphics", kind = "framework")] diff --git a/src/platform_impl/apple/appkit/monitor.rs b/src/platform_impl/apple/appkit/monitor.rs index 830771feca..4d00576105 100644 --- a/src/platform_impl/apple/appkit/monitor.rs +++ b/src/platform_impl/apple/appkit/monitor.rs @@ -9,13 +9,14 @@ use dispatch2::run_on_main; use objc2::rc::Retained; use objc2::MainThreadMarker; use objc2_app_kit::NSScreen; -use objc2_core_foundation::{CFArray, CFRetained}; +use objc2_core_foundation::{CFArray, CFRetained, CFUUID}; use objc2_core_graphics::{ CGDirectDisplayID, CGDisplayBounds, CGDisplayCopyAllDisplayModes, CGDisplayCopyDisplayMode, CGDisplayMode, CGDisplayModelNumber, CGGetActiveDisplayList, CGMainDisplayID, }; use objc2_core_video::{kCVReturnSuccess, CVDisplayLink, CVTimeFlags}; use objc2_foundation::{ns_string, NSNumber, NSPoint, NSRect}; +use tracing::warn; use super::ffi; use super::util::cgerr; @@ -92,35 +93,55 @@ impl VideoModeHandle { } } -#[derive(Clone)] -pub struct MonitorHandle(CGDirectDisplayID); +/// `CGDirectDisplayID` is documented as: +/// > a framebuffer, a color correction (gamma) table, and possibly an attached monitor. +/// +/// That is, it doesn't actually represent the monitor itself. Instead, we use the UUID of the +/// monitor, as retrieved from `CGDisplayCreateUUIDFromDisplayID` (this makes the monitor ID stable, +/// even across reboots and video mode changes). +/// +/// NOTE: I'd be perfectly valid to store `[u8; 16]` in here instead, we only store `CFUUID` to +/// avoid having to re-create it when we want to fetch the display ID. +#[derive(Clone, PartialEq, Eq, Hash)] +pub struct MonitorHandle(CFRetained); impl MonitorHandle { /// Internal comparisons of [`MonitorHandle`]s are done first requesting a UUID for the handle. fn uuid(&self) -> u128 { + u128::from_ne_bytes(self.0.uuid_bytes().into()) + } + + fn display_id(&self) -> CGDirectDisplayID { + unsafe { ffi::CGDisplayGetDisplayIDFromUUID(&self.0) } + } + + #[track_caller] + pub(crate) fn new(display_id: CGDirectDisplayID) -> Option { + // kCGNullDirectDisplay + if display_id == 0 { + // `CGDisplayCreateUUIDFromDisplayID` checks kCGNullDirectDisplay internally. + warn!("constructing monitor from invalid display ID 0; falling back to main monitor"); + } // SAFETY: Valid to call. - let ptr = unsafe { ffi::CGDisplayCreateUUIDFromDisplayID(self.0) }; + let ptr = unsafe { ffi::CGDisplayCreateUUIDFromDisplayID(display_id) }; + let ptr = NonNull::new(ptr)?; // SAFETY: `CGDisplayCreateUUIDFromDisplayID` is a "create" function, so the pointer has // +1 retain count. - let cf_uuid = unsafe { CFRetained::from_raw(NonNull::new(ptr).unwrap()) }; - u128::from_ne_bytes(cf_uuid.uuid_bytes().into()) - } - - pub fn new(id: CGDirectDisplayID) -> Self { - MonitorHandle(id) + let uuid = unsafe { CFRetained::from_raw(ptr) }; + Some(Self(uuid)) } fn refresh_rate_millihertz(&self) -> Option { let current_display_mode = - NativeDisplayMode(unsafe { CGDisplayCopyDisplayMode(self.0) }.unwrap()); - refresh_rate_millihertz(self.0, ¤t_display_mode) + NativeDisplayMode(unsafe { CGDisplayCopyDisplayMode(self.display_id()) }.unwrap()); + refresh_rate_millihertz(self.display_id(), ¤t_display_mode) } pub fn video_mode_handles(&self) -> impl Iterator { let refresh_rate_millihertz = self.refresh_rate_millihertz(); let monitor = self.clone(); - let array = unsafe { CGDisplayCopyAllDisplayModes(self.0, None) } + let array = unsafe { CGDisplayCopyAllDisplayModes(self.display_id(), None) } .expect("failed to get list of display modes"); // SAFETY: `CGDisplayCopyAllDisplayModes` is documented to return an array of display modes. let modes = unsafe { CFRetained::cast_unchecked::>(array) }; @@ -144,7 +165,8 @@ impl MonitorHandle { let uuid = self.uuid(); NSScreen::screens(mtm).into_iter().find(|screen| { let other_native_id = get_display_id(screen); - let other = MonitorHandle::new(other_native_id); + // Display ID just fetched from live NSScreen, should be fine to unwrap. + let other = MonitorHandle::new(other_native_id).expect("invalid display ID"); uuid == other.uuid() }) } @@ -156,14 +178,14 @@ impl MonitorHandleProvider for MonitorHandle { } fn native_id(&self) -> u64 { - self.0 as _ + self.display_id() as _ } // TODO: Be smarter about this: // // fn name(&self) -> Option> { - let screen_num = unsafe { CGDisplayModelNumber(self.0) }; + let screen_num = unsafe { CGDisplayModelNumber(self.display_id()) }; Some(format!("Monitor #{screen_num}").into()) } @@ -171,7 +193,7 @@ impl MonitorHandleProvider for MonitorHandle { // This is already in screen coordinates. If we were using `NSScreen`, // then a conversion would've been needed: // flip_window_screen_coordinates(self.ns_screen(mtm)?.frame()) - let bounds = unsafe { CGDisplayBounds(self.0) }; + let bounds = unsafe { CGDisplayBounds(self.display_id()) }; let position = LogicalPosition::new(bounds.origin.x, bounds.origin.y); Some(position.to_physical(self.scale_factor())) } @@ -186,8 +208,9 @@ impl MonitorHandleProvider for MonitorHandle { } fn current_video_mode(&self) -> Option { - let mode = NativeDisplayMode(unsafe { CGDisplayCopyDisplayMode(self.0) }.unwrap()); - let refresh_rate_millihertz = refresh_rate_millihertz(self.0, &mode); + let mode = + NativeDisplayMode(unsafe { CGDisplayCopyDisplayMode(self.display_id()) }.unwrap()); + let refresh_rate_millihertz = refresh_rate_millihertz(self.display_id(), &mode); Some(VideoModeHandle::new(self.clone(), mode, refresh_rate_millihertz).mode) } @@ -196,35 +219,6 @@ impl MonitorHandleProvider for MonitorHandle { } } -// `CGDirectDisplayID` changes on video mode change, so we cannot rely on that -// for comparisons, but we can use `CGDisplayCreateUUIDFromDisplayID` to get an -// unique identifier that persists even across system reboots -impl PartialEq for MonitorHandle { - fn eq(&self, other: &Self) -> bool { - self.uuid() == other.uuid() - } -} - -impl Eq for MonitorHandle {} - -impl PartialOrd for MonitorHandle { - fn partial_cmp(&self, other: &Self) -> Option { - Some(self.cmp(other)) - } -} - -impl Ord for MonitorHandle { - fn cmp(&self, other: &Self) -> std::cmp::Ordering { - self.uuid().cmp(&other.uuid()) - } -} - -impl std::hash::Hash for MonitorHandle { - fn hash(&self, state: &mut H) { - self.uuid().hash(state); - } -} - pub fn available_monitors() -> VecDeque { let mut expected_count = 0; let res = cgerr(unsafe { CGGetActiveDisplayList(0, ptr::null_mut(), &mut expected_count) }); @@ -245,20 +239,23 @@ pub fn available_monitors() -> VecDeque { let mut monitors = VecDeque::with_capacity(displays.len()); for display in displays { - monitors.push_back(MonitorHandle(display)); + // Display ID just fetched from `CGGetActiveDisplayList`, should be fine to unwrap. + monitors.push_back(MonitorHandle::new(display).expect("invalid display ID")); } monitors } pub fn primary_monitor() -> MonitorHandle { - MonitorHandle(unsafe { CGMainDisplayID() }) + // Display ID just fetched from `CGMainDisplayID`, should be fine to unwrap. + MonitorHandle::new(unsafe { CGMainDisplayID() }).expect("invalid display ID") } impl fmt::Debug for MonitorHandle { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("MonitorHandle") .field("name", &self.name()) - .field("native_id", &self.native_id()) + .field("uuid", &self.uuid()) + .field("display_id", &self.display_id()) .field("position", &self.position()) .field("scale_factor", &self.scale_factor()) .finish_non_exhaustive() @@ -336,3 +333,54 @@ fn refresh_rate_millihertz(id: CGDirectDisplayID, mode: &NativeDisplayMode) -> O .and_then(NonZeroU32::new) } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn uuid_stable() { + let handle_a = MonitorHandle::new(1).unwrap(); + let handle_b = MonitorHandle::new(1).unwrap(); + assert_eq!(handle_a, handle_b); + assert_eq!(handle_a.display_id(), handle_b.display_id()); + assert_eq!(handle_a.uuid(), handle_b.uuid()); + + let handle_a = primary_monitor(); + let handle_b = primary_monitor(); + assert_eq!(handle_a, handle_b); + assert_eq!(handle_a.display_id(), handle_b.display_id()); + assert_eq!(handle_a.uuid(), handle_b.uuid()); + } + + /// Test the MonitorHandle::new fallback. + #[test] + fn monitorhandle_from_zero() { + let handle0 = MonitorHandle::new(0).unwrap(); + let handle1 = MonitorHandle::new(1).unwrap(); + assert_eq!(handle0, handle1); + assert_eq!(handle0.display_id(), handle1.display_id()); + assert_eq!(handle0.uuid(), handle1.uuid()); + } + + #[test] + fn from_invalid_id() { + // Assume there are never this many monitors connected. + assert!(MonitorHandle::new(10000).is_none()); + } + + /// Test that calling `CGDisplayGetDisplayIDFromUUID` on an invalid UUID returns an invalid + /// display ID. + #[test] + fn invalid_monitor_handle() { + // `CGMainDisplayID` must be called to avoid: + // ``` + // Assertion failed: (did_initialize), function CGS_REQUIRE_INIT, file CGInitialization.c, line 44. + // ``` + // See https://github.com/JXA-Cookbook/JXA-Cookbook/issues/27#issuecomment-277517668 + let _ = unsafe { CGMainDisplayID() }; + + let handle = MonitorHandle(CFUUID::new(None).unwrap()); + assert_eq!(handle.display_id(), 0); + } +} diff --git a/src/platform_impl/apple/appkit/window_delegate.rs b/src/platform_impl/apple/appkit/window_delegate.rs index 14b65191aa..71d4bde0af 100644 --- a/src/platform_impl/apple/appkit/window_delegate.rs +++ b/src/platform_impl/apple/appkit/window_delegate.rs @@ -1756,7 +1756,8 @@ impl WindowDelegate { // Allow directly accessing the current monitor internally without unwrapping. pub(crate) fn current_monitor_inner(&self) -> Option { let display_id = get_display_id(&*self.window().screen()?); - Some(MonitorHandle::new(display_id)) + // Display ID just fetched from live NSScreen, should be fine to unwrap. + Some(MonitorHandle::new(display_id).expect("invalid display ID")) } #[inline]