After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 721838 - gtk_container_child_set_property doesn't work from GJS with NULL values
gtk_container_child_set_property doesn't work from GJS with NULL values
Status: RESOLVED FIXED
Product: gjs
Classification: Bindings
Component: general
1.39.x
Other Linux
: Normal normal
: ---
Assigned To: gjs-maint
gjs-maint
Depends on:
Blocks:
 
 
Reported: 2014-01-08 23:36 UTC by Cosimo Cecchi
Modified: 2014-01-10 00:08 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
JS testcase (215 bytes, text/javascript)
2014-01-08 23:37 UTC, Cosimo Cecchi
  Details
C testcase (471 bytes, text/plain)
2014-01-08 23:38 UTC, Cosimo Cecchi
  Details
overrides: fix gtk_container_child_set_property() for NULL values (7.58 KB, patch)
2014-01-09 21:56 UTC, Cosimo Cecchi
reviewed Details | Review
gvalue: allow NULL to transform from G_TYPE_POINTER (1.49 KB, patch)
2014-01-09 21:57 UTC, Cosimo Cecchi
rejected Details | Review
overrides: fix gtk_container_child_set_property() for NULL values (9.33 KB, patch)
2014-01-09 22:52 UTC, Cosimo Cecchi
accepted-commit_now Details | Review
overrides: fix gtk_container_child_set_property() for NULL values (11.20 KB, patch)
2014-01-09 23:46 UTC, Cosimo Cecchi
committed Details | Review

Description Cosimo Cecchi 2014-01-08 23:36:50 UTC
gtk_container_child_set_property doesn't seem to work properly from GJS with NULL values. See attached JS testcase, and a (working) testcase in C. The JS testcase will print the following error

(gjs:3495): Gtk-WARNING **: unable to set child property `name' of type `gchararray' from value of type `gpointer'

A preliminary analysis seems to point to the fact that GJS doesn't have any knowledge about the GParamSpec defining the container child property, so it proceeds with gjs_js_value_to_g_value(), which will see NULL and set the GValue to G_TYPE_POINTER. Later the g_value_transform() here [1] will fail triggering the above warning.

Any hints appreciated.

[1] https://git.gnome.org/browse/gtk+/tree/gtk/gtkcontainer.c#n886
Comment 1 Cosimo Cecchi 2014-01-08 23:37:43 UTC
Created attachment 265802 [details]
JS testcase
Comment 2 Cosimo Cecchi 2014-01-08 23:38:02 UTC
Created attachment 265803 [details]
C testcase
Comment 3 Emmanuele Bassi (:ebassi) 2014-01-09 14:19:54 UTC
any GParamSpec usage outside GObject requires ad hoc code inside language bindings, because as you've correctly pointed out, there is no automatic way to recover the GParamSpec needed for GValue instantiation. this is one of the reasons why Clutter uses actual GObject instance for child and layout properties, instead of ad hoc accessors.

there's also the issue of string values being nullable or not, which is not describable in the type system. I guess the closest thing to a workaround would be to use an empty string instead of a 'null' value, but then the class in question would need to handle empty strings as well as null values, which is not always the case.
Comment 4 Jasper St. Pierre (not reading bugmail) 2014-01-09 14:36:51 UTC
For now, I'd just write a MyLibrary.set_child_string_property or something which will handle the NULL special case for you.
Comment 5 Cosimo Cecchi 2014-01-09 16:24:17 UTC
I was using a string property here to make the example simple, but our specific use case would be a GtkWidget type child property.
In any case, the private lib helper workaround should be okay, but I'm curious if there's any interest from upstream gjs to add the ad hoc code directly here as an override.
Comment 6 Jasper St. Pierre (not reading bugmail) 2014-01-09 16:37:22 UTC
I'm not exactly sure what the override would do. Perhaps something that we could look into is allowing NULL values to be transformable to GTK_TYPE_WIDGET or G_TYPE_STRING.
Comment 7 Cosimo Cecchi 2014-01-09 17:39:46 UTC
I like the idea of making the transform function work for those cases. If it works, we could also allow it for G_TYPE_OBJECT in general, as I don't see why GTK_TYPE_WIDGET would be special in that regard. I'll explore this route.

I was initially thinking the special case would use e.g. gtk_container_class_find_child_property() to get the GParamSpec and then construct a GValue with the right type.
Comment 8 Cosimo Cecchi 2014-01-09 21:56:49 UTC
Created attachment 265879 [details] [review]
overrides: fix gtk_container_child_set_property() for NULL values

Install a GTK override with a fix for
gtk_container_chilld_set_property() not using the correct GType for NULL
values.
Comment 9 Cosimo Cecchi 2014-01-09 21:57:56 UTC
Created attachment 265880 [details] [review]
gvalue: allow NULL to transform from G_TYPE_POINTER

...to G_TYPE_STRING, G_TYPE_OBJECT and G_TYPE_BOXED types.
Comment 10 Cosimo Cecchi 2014-01-09 21:59:31 UTC
So I went ahead and tried fixing this both in GLib and with a GJS override.
After writing the code, I don't particularly like the GLib fix approach to be honest, and I'm not sure if this breaks some of the GValue API contracts.

Feedback welcome.
Comment 11 Jasper St. Pierre (not reading bugmail) 2014-01-09 22:02:04 UTC
Review of attachment 265879 [details] [review]:

This is a lot cleaner than I thought it'd be. Nice job.

::: configure.ac
@@ +76,2 @@
 common_packages="gmodule-2.0 gthread-2.0 gio-2.0 >= glib_required_version mozjs-17.0"
+gjs_packages="gobject-introspection-1.0 libffi gtk+-3.0 $common_packages"

Hm, I know Colin wanted to keep cairo out for a reason. Could we conditionally enable this? Would be even better if we didn't have to link against GTK+ at all, but, eh, well.

::: libgjs-private/gjs-gtk-util.c
@@ +43,3 @@
+        override = *value;
+    } else if ((G_VALUE_TYPE (value) == G_TYPE_POINTER) &&
+               !g_value_get_pointer (value)) {

Couldn't this be:

    if (G_VALUE_TYPE (value) == G_TYPE_POINTER && g_value_get_pointer (value) == NULL) {
        /* Special-case NULL because... */
        GValue null_value;
        g_value_init (&null_value, pspec->value_type);
        gtk_container_child_set_property (...);
        g_value_unset (&null_value);
    } else {
        gtk_container_child_set_property (...);
    }

I think that would be a lot cleaner, and sets us up for future overrides later.
Comment 12 Cosimo Cecchi 2014-01-09 22:52:50 UTC
Created attachment 265883 [details] [review]
overrides: fix gtk_container_child_set_property() for NULL values

---

Thanks for the review. This should address your comments - I left the g_value_type_transformable() check in case a GtkContainer defines a child property that is actually G_TYPE_POINTER.
Comment 13 Jasper St. Pierre (not reading bugmail) 2014-01-09 23:02:28 UTC
Review of attachment 265883 [details] [review]:

Looks good.
Comment 14 Emmanuele Bassi (:ebassi) 2014-01-09 23:09:47 UTC
Review of attachment 265883 [details] [review]:

::: libgjs-private/gjs-gtk-util.c
@@ +36,3 @@
+    pspec = gtk_container_class_find_child_property (G_OBJECT_GET_CLASS (container),
+                                                     property);
+    g_return_if_fail (pspec != NULL);

wouldn't it be better to have a warning in place that says that the property does not exist? you're intercepting child_set_property().

also, g_return_if_fail() can be compiled out.
Comment 15 Cosimo Cecchi 2014-01-09 23:11:17 UTC
(In reply to comment #14)

> wouldn't it be better to have a warning in place that says that the property
> does not exist? you're intercepting child_set_property().
> 
> also, g_return_if_fail() can be compiled out.

Good idea - I also realized this patch needs a bit of change for git master to use the new GResource framework. I'll post an updated one in a bit.
Comment 16 Cosimo Cecchi 2014-01-09 23:46:14 UTC
Created attachment 265886 [details] [review]
overrides: fix gtk_container_child_set_property() for NULL values

---

Use GResource and add a g_warning.
Comment 17 Jasper St. Pierre (not reading bugmail) 2014-01-10 00:05:23 UTC
Review of attachment 265886 [details] [review]:

OK.
Comment 18 Cosimo Cecchi 2014-01-10 00:07:51 UTC
Attachment 265886 [details] pushed as 68b835c - overrides: fix gtk_container_child_set_property() for NULL values

Thanks for the review!