GNOME Bugzilla – Bug 721838
gtk_container_child_set_property doesn't work from GJS with NULL values
Last modified: 2014-01-10 00:08:02 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
Created attachment 265802 [details] JS testcase
Created attachment 265803 [details] C testcase
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.
For now, I'd just write a MyLibrary.set_child_string_property or something which will handle the NULL special case for you.
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.
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.
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.
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.
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.
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.
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.
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.
Review of attachment 265883 [details] [review]: Looks good.
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.
(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.
Created attachment 265886 [details] [review] overrides: fix gtk_container_child_set_property() for NULL values --- Use GResource and add a g_warning.
Review of attachment 265886 [details] [review]: OK.
Attachment 265886 [details] pushed as 68b835c - overrides: fix gtk_container_child_set_property() for NULL values Thanks for the review!