From 2894699acec094695b45f0d9f436dd54b193422f Mon Sep 17 00:00:00 2001
From: Ryan McGrath <ryan@rymc.io>
Date: Fri, 26 Mar 2021 14:26:17 -0700
Subject: [PATCH] Fix button callbacks and avoid double-locks in listview.

---
 src/button/mod.rs       |  8 +++++---
 src/listview/mod.rs     | 20 ++++++++++++++++----
 src/utils/properties.rs |  7 +++++++
 3 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/src/button/mod.rs b/src/button/mod.rs
index 2d70855..3473ea6 100644
--- a/src/button/mod.rs
+++ b/src/button/mod.rs
@@ -23,7 +23,7 @@ use std::sync::Once;
 use std::cell::RefCell;
 use std::rc::Rc;
 
-use objc_id::Id;
+use objc_id::ShareId;
 use objc::declare::ClassDecl;
 use objc::runtime::{Class, Object, Sel};
 use objc::{class, msg_send, sel, sel_impl};
@@ -157,8 +157,10 @@ impl Button {
     /// Attaches a callback for button press events. Don't get too creative now...
     /// best just to message pass or something.
     pub fn set_action<F: Fn() + Send + Sync + 'static>(&mut self, action: F) {
-        //let handler = TargetActionHandler::new(&*self.objc, action);
-        //self.handler = Some(handler);
+        // @TODO: This probably isn't ideal but gets the job done for now; needs revisiting.
+        let this = self.objc.get(|obj| unsafe { ShareId::from_ptr(msg_send![obj, self]) });
+        let handler = TargetActionHandler::new(&*this, action);
+        self.handler = Some(handler);
     }
 
     /// Call this to set the background color for the backing layer.
diff --git a/src/listview/mod.rs b/src/listview/mod.rs
index cf1e899..0653b74 100644
--- a/src/listview/mod.rs
+++ b/src/listview/mod.rs
@@ -428,14 +428,21 @@ impl<T> ListView<T> {
         // Note that we need to thread the `with_mut` calls carefully, to avoid deadlocking.
         #[cfg(target_os = "macos")]
         {
-            self.objc.with_mut(|obj| unsafe {
+            self.objc.get(|obj| unsafe {
                 let _: () = msg_send![obj, beginUpdates];
             });
            
             let handle = self.clone_as_handle();
             update(handle);
 
-            self.objc.with_mut(|obj| unsafe {
+            // This is cheating, but there's no good way around it at the moment. If we (mutably) lock in
+            // Rust here, firing this call will loop back around into `dequeue`, which will then
+            // hit a double lock.
+            //
+            // Personally, I can live with this - `endUpdates` is effectively just flushing the
+            // already added updates, so with this small hack here we're able to keep the mutable
+            // borrow structure everywhere else, which feels "correct".
+            self.objc.get(|obj| unsafe {
                 let _: () = msg_send![obj, endUpdates];
             });
         }
@@ -461,6 +468,7 @@ impl<T> ListView<T> {
             // We need to temporarily retain this; it can drop after the underlying NSTableView
             // has also retained it.
             let x = ShareId::from_ptr(index_set);
+            
             self.objc.with_mut(|obj| {
                 let _: () = msg_send![obj, insertRowsAtIndexes:&*x withAnimation:animation_options];
             });
@@ -483,7 +491,8 @@ impl<T> ListView<T> {
             let ye: id = msg_send![class!(NSIndexSet), indexSetWithIndex:0];
             let y = ShareId::from_ptr(ye);
 
-            self.objc.with_mut(|obj| {
+            // Must use `get` to avoid a double lock.
+            self.objc.get(|obj| {
                 let _: () = msg_send![obj, reloadDataForRowIndexes:&*x columnIndexes:&*y];
             });
         }
@@ -620,7 +629,10 @@ impl<T> ListView<T> {
 
 impl<T> Layout for ListView<T> {
     fn with_backing_node<F: Fn(id)>(&self, handler: F) {
-        self.objc.with_mut(handler);
+        // On macOS, we need to provide the scrollview for layout purposes - iOS and tvOS will know
+        // what to do normally.
+        #[cfg(target_os = "macos")]
+        self.scrollview.objc.with_mut(handler);
     }
 }
 
diff --git a/src/utils/properties.rs b/src/utils/properties.rs
index 4cbe37f..76e69e6 100644
--- a/src/utils/properties.rs
+++ b/src/utils/properties.rs
@@ -24,6 +24,13 @@ impl ObjcProperty {
             Id::from_ptr(obj)
         })))
     }
+    
+    /// Given an Objective-C object, retains it and wraps it as a `Property`.
+    pub fn from_retained(obj: id) -> Self {
+        ObjcProperty(Rc::new(RefCell::new(unsafe {
+            Id::from_retained_ptr(obj)
+        })))
+    }
 
     /// Runs a handler with mutable access for the underlying Objective-C object.
     ///