I guess the root of this problem is that Gtk.Image defines its own file property, as a char*. Then we try to define a new property with the same name and different type, and things go wrong.

Here is the gdb trace:

+ Trace 231116

Inside pygi_get_property_value_real():

(gdb) p type_tag
$14 = GI_TYPE_TAG_UTF8
(gdb) p value
$15 = {g_type = 135674264, data = {{v_int = 0, v_uint = 0, v_long = 0, 
      v_ulong = 0, v_int64 = 0, v_uint64 = 0, v_float = 0, v_double = 0, 
      v_pointer = 0x0}, {v_int = 0, v_uint = 0, v_long = 0, v_ulong = 0, 
      v_int64 = 0, v_uint64 = 0, v_float = 0, v_double = 0, v_pointer = 0x0}}}
(gdb) p (char *) g_type_name(135674264)
$16 = 0x8164040 "PyObject"


So it tries to call g_value_dup_string() on a PyObject type, and its not happy for obvious reasons.

For the next step, I need to remind myself what would happen in C here, where a subclass defines a property with the same name as one in the parent but with a different type.

Created attachment 227752 [details] [review]
patch

Here's a patch with full description inside.

The only bit I'm not sure about is what to do with canonicalize_key. What was the purpose of this, what does it solve? I wrote to the mailing list for wider attention.

Review of attachment 227752 [details] [review]:

This is great and simplifies the internals a bit. In general it seems like a bad idea to override properties at all, are there any warnings that occur when you do this? I just have a few comments and questions. Thanks.

::: gi/_gobject/pygobject.c
@@ +263,1 @@
     pspec = g_object_class_find_property(class, attr_name);

canonicalize_key will need to happen on attr_name or a copy of it before this call. The reason is python properties must use underscores whereas gobject uses hyphens. This will replace underscores with hyphens to make it gobject compatible.

@@ +288,1 @@
+    /* If we reach here, it must be a property defined at the Python level.

I gather "defined at the Python level" means it is a gobject property defined in python (or some other way) and not exposed through gi. In this case it is not necessarily defined in python, just defined dynamically and outside of gi typelibs.

@@ +369,1 @@
     pspec = g_object_class_find_property(G_OBJECT_GET_CLASS(obj), attr_name);

attr_name or a copy of it should be canonicalized before this call.

::: gi/pygi-property.c
@@ +54,2 @@
     repository = g_irepository_get_default();
     info = g_irepository_find_by_gtype (repository, g_type);

Ideally it would be nice if the rest of this function could simply become an API call: g_object_info_find_property
But that's for another time.

@@ +55,3 @@
     info = g_irepository_find_by_gtype (repository, g_type);
+    if (info == NULL || !GI_IS_OBJECT_INFO(info))
+       return NULL;

So now that g_object_class_find_property is being used before the call to pygi_get_property_value within PyGProps_set/getattro, we no longer need to do the recursive lookup here? this is much nicer.

As a side note, if info is not NULL but also not an object info, you need to unref it before returning. Also do you know why is this section of would be entered on a non object type?

::: gi/pygi.h
@@ +78,3 @@
     PyObject* (*type_import_by_g_type) (GType g_type);
     PyObject* (*get_property_value) (PyGObject *instance,
+                                     GParamSpec *pspec);

Please verify the PyGI_API is only used internally by pygi/pygobject. I don't know anything about it other than internals, but if there are clients of it outside of pygobject it would be an API breakage.

::: tests/test_properties.py
@@ +21,3 @@
 from gi.repository import Gio
 from gi.repository import GLib
+from gi.repository import Gtk

Tests dependent on Gtk should live in test_overrides_gtk.py. If possible please use test objects from the Regress or GIMarshellingTests packages here instead of Gtk. Please also verify we have coverage of all possible combinations of property usage and inheritance.

pygobject should attempt independence from gtk as eventually the Gtk overrides and unittests will be moved outside of pygobject, so keeping it separate will make it easier in the future.

Thanks for the review.

(In reply to comment #3)
> This is great and simplifies the internals a bit. In general it seems like a
> bad idea to override properties at all, are there any warnings that occur when
> you do this? I just have a few comments and questions. Thanks.

No, not here nor in C. Also, the original bug/test case comes from part of a public API that we've been shipping for a while, with many users, so changing it would be a real pain :/
It also doesn't seem like an impossible situation to run into in general, with some standard GTK objects having a large inheritance chain and properties being added to those GTK classes now and then which may conflict with properties defined by existing users on their own subclasses.
 
> @@ +55,3 @@
>      info = g_irepository_find_by_gtype (repository, g_type);
> +    if (info == NULL || !GI_IS_OBJECT_INFO(info))
> +       return NULL;
> 
> So now that g_object_class_find_property is being used before the call to
> pygi_get_property_value within PyGProps_set/getattro, we no longer need to do
> the recursive lookup here? this is much nicer.

Yes - mentioned in the commit message.

> As a side note, if info is not NULL but also not an object info, you need to
> unref it before returning. Also do you know why is this section of would be
> entered on a non object type?

I don't really understand why we didn't hit it before, but we can now arrive here with an interface. This is triggered by test_overrides_gtk.TestGtk.test_viewport: a viewport implements the GtkScrollable interface, and the test adjusts the property owned by the GtkScrollable interface. Without that particular change, the test was crashing.

The next version of the patch adds support for handling GIInterfaceInfo here.

> ::: gi/pygi.h
> @@ +78,3 @@
>      PyObject* (*type_import_by_g_type) (GType g_type);
>      PyObject* (*get_property_value) (PyGObject *instance,
> +                                     GParamSpec *pspec);
> 
> Please verify the PyGI_API is only used internally by pygi/pygobject. I don't
> know anything about it other than internals, but if there are clients of it
> outside of pygobject it would be an API breakage.

I googled around and couldn't find any other users.
 
> ::: tests/test_properties.py
> @@ +21,3 @@
>  from gi.repository import Gio
>  from gi.repository import GLib
> +from gi.repository import Gtk
> 
> Tests dependent on Gtk should live in test_overrides_gtk.py. If possible please
> use test objects from the Regress or GIMarshellingTests packages here instead
> of Gtk. Please also verify we have coverage of all possible combinations of
> property usage and inheritance.

OK, we can use Regress for the same kind of test, and I've also added a few more tests.

I've also addressed all the other comments in the patch coming up.

Created attachment 227816 [details] [review]
patch v2

Bump...any chance of a second review here? This is breaking a large part of Sugar. Thanks!

Review of attachment 227816 [details] [review]:

This looks, good thanks. I still have some reservation about the PyGI_API (mostly because I don't fully understand its usage) and will try to gather more information about it tonight.

This was discussed in #python, looks all good. Thanks Daniel for fixing this!