macOS: use CFRunLoopTimer, improve view release logic (#84)

* macOS: fix property_no fn

* Use CFRunLoopTimer instead if NSTimer

This means the timer doesn't keep a reference to the view,
which should make it easer to check retain_count in release.

* macOS: take pointer instead of Arc in WindowState::setup_timer

* Save retain count increase from build fn, use in release fn

* macOS: in window setup, run build fn before doing parenting

* macOS: clean up parenting

* macOS: wrap WindowState in Box instead of Arc to improve clarity

* macOS: use better names for ivar consts, move them to view.rs

* Remove no longer used crate static_assertions

* macOS: in view release fn, delete class when retain_count == 1

* macOS: set window state ivar to null after dropping

* macOS: store retain count after build in WindowState

* macOS: rename BASEVIEW_WINDOW_STATE_IVAR to BASEVIEW_STATE_IVAR

Author: greatest-ape

Cargo.toml

@@ -16,7 +16,6 @@ license = "MIT OR Apache-2.0"
 [dependencies]
 keyboard-types = { version = "0.5.0", default-features = false }
 raw-window-handle = "0.3.3"
-static_assertions = "1.1.0"
 
 [target.'cfg(target_os="linux")'.dependencies]
 xcb = { version = "0.9", features = ["thread", "xlib_xcb", "dri2"] }
@@ -29,6 +28,7 @@ winapi = { version = "0.3.8", features = ["libloaderapi", "winuser", "windef", "
 
 [target.'cfg(target_os="macos")'.dependencies]
 cocoa = "0.24.0"
+core-foundation = "0.9.1"
 objc = "0.2.7"
 uuid = { version = "0.8", features = ["v4"] }
 

src/macos/view.rs

@@ -1,5 +1,4 @@
 use std::ffi::c_void;
-use std::sync::Arc;
 
 use cocoa::appkit::{NSEvent, NSView};
 use cocoa::base::{id, nil, BOOL, YES, NO};
@@ -17,9 +16,11 @@ use uuid::Uuid;
 use crate::{Event, MouseButton, MouseEvent, Point, WindowOpenOptions};
 use crate::MouseEvent::{ButtonPressed, ButtonReleased};
 
-use super::window::{
-    WindowState, WINDOW_STATE_IVAR_NAME, FRAME_TIMER_IVAR_NAME
-};
+use super::window::WindowState;
+
+
+/// Name of the field used to store the `WindowState` pointer.
+pub(super) const BASEVIEW_STATE_IVAR: &str = "baseview_state";
 
 
 pub(super) unsafe fn create_view(
@@ -66,14 +67,9 @@ unsafe fn create_view_class() -> &'static Class {
         accepts_first_mouse as extern "C" fn(&Object, Sel, id) -> BOOL
     );
 
-    class.add_method(
-        sel!(triggerOnFrame:),
-        trigger_on_frame as extern "C" fn(&Object, Sel, id)
-    );
-
     class.add_method(
         sel!(release),
-        release as extern "C" fn(&Object, Sel)
+        release as extern "C" fn(&mut Object, Sel)
     );
     class.add_method(
         sel!(viewWillMoveToWindow:),
@@ -151,8 +147,7 @@ unsafe fn create_view_class() -> &'static Class {
         flags_changed as extern "C" fn(&Object, Sel, id),
     );
 
-    class.add_ivar::<*mut c_void>(WINDOW_STATE_IVAR_NAME);
-    class.add_ivar::<*mut c_void>(FRAME_TIMER_IVAR_NAME);
+    class.add_ivar::<*mut c_void>(BASEVIEW_STATE_IVAR);
 
     class.register()
 }
@@ -170,7 +165,7 @@ extern "C" fn property_no(
     _this: &Object,
     _sel: Sel,
 ) -> BOOL {
-    YES
+    NO
 }
 
 
@@ -183,20 +178,7 @@ extern "C" fn accepts_first_mouse(
 }
 
 
-extern "C" fn trigger_on_frame(
-    this: &Object,
-    _sel: Sel,
-    _event: id
-){
-    let state: &mut WindowState = unsafe {
-        WindowState::from_field(this)
-    };
-
-    state.trigger_frame();
-}
-
-
-extern "C" fn release(this: &Object, _sel: Sel) {
+extern "C" fn release(this: &mut Object, _sel: Sel) {
     unsafe {
         let superclass = msg_send![this, superclass];
 
@@ -206,19 +188,26 @@ extern "C" fn release(this: &Object, _sel: Sel) {
     unsafe {
         let retain_count: usize = msg_send![this, retainCount];
 
-        if retain_count == 1 {
-            // Invalidate frame timer
-            let frame_timer_ptr: *mut c_void = *this.get_ivar(
-                FRAME_TIMER_IVAR_NAME
-            );
-            let _: () = msg_send![frame_timer_ptr as id, invalidate];
-
-            // Drop WindowState
-            let state_ptr: *mut c_void = *this.get_ivar(
-                WINDOW_STATE_IVAR_NAME
-            );
-            Arc::from_raw(state_ptr as *mut WindowState);
+        let state_ptr: *mut c_void = *this.get_ivar(BASEVIEW_STATE_IVAR);
+
+        if !state_ptr.is_null(){
+            let retain_count_after_build = WindowState::from_field(this)
+                .retain_count_after_build;
 
+            if retain_count <= retain_count_after_build {
+                WindowState::from_field(this).remove_timer();
+
+                this.set_ivar(
+                    BASEVIEW_STATE_IVAR,
+                    ::std::ptr::null() as *const c_void
+                );
+
+                // Drop WindowState
+                Box::from_raw(state_ptr as *mut WindowState);
+            }
+        }
+
+        if retain_count == 1 {
             // Delete class
             let class = msg_send![this, class];
             ::objc::runtime::objc_disposeClassPair(class);

src/macos/window.rs

@@ -1,12 +1,15 @@
 use std::ffi::c_void;
-use std::sync::Arc;
 
 use cocoa::appkit::{
     NSApp, NSApplication, NSApplicationActivationPolicyRegular,
     NSBackingStoreBuffered, NSWindow, NSWindowStyleMask,
 };
-use cocoa::base::{id, nil, NO, YES};
+use cocoa::base::{id, nil, NO};
 use cocoa::foundation::{NSAutoreleasePool, NSPoint, NSRect, NSSize, NSString};
+use core_foundation::runloop::{
+    CFRunLoop, CFRunLoopTimer, CFRunLoopTimerContext, __CFRunLoopTimer,
+    kCFRunLoopDefaultMode,
+};
 use keyboard_types::KeyboardEvent;
 
 use objc::{msg_send, runtime::Object, sel, sel_impl};
@@ -18,17 +21,10 @@ use crate::{
     WindowScalePolicy, WindowInfo
 };
 
-use super::view::create_view;
+use super::view::{create_view, BASEVIEW_STATE_IVAR};
 use super::keyboard::KeyboardState;
 
 
-/// Name of the field used to store the `WindowState` pointer in the custom
-/// view class.
-pub(super) const WINDOW_STATE_IVAR_NAME: &str = "WINDOW_STATE_IVAR_NAME";
-
-pub(super) const FRAME_TIMER_IVAR_NAME: &str = "FRAME_TIMER";
-
-
 pub struct AppRunner;
 
 impl AppRunner {
@@ -61,38 +57,35 @@ impl Window {
     {
         let _pool = unsafe { NSAutoreleasePool::new(nil) };
 
-        let (mut window, opt_app_runner) = match options.parent {
-            Parent::WithParent(parent) => {
-                if let RawWindowHandle::MacOS(handle) = parent {
-                    let ns_view = handle.ns_view as *mut objc::runtime::Object;
-
-                    unsafe {
-                        let subview = create_view(&options);
+        let mut window = unsafe {
+            Window {
+                ns_window: None,
+                ns_view: create_view(&options),
+            }
+        };
 
-                        let _: id = msg_send![ns_view, addSubview: subview];
+        let window_handler = Box::new(build(&mut crate::Window(&mut window)));
 
-                        let window = Window {
-                            ns_window: None,
-                            ns_view: subview,
-                        };
+        let retain_count_after_build: usize = unsafe {
+            msg_send![window.ns_view, retainCount]
+        };
 
-                        (window, None)
-                    }
-                } else {
-                    panic!("Not a macOS window");
+        let opt_app_runner = match options.parent {
+            Parent::WithParent(RawWindowHandle::MacOS(handle)) => {
+                unsafe {
+                    let () = msg_send![
+                        handle.ns_view as *mut Object,
+                        addSubview: window.ns_view
+                    ];
                 }
+
+                None
+            },
+            Parent::WithParent(_) => {
+                panic!("Not a macOS window");
             },
             Parent::AsIfParented => {
-                let ns_view = unsafe {
-                    create_view(&options)
-                };
-
-                let window = Window {
-                    ns_window: None,
-                    ns_view,
-                };
-
-                (window, None)
+                None
             },
             Parent::None => {
                 // It seems prudent to run NSApp() here before doing other
@@ -143,58 +136,30 @@ impl Window {
 
                     ns_window.makeKeyAndOrderFront_(nil);
 
-                    let subview = create_view(&options);
+                    ns_window.setContentView_(window.ns_view);
 
-                    ns_window.setContentView_(subview);
+                    window.ns_window = Some(ns_window);
 
-                    let window = Window {
-                        ns_window: Some(ns_window),
-                        ns_view: subview,
-                    };
-
-                    (window, Some(crate::AppRunner(AppRunner)))
+                    Some(crate::AppRunner(AppRunner))
                 }
             },
         };
 
-        let window_handler = Box::new(build(&mut crate::Window(&mut window)));
-
-        let window_state_arc = Arc::new(WindowState {
+        let window_state_ptr = Box::into_raw(Box::new(WindowState {
             window,
             window_handler,
             keyboard_state: KeyboardState::new(),
-        });
-
-        let window_state_pointer = Arc::into_raw(
-            window_state_arc.clone()
-        ) as *mut c_void;
+            frame_timer: None,
+            retain_count_after_build,
+        }));
 
         unsafe {
-            (*window_state_arc.window.ns_view).set_ivar(
-                WINDOW_STATE_IVAR_NAME,
-                window_state_pointer
+            (*(*window_state_ptr).window.ns_view).set_ivar(
+                BASEVIEW_STATE_IVAR,
+                window_state_ptr as *mut c_void
             );
-        }
 
-        // Setup frame timer once window state is stored
-        unsafe {
-            let timer_interval = 0.015;
-            let selector = sel!(triggerOnFrame:);
-
-            let timer: id = msg_send![
-                ::objc::class!(NSTimer),
-                scheduledTimerWithTimeInterval:timer_interval
-                target:window_state_arc.window.ns_view
-                selector:selector
-                userInfo:nil
-                repeats:YES
-            ];
-
-            // Store pointer to timer for invalidation when view is released
-            (*window_state_arc.window.ns_view).set_ivar(
-                FRAME_TIMER_IVAR_NAME,
-                timer as *mut c_void,
-            )
+            WindowState::setup_timer(window_state_ptr);
         }
 
         opt_app_runner
@@ -206,6 +171,8 @@ pub(super) struct WindowState {
     window: Window,
     window_handler: Box<dyn WindowHandler>,
     keyboard_state: KeyboardState,
+    frame_timer: Option<CFRunLoopTimer>,
+    pub retain_count_after_build: usize,
 }
 
 
@@ -216,7 +183,7 @@ impl WindowState {
     /// WindowState. Apparently, macOS blocks for the duration of an event,
     /// callback, meaning that this shouldn't be a problem in practice.
     pub(super) unsafe fn from_field(obj: &Object) -> &mut Self {
-        let state_ptr: *mut c_void = *obj.get_ivar(WINDOW_STATE_IVAR_NAME);
+        let state_ptr: *mut c_void = *obj.get_ivar(BASEVIEW_STATE_IVAR);
 
         &mut *(state_ptr as *mut Self)
     }
@@ -238,6 +205,54 @@ impl WindowState {
     ) -> Option<KeyboardEvent> {
         self.keyboard_state.process_native_event(event)
     }
+
+    /// Don't call until WindowState pointer is stored in view
+    unsafe fn setup_timer(window_state_ptr: *mut WindowState){
+        extern "C" fn timer_callback(
+            _: *mut __CFRunLoopTimer,
+            window_state_ptr: *mut c_void,
+        ){
+            unsafe {
+                let window_state = &mut *(
+                    window_state_ptr as *mut WindowState
+                );
+
+                window_state.trigger_frame();
+            }
+        }
+
+        let mut timer_context = CFRunLoopTimerContext {
+            version: 0,
+            info: window_state_ptr as *mut c_void,
+            retain: None,
+            release: None,
+            copyDescription: None,
+        };
+
+        let timer = CFRunLoopTimer::new(
+            0.0,
+            0.015,
+            0,
+            0,
+            timer_callback,
+            &mut timer_context,
+        );
+
+        CFRunLoop::get_current()
+            .add_timer(&timer, kCFRunLoopDefaultMode);
+        
+        let window_state = &mut *(window_state_ptr);
+
+        window_state.frame_timer = Some(timer);
+    }
+
+    /// Call when freeing view
+    pub(super) unsafe fn remove_timer(&mut self){
+        if let Some(frame_timer) = self.frame_timer.take(){
+            CFRunLoop::get_current()
+                .remove_timer(&frame_timer, kCFRunLoopDefaultMode);
+        }
+    }
 }