Created attachment 226914 [details] [review]
Move property install function into propertyhelper.py

A minor re-factoring which move _install_properties into
gi/_gobject/propertyhelper.py. Also fixes the same potential
problem seen with: https://bugzilla.gnome.org/show_bug.cgi?id=686496
As well as adds a handful of unittest and regression tests.

Created attachment 226915 [details] [review]
Move property install function into propertyhelper.py

A minor re-factoring which moves _install_properties into
gi/_gobject/propertyhelper.py. Also fixes the same potential
problem seen with: https://bugzilla.gnome.org/show_bug.cgi?id=686496
A handful of unittest and regression tests were added as well.

Review of attachment 226915 [details] [review]:

While I do not understand in detail the intricacies of the property decorator etc, the patch looks like a good and safe refactoring with the added bonus of unit tests.

My only suggestion would be to split the commit in two steps: first the change for the problem similar to bug 686496 and then the refactoring. If we ever have to bisect it would be very hard to spot the changed line in a large block moved from a file to another

Created attachment 226951 [details] [review]
Move property install function into propertyhelper.py

A minor re-factoring which moves _install_properties into
gi/_gobject/propertyhelper.py and dded a handful of unittests.

Created attachment 226952 [details] [review]
Change install_properties to not use getattr on classes

This changes an issue of using getattr to pull in a classes
__gproperties__ dictionary. The issue with this is it will it will
pull the class attribute off of base class if it is not defined
in the given class. Changed to use cls.__dict__.get('__gproperties__')
instead.

Thanks Paolo,

I agree, refactoring should not simultaneously include change in behavior. In this case the refactoring was needed to write good tests. I ended up breaking the refactoring out into a first patch which moves the code but keeps the same functionality and adds unittests. The second patch fixes the problem and updates the tests making it more obvious the behavior was changed.

Something else to note here is the issue we saw in https://bugzilla.gnome.org/show_bug.cgi?id=686496 is being masked in regards to properties do to type registration in C deleting __gproperties__ off of the type being registered (which also seems a bit odd). These patches ensure things will be correct regardless of what happens on the C side.

Comment on attachment 226952 [details] [review]
Change install_properties to not use getattr on classes

Hello Simon,

the patch looks good to me, but I have some nitpicks with the changelog:

>This changes an issue of using getattr to pull in a classes
>__gproperties__ dictionary.

This sentence doesn't explain much, I think you can just drop it.

> The issue with this is it will it will

Just one "it will". But simplifying this to "This will" is enough IMHO :-)

> pull the class attribute off of base class if it is not defined
>in the given class.

Can the changelog please explain why this is bad? Some pointers to bugs, etc.

> Changed to use cls.__dict__.get('__gproperties__') instead.

Please use the present tense consistently (just drop the "Changed to").

Comment on attachment 226951 [details] [review]
Move property install function into propertyhelper.py

Nice cleanup, thanks!

> A minor re-factoring which moves _install_properties into

"Move _install_properties() into"

> gi/_gobject/propertyhelper.py and dded a handful of unittests.

Missing "a", and please use present tense consistently, so "add".

Please go ahead with these changes, thanks!

Pushed with changelog fixes.