Created attachment 236418 [details] [review]
GTK overrides: Make connect_signals handle tuple

This is used for passing passing extra arguments to a callback.
pygtk2 allowed this so we should at least do it for back compat.

ping

Review of attachment 236418 [details] [review]:

Since gtk_builder_connect_signals is so specific to C programming (and not useful for Python) it makes sense to override this to work well for Python programmers. A couple things:

* docs: It would be nice to have a pydoc describing the addition (or even the whole function).
* unittests: It would be great to have tests for different variations of the added functionality. As a starting point, there are existing builder tests: pygobject/tests/test_overrides_gtk.py : TestGtk.test_builder. I recommend breaking down "_full_callback" to make it more testable. i.e. A new utility function which extracts the handler and args given the obj_or_map and handler_name args. This will make testing easier because you won't have to mess with actually connecting signals and can test the behaviour of how the obj_or_map arg is supposed to work in isolation. For example:

handler, args = _extract_handler_and_args(obj_or_map, handler_name)

Basically the entirety of what _full_callback does with obj_or_map would go in here (including the error conditions which should also be tested). Note: to make this accessible to the unittest you might need to do something like add it as a static method to the Builder class.

A workaround (if you don't already do something like this):

def _builder_callback(builder, gobj, signal_name, handler_name, connect_obj, flags, signal_map):
    handler, *args = signal_map[handler_name]  # Python3 only
    gobj.connect(signal_name, handler, *args)

builder.connect_signals_full(_builder_callback, signal_map)

::: gi/overrides/Gtk.py
@@ +360,3 @@
 
+            args = ()
+            if type(handler) is tuple:

Use isinstance(handler, collections.Sequence) here

Note: We are currently in a hard code freeze for 3.8 so I'm adding this to the 3.10 milestone.

Hi Simon, thanks for the review.

(In reply to comment #3)
> Review of attachment 236418 [details] [review]:
> 
> Since gtk_builder_connect_signals is so specific to C programming (and not
> useful for Python) it makes sense to override this to work well for Python
> programmers. A couple things:
> 
> * docs: It would be nice to have a pydoc describing the addition (or even the
> whole function).
> * unittests: It would be great to have tests for different variations of the
> added functionality. As a starting point, there are existing builder tests:
> pygobject/tests/test_overrides_gtk.py : TestGtk.test_builder. I recommend
> breaking down "_full_callback" to make it more testable. i.e. A new utility
> function which extracts the handler and args given the obj_or_map and
> handler_name args. This will make testing easier because you won't have to mess
> with actually connecting signals and can test the behaviour of how the
> obj_or_map arg is supposed to work in isolation. For example:
> 
> handler, args = _extract_handler_and_args(obj_or_map, handler_name)
> 
> Basically the entirety of what _full_callback does with obj_or_map would go in
> here (including the error conditions which should also be tested). Note: to
> make this accessible to the unittest you might need to do something like add it
> as a static method to the Builder class.
> 

Thanks for the detailed info. Unfortunately doing the above will likely exceed the time I can allocate to this right now.

I'll post a v2 with the collections.Sequence bit you mention. If anyone wants to run with the docs + tests bit, please do, otherwise I'll try and get to it when I have more time.

> A workaround (if you don't already do something like this):
> 
> def _builder_callback(builder, gobj, signal_name, handler_name, connect_obj,
> flags, signal_map):
>     handler, *args = signal_map[handler_name]  # Python3 only
>     gobj.connect(signal_name, handler, *args)
> 
> builder.connect_signals_full(_builder_callback, signal_map)
> 

Thanks for the hint. I already reworked my code to not need this, but that's nice in the general case.

> ::: gi/overrides/Gtk.py
> @@ +360,3 @@
> 
> +            args = ()
> +            if type(handler) is tuple:
> 
> Use isinstance(handler, collections.Sequence) here

Thanks, will do.

(In reply to comment #4)
> Note: We are currently in a hard code freeze for 3.8 so I'm adding this to the
> 3.10 milestone.

No prob.

Created attachment 239817 [details] [review]
GTK overrides: Make connect_signals handle tuple (v2)

This is used for passing passing extra arguments to a callback.
pygtk2 allowed this so we should at least do it for back compat.

v2: Check against collections.Sequence, not raw tuple

Pushed with additional refactoring, tests and docs described in comment #3

The following fixes have been pushed:

Created attachment 248303 [details] [review]
GTK overrides: Make connect_signals handle tuple

This is used for passing extra arguments to callbacks during
signal emission in the form of:
builder.connect_signals({'on_clicked': (on_clicked, arg1, arg2)})

Co-Authored-By: Simon Feltman <sfeltman@src.gnome.org>