Created attachment 166388 [details] [review]
this seems to work

This needs a test of some sort.   As far as the code goes it looks correct but is there a case where more than one vfuncs have the same name but you only want to override one of them?

Created attachment 166749 [details] [review]
fixed, with test

No longer breaks existing tests - the is_function_in_classes check in _setup_vfuncs needs to check against the class we're defining vfuncs for, not the one we're currently recursing through.

Added a test in TestPythonGObject, creating a subclass of GIMarshallingTests.SubObject and overriding a GIMarshallingTests.Object vfunc on it.

I'm not sure what should happen if there are multiple instances of the same vfunc name in the class hierarchy.  Maybe we should support implementing vfuncs with explicit class names, like do_Source_whatever, do_BrowserSource_whatever, and treat ambiguities as errors?  Are there any known cases of this that we can look at to see what makes sense?

Comment on attachment 166749 [details] [review]
fixed, with test

Looks good except I would assert if we encounter two or more virtual methods of the same name when trying to override that method.  If the results are unknown we should not allow it as it could be dangerous.

Created attachment 171244 [details] [review]
implement richcompare for GIBaseInfo

This allows us to compare GIBaseInfos for equality (and inequality) in python, which we need since it's OK to have multiple instances of the same vfunc, just not multiple different vfuncs with the same name.

I think I've got this mostly working now, with one major exception: The Gtk.Button override fails because we now try to set up vfuncs for Gtk.Activatable, which fails with:

TypeError: Class implementing Gtk.Activatable should implement the method do_update()

It doesn't have a problem with sync_action_properties on the same class, though.  I think it's finding the wrapper for gtk_activatable_sync_action_properties, which is just the vfunc accessor, so it shouldn't satisfy the requirement to implement the sync_action_properties method.

GtkButton implements both those methods, but I can't figure out how to check for that here.

This can't go in until the test passes

Looks like I'll need to fix bug 619606 before this is useful.  I've got some mostly-working code that checks method pointers to determine whether a vfunc has an implementation for a class, and with that instead of the current check, lots of things fail due to missing implementations GtkBuildable methods on gtk widget classes and so on.

Patch added to bug 619606 - think you can whip this bug into shape so we can add it to the release?

Sure, that shouldn't be a problem.

We're also missing annotations in g-i to make it possible to set those flags, but I've got most of a patch for that somewhere.

Annotation patch is on bug 633447.

Created attachment 173481 [details] [review]
updated, use vfunc info flags

This looks at the class/interface vtable to figure out if there's already an implementation, uses the vfunc flags to figure out if there needs to be an implementation, and checks other classes/interfaces to figure out if the method name is ambiguous.

The test for the last bit is commented out for now because it requires an addition to GIMarshallingTests - we need two interfaces with the same method name in there to check that ambiguous method overrides are detected.

I've been thinking about this a bit and I think for the overrides we should just not error out.  We are overriding an interface to create a new interface so the methods shouldn't have to be implemented.  When a user uses a class that inherits from the interface, then we should error out if they have not implemented all of the vfuncs.  We still need to know absolutely which need to be implemented.

I'm not sure I follow you - we should check that all required methods have been implemented at object construction time rather than class creation time?

With the current patch, if I have interface Namespace.Interface with virtual method 'invoke', this code, which seems like it should be OK:

class BaseObject(Namespace.Interface):
    def __init__(self):
        pass

class ActualObject(BaseObject):
    def do_invoke(self):
        whatever

won't work because the vfunc implementation check for BaseObject will fail.  If we deferred the check to the first time we created an instance of a class, it would only error out if I tried to create an instance of BaseObject.  Is that more or less what you're getting at?

What I was thinking is we don't error out if the object implementing the interface is within the "gi.overrides" module.  If the user is implementing an interface, we do error out on class creation time.

Sounds reasonable.

I guess to open that up a bit more, we could provide a way to mark a GType implemented in python as abstract, then check impl.__gtype__.is_abstract() before erroring out.

Created attachment 173939 [details] [review]
exclude gi.overrides module from vfunc requirements

Comment on attachment 171244 [details] [review]
implement richcompare for GIBaseInfo

committed this in preparation of committing the main patch after a bit more review.

Comment on attachment 173939 [details] [review]
exclude gi.overrides module from vfunc requirements

I committed this as two commits with fixups.  First I committed the supporting methods as they applied cleanly. I then refactored the types.py patch and fix a couple of things.  See next comment for details

* refactored def find_vfunc_in_other_classes(vfunc, base, classes) to be def find_vfunc_conflict_in_bases(vfunc, bases):
* base parameter not used in find_vfunc_conflict_in_bases so I removed it
* when calling find_vfunc_conflict_in_bases I send in the cls.__bases__ not impl.__bases__ because when recursing up the inheritance stack there is no need to check the children, the conflict would have already be caught
* removed the vfunc_info.must_override() check because that isn't hooked up in GI and will always return False

BTW the code is pretty confusing as it is.  I don't think we can make it much clearer so I accepted as is.  We should probably look to see if we can clean it up so that it is easier to understand what the code is doing.