Use GObject.Closure to marshal signal IDsewlsh/fix-closures

Fixes #421

6 files changed, 105 insertions, 17 deletions

diff --git a/gi/arg.cpp b/gi/arg.cpp
index 636d7820..e00ca0e2 100644
--- a/gi/arg.cpp
+++ b/gi/arg.cpp
@@ -1357,8 +1357,7 @@ static bool value_to_interface_gi_argument(
                         return true;
                     }
 
-                    Gjs::Closure* closure =
-                        BoxedBase::to_c_ptr<Gjs::Closure>(cx, obj);
+                    GClosure* closure = BoxedBase::to_c_ptr<GClosure>(cx, obj);
                     if (!closure)
                         return false;
 
diff --git a/gi/closure.h b/gi/closure.h
index 220d5f9e..b0d34216 100644
--- a/gi/closure.h
+++ b/gi/closure.h
@@ -26,6 +26,10 @@ class HandleValueArray;
 
 namespace Gjs {
 
+struct SignalClosureMeta {
+    unsigned signal_id;
+};
+
 class Closure : public GClosure {
  protected:
     Closure(JSContext*, JSObject* callable, bool root, const char* description);
@@ -108,6 +112,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);
@@ -131,6 +136,30 @@ class Closure : public GClosure {
     GjsMaybeOwned<JSObject*> m_callable;
 };
 
+class SignalClosure : public Closure {
+ protected:
+    SignalClosureMeta m_meta;
+
+    SignalClosure(JSContext* cx, JSObject* callable, const char* description,
+                  unsigned signal_id)
+        : Closure(cx, callable, 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,
+                                               JSObject* 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 62e79c18..b401c25d 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -2186,7 +2186,7 @@ ObjectInstance::connect_impl(JSContext          *context,
         return false;
     }
 
-    GClosure* closure = Gjs::Closure::create_for_signal(
+    GClosure* closure = Gjs::SignalClosure::create(
         context, callback, "signal callback", signal_id);
     if (closure == NULL)
         return false;
diff --git a/gi/private.cpp b/gi/private.cpp
index 9eba0d94..c09c6c9d 100644
--- a/gi/private.cpp
+++ b/gi/private.cpp
@@ -341,14 +341,13 @@ static bool gjs_create_closure(JSContext* cx, unsigned argc, JS::Value* vp) {
         return false;
     }
 
-    JS::RootedFunction func(cx, JS_GetObjectFunction(callable));
+    Gjs::Closure::Ptr closure(
+        Gjs::Closure::create_marshaled(cx, callable, "custom callback", false),
+        GjsAutoTakeOwnership{});
 
-    Gjs::Closure* closure =
-        Gjs::Closure::create_marshaled(cx, func, "custom callback", false);
     if (closure == nullptr)
         return false;
 
-    g_closure_ref(closure);
     g_closure_sink(closure);
 
     GjsAutoStructInfo info =
@@ -357,7 +356,6 @@ static bool gjs_create_closure(JSContext* cx, unsigned argc, JS::Value* vp) {
 
     JS::RootedObject boxed(cx,
                            BoxedInstance::new_for_c_struct(cx, info, closure));
-    g_closure_unref(closure);
 
     if (!boxed)
         return false;
@@ -367,6 +365,44 @@ static bool gjs_create_closure(JSContext* cx, unsigned argc, JS::Value* vp) {
 }
 
 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();
+
+    Gjs::Closure* closure = Gjs::SignalClosure::create(
+        cx, callable, "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_register_type_impl(JSContext* cx, const char* name,
                                    GTypeFlags type_flags,
                                    JS::HandleObject parent,
@@ -626,6 +662,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_FS_END,
 };
 
diff --git a/gi/value.cpp b/gi/value.cpp
index 17db002f..38ec7f2a 100644
--- a/gi/value.cpp
+++ b/gi/value.cpp
@@ -165,9 +165,22 @@ void Gjs::Closure::marshal(GValue* return_value, unsigned n_param_values,
 
     if (marshal_data) {
         /* we are used for a signal handler */
-        guint signal_id;
+        auto* 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);
 
@@ -798,11 +811,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 ce63ba4e..8c790073 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;
 const {_registerType} = imports._common;
 
@@ -143,14 +144,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;