Merge branch 'even-better-nseventforwarder'

2 files changed, 52 insertions, 58 deletions

diff --git a/desktop/packages/nseventforwarder/nseventforwarder-rs/lib.rs b/desktop/packages/nseventforwarder/nseventforwarder-rs/lib.rs
index 1109cd45f0..09c7271e5a 100644
--- a/desktop/packages/nseventforwarder/nseventforwarder-rs/lib.rs
+++ b/desktop/packages/nseventforwarder/nseventforwarder-rs/lib.rs
@@ -2,15 +2,14 @@
 #![cfg(target_os = "macos")]
 #![warn(clippy::undocumented_unsafe_blocks)]
 
+use std::ptr::NonNull;
 use std::sync::{mpsc, Arc, Mutex};
 use std::thread::JoinHandle;
 
 use block2::RcBlock;
 use neon::prelude::{
-    Context, FunctionContext, Handle, JsFunction, JsNull, JsResult, JsUndefined, ModuleContext,
-    NeonResult, Object, Root,
+    Context, FunctionContext, JsFunction, JsNull, JsResult, ModuleContext, NeonResult, Object, Root,
 };
-use neon::result::Throw;
 use objc2_app_kit::{NSEvent, NSEventMask};
 
 #[neon::main]
@@ -19,64 +18,44 @@ fn main(mut cx: ModuleContext<'_>) -> NeonResult<()> {
     Ok(())
 }
 
-/// NSEventForwarder instance. It must be initialized by `start` and cleaned up by the callback
-/// function returned from `start`.
-static NSEVENTFORWARDER: Mutex<Option<NSEventForwarder>> = Mutex::new(None);
-
-struct NSEventForwarder {
-    /// The thread listening for incoming [NSEvent]s.
-    thread: JoinHandle<()>,
-    /// Signal for the current execution context to stop.
-    stop: mpsc::Sender<()>,
-}
-
-impl NSEventForwarder {
-    fn stop(self) {
-        // Tell the thread to stop running
-        let _ = self.stop.send(());
-        // Wait for the thread to shutdown
-        self.thread
-            .join()
-            .expect("Couldn't join the NSEventForwarder thread");
-    }
-}
-
 /// Register a callback to fire every time a [NSEventMask::LeftMouseDown] or [NSEventMask::RightMouseDown] event occur.
 ///
-/// Returns a stop function to call when the original callback shouldn't be called anymore.
+/// Returns a stop function to call when the original callback shouldn't be called anymore. This
+/// stop function returns a `true` value when called the first time and the callback is
+/// deregistered. If it were to be called yet again, it will keep returning `false`.
 fn start(mut cx: FunctionContext<'_>) -> JsResult<'_, JsFunction> {
-    // Set up neon stuff
-    let callback = cx.argument::<JsFunction>(0)?.root(&mut cx);
-    let callback: Arc<Root<JsFunction>> = Arc::new(callback);
+    // Set up neon stuff.
+    // These will be moved into the spawned thread
+    let nodejs_callback = cx.argument::<JsFunction>(0)?.root(&mut cx);
     let channel = cx.channel();
-
     // Start a long-running thread which handles incoming NS events
     // When a new event is received, call the callback passed to us from the JavaScript caller
     let (stop_tx, stop_rx) = mpsc::channel();
     let join_handle = std::thread::spawn(move || {
-        // Scaffolding for calling the JavaScript callback function
-        let call_callback = move || {
-            let cb = Arc::clone(&callback);
+        // Each time the nodejs callback is triggered, we need to reference the nodejs
+        // function reference. As such, we keep it from being garbage-collected with the
+        // Root handle type and allow sharing it with the RCBlock via an Arc.
+        let nodesjs_callback: Arc<Root<JsFunction>> = Arc::new(nodejs_callback);
+        // Create a callback which will be called on the registered NSEvents.
+        // When called schedules a closure to execute in nodejs thread that invoked start.
+        let nsevent_callback = move |_nsevent: NonNull<NSEvent>| {
+            let nodejs_callback = Arc::clone(&nodesjs_callback);
             channel.send(move |mut cx| {
                 let this = JsNull::new(&mut cx);
-                let _ = cb.to_inner(&mut cx).call(&mut cx, this, []);
+                let _ = nodejs_callback.to_inner(&mut cx).call(&mut cx, this, []);
                 Ok(())
-            })
+            });
         };
         // Start monitoring incoming NS events
-        let block = RcBlock::new(move |_event| {
-            call_callback();
-        });
         // SAFETY: This function is trivially safe to call.
         // Note: Make sure to cancel this handler with [NSEvent::removeMonitor] to unregister the
         // listener.
         let mut handler = unsafe {
             NSEvent::addGlobalMonitorForEventsMatchingMask_handler(
                 NSEventMask::LeftMouseDown | NSEventMask::RightMouseDown,
-                &block,
+                &RcBlock::new(nsevent_callback),
             )
         };
-
         // Listen for stop signal
         let _ = stop_rx.recv();
         if let Some(handler) = handler.take() {
@@ -87,27 +66,42 @@ fn start(mut cx: FunctionContext<'_>) -> JsResult<'_, JsFunction> {
         // The thread's execution will stop when this function returns
     });
 
-    let new_context = NSEventForwarder {
+    // NSEventForwarder instance. It must be cleaned up by the callback
+    // function returned from `start` (aka `stop`). We use an Option here
+    // because we can not enforce the Nodejs caller to only call `stop` once.
+    let nseventforwarder = Mutex::new(Some(NSEventForwarder {
         thread: join_handle,
         stop: stop_tx,
-    };
+    }));
 
-    // Update the global NSEventForwarder state
-    let mut nseventmonitor_context = NSEVENTFORWARDER.lock().unwrap();
-    // Stop any old NSEventForwarder
-    if let Some(context) = nseventmonitor_context.take() {
-        context.stop();
-    }
-    let _ = nseventmonitor_context.insert(new_context);
-    drop(nseventmonitor_context);
+    // Return a stop function to be invoked from the node runtime to deregister the NSEvent
+    // callback.
+    JsFunction::new(&mut cx, move |mut cx: FunctionContext<'_>| {
+        // Stop this NSEventForwarder
+        // Returns whether NSEventForwarder was stopped on this invocation of the stop function
+        let mut stopped = false;
+        if let Some(context) = nseventforwarder.lock().unwrap().take() {
+            context.stop();
+            stopped = true;
+        }
+        Ok(cx.boolean(stopped))
+    })
+}
 
-    JsFunction::new(&mut cx, stop)
+struct NSEventForwarder {
+    /// The thread listening for incoming [NSEvent]s.
+    thread: JoinHandle<()>,
+    /// Signal for the current execution context to stop.
+    stop: mpsc::Sender<()>,
 }
 
-fn stop(mut cx: FunctionContext<'_>) -> Result<Handle<'_, JsUndefined>, Throw> {
-    if let Some(context) = NSEVENTFORWARDER.lock().unwrap().take() {
-        context.stop();
+impl NSEventForwarder {
+    fn stop(self) {
+        // Tell the thread to stop running
+        let _ = self.stop.send(());
+        // Wait for the thread to shutdown
+        self.thread
+            .join()
+            .expect("Couldn't join the NSEventForwarder thread");
     }
-
-    Ok(JsUndefined::new(&mut cx))
 }
diff --git a/desktop/packages/nseventforwarder/src/index.cts b/desktop/packages/nseventforwarder/src/index.cts
index bee20624ae..0d480d15ad 100644
--- a/desktop/packages/nseventforwarder/src/index.cts
+++ b/desktop/packages/nseventforwarder/src/index.cts
@@ -6,9 +6,9 @@ import * as addon from './load.cjs';
 // Use this declaration to assign types to the addon's exports,
 // which otherwise by default are `any`.
 declare module './load.cjs' {
-  function start(cb: () => void): () => void;
+  function start(cb: () => void): () => boolean;
 }
 
-export function start(cb: () => void): () => void {
+export function start(cb: () => void): () => boolean {
   return addon.start(cb);
 }