[gjs/ewlsh/fix-closures: 2/2] Use GObject.Closure to marshal signal IDs

[Evan Welsh <ewlsh src gnome org>]

commit 65b2a1091af82f81ba223a5ae9d2034fa003d217
Author: Evan Welsh <contact evanwelsh com>
Date:   Sat Jan 15 12:26:53 2022 -0800

    Use GObject.Closure to marshal signal IDs
    
    Fixes #421

 gi/closure.h                  | 42 +++++++++++++++++++++++++++++-------------
 gi/object.cpp                 |  2 +-
 gi/private.cpp                | 42 ++++++++++++++++++++++++++++++++++++++++++
 gi/value.cpp                  | 29 ++++++++++++++++++++++-------
 modules/core/overrides/Gtk.js | 12 ++++++++++--
 5 files changed, 104 insertions(+), 23 deletions(-)
---
diff --git a/gi/closure.h b/gi/closure.h
index 482f1b2ec..778d856db 100644
--- a/gi/closure.h
+++ b/gi/closure.h
@@ -12,10 +12,8 @@
 #include <glib-object.h>
 #include <stddef.h>
 
-#include <js/RootingAPI.h>
 #include <js/TypeDecls.h>
 
-#include "gi/utils-inl.h"
 #include "gjs/jsapi-util-root.h"
 #include "gjs/jsapi-util.h"
 #include "gjs/macros.h"
@@ -27,6 +25,10 @@ class HandleValueArray;
 
 namespace Gjs {
 
+struct SignalClosureMeta {
+    unsigned signal_id;
+};
+
 class Closure : public GClosure {
  protected:
     Closure(JSContext*, JSFunction*, bool root, const char* description);
@@ -77,17 +79,6 @@ class Closure : public GClosure {
         return self;
     }
 
-    [[nodiscard]] static Closure* create_for_signal(JSContext* cx,
-                                                    JSFunction* callable,
-                                                    const char* description,
-                                                    int signal_id) {
-        auto* self = new Closure(cx, callable, false /* root */, description);
-        self->add_finalize_notifier<Closure>();
-        g_closure_set_meta_marshal(self, gjs_int_to_pointer(signal_id),
-                                   marshal_cb);
-        return self;
-    }
-
     constexpr JSFunction* callable() const { return m_func; }
     [[nodiscard]] constexpr JSContext* context() const { return m_cx; }
     [[nodiscard]] constexpr bool is_valid() const { return !!m_cx; }
@@ -109,6 +100,7 @@ class Closure : public GClosure {
         m_cx = nullptr;
     }
 
+ protected:
     static void marshal_cb(GClosure* closure, GValue* ret, unsigned n_params,
                            const GValue* params, void* hint, void* data) {
         for_gclosure(closure)->marshal(ret, n_params, params, hint, data);
@@ -132,6 +124,30 @@ class Closure : public GClosure {
     GjsMaybeOwned<JSFunction*> m_func;
 };
 
+class SignalClosure : public Closure {
+ protected:
+    SignalClosureMeta m_meta;
+
+    SignalClosure(JSContext* cx, JSFunction* func, const char* description,
+                  unsigned signal_id)
+        : Closure(cx, func, false, description) {
+        m_meta.signal_id = signal_id;
+
+        g_closure_set_meta_marshal(this, &m_meta, marshal_cb);
+    }
+
+ public:
+    [[nodiscard]] static SignalClosure* create(JSContext* cx,
+                                               JSFunction* callable,
+                                               const char* description,
+                                               unsigned signal_id) {
+        auto* self = new SignalClosure(cx, callable, description, signal_id);
+
+        self->add_finalize_notifier<SignalClosure>();
+        return self;
+    }
+};
+
 }  // namespace Gjs
 
 #endif  // GI_CLOSURE_H_
diff --git a/gi/object.cpp b/gi/object.cpp
index d1880966b..80a5acac6 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -2141,7 +2141,7 @@ ObjectInstance::connect_impl(JSContext          *context,
         return false;
     }
 
-    GClosure* closure = Gjs::Closure::create_for_signal(
+    GClosure* closure = Gjs::SignalClosure::create(
         context, JS_GetObjectFunction(callback), "signal callback", signal_id);
     if (closure == NULL)
         return false;
diff --git a/gi/private.cpp b/gi/private.cpp
index 6a67862bd..c8aac9d00 100644
--- a/gi/private.cpp
+++ b/gi/private.cpp
@@ -300,6 +300,46 @@ static bool gjs_create_closure(JSContext* cx, unsigned argc, JS::Value* vp) {
     return true;
 }
 
+GJS_JSAPI_RETURN_CONVENTION
+static bool gjs_create_signal_closure(JSContext* cx, unsigned argc,
+                                      JS::Value* vp) {
+    JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
+
+    JS::RootedObject owner(cx), callable(cx);
+    uint32_t signal_id = 0;
+
+    if (!gjs_parse_call_args(cx, "create_signal_closure", args, "oou", "owner",
+                             &owner, "callable", &callable, "signal_id",
+                             &signal_id))
+        return false;
+
+    if (!JS_ObjectIsFunction(callable)) {
+        gjs_throw(cx, "create_signal_closure() expects a callable function");
+        return false;
+    }
+
+    ObjectBase* base;
+    if (!ObjectInstance::for_js_typecheck(cx, owner, &base))
+        return false;
+
+    if (!base->check_is_instance(cx, "signal hookup"))
+        return false;
+
+    ObjectInstance* instance = base->to_instance();
+
+    JS::RootedFunction func(cx, JS_GetObjectFunction(callable));
+
+    Gjs::Closure* closure = Gjs::SignalClosure::create(
+        cx, func, "custom signal callback", signal_id);
+
+    if (closure == nullptr)
+        return false;
+    if (!instance->associate_closure(cx, closure))
+        return false;
+
+    return gjs_value_from_closure(cx, closure, args.rval());
+}
+
 GJS_JSAPI_RETURN_CONVENTION
 static bool gjs_invoke_closure(JSContext* cx, unsigned argc, JS::Value* vp) {
     JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
@@ -566,6 +606,8 @@ static JSFunctionSpec private_module_funcs[] = {
     JS_FN("signal_new", gjs_signal_new, 6, GJS_MODULE_PROP_FLAGS),
     JS_FN("lookupConstructor", gjs_lookup_constructor, 1, 0),
     JS_FN("create_closure", gjs_create_closure, 1, GJS_MODULE_PROP_FLAGS),
+    JS_FN("create_signal_closure", gjs_create_signal_closure, 3,
+          GJS_MODULE_PROP_FLAGS),
     JS_FN("invoke_closure", gjs_invoke_closure, 3, GJS_MODULE_PROP_FLAGS),
     JS_FS_END,
 };
diff --git a/gi/value.cpp b/gi/value.cpp
index af1f67028..d6e52686e 100644
--- a/gi/value.cpp
+++ b/gi/value.cpp
@@ -160,9 +160,23 @@ void Gjs::Closure::marshal(GValue* return_value, unsigned n_param_values,
 
     if (marshal_data) {
         /* we are used for a signal handler */
-        guint signal_id;
+        SignalClosureMeta* signal_meta =
+            static_cast<SignalClosureMeta*>(marshal_data);
+        unsigned signal_id = signal_meta->signal_id;
+
+        if (signal_id == 0) {
+            GSignalInvocationHint* hint =
+                static_cast<GSignalInvocationHint*>(invocation_hint);
+
+            if (!hint) {
+                gjs_debug(GJS_DEBUG_GCLOSURE,
+                          "Closure is not a signal handler but is being "
+                          "handled like one.");
+                return;
+            }
 
-        signal_id = GPOINTER_TO_UINT(marshal_data);
+            signal_id = hint->signal_id;
+        }
 
         g_signal_query(signal_id, &signal_query);
 
@@ -731,11 +745,12 @@ gjs_value_to_g_value_internal(JSContext      *context,
         if (!FundamentalBase::to_gvalue(context, fundamental_object, gvalue))
             return false;
     } else {
-        gjs_debug(GJS_DEBUG_GCLOSURE, "JS::Value is number %d gtype fundamental %d transformable to int %d 
from int %d",
-                  value.isNumber(),
-                  G_TYPE_IS_FUNDAMENTAL(gtype),
-                  g_value_type_transformable(gtype, G_TYPE_INT),
-                  g_value_type_transformable(G_TYPE_INT, gtype));
+        gjs_debug(GJS_DEBUG_GCLOSURE,
+                  "JS::Value is number %d\ngtype fundamental %d\ntransformable "
+                  "to int %s\ntransformable from int %s",
+                  value.isNumber(), G_TYPE_IS_FUNDAMENTAL(gtype),
+                  g_value_type_transformable(gtype, G_TYPE_INT) ? "yes" : "no",
+                  g_value_type_transformable(G_TYPE_INT, gtype) ? "yes" : "no");
 
         gjs_throw(context,
                   "Don't know how to convert JavaScript object to GType %s",
diff --git a/modules/core/overrides/Gtk.js b/modules/core/overrides/Gtk.js
index 77649a738..d974bed52 100644
--- a/modules/core/overrides/Gtk.js
+++ b/modules/core/overrides/Gtk.js
@@ -3,6 +3,7 @@
 // SPDX-FileCopyrightText: 2013 Giovanni Campagna
 
 const Legacy = imports._legacy;
+const Gi = imports._gi;
 const {Gio, GjsPrivate, GObject} = imports.gi;
 
 let Gtk;
@@ -124,14 +125,21 @@ function _init() {
         }, class extends GObject.Object {
             vfunc_create_closure(builder, handlerName, flags, connectObject) {
                 const swapped = flags & Gtk.BuilderClosureFlags.SWAPPED;
-                return _createClosure(
+                return _wrapInSignalMeta(connectObject, _createClosure(
                     builder, builder.get_current_object(),
-                    handlerName, swapped, connectObject);
+                    handlerName, swapped, connectObject));
             }
         });
     }
 }
 
+function _wrapInSignalMeta(connectObject, callable) {
+    if (connectObject instanceof GObject.Object)
+        return Gi.create_signal_closure(connectObject, callable, 0);
+    else
+        return callable;
+}
+
 function _createClosure(builder, thisArg, handlerName, swapped, connectObject) {
     connectObject = connectObject || thisArg;