GNOME Bugzilla – Bug 621092
Add with_closures() variants for bindings
Last modified: 2010-06-18 23:30:29 UTC
Then introspection bindings can use like this: from gi.repository import GObject, Gio loop = GObject.MainLoop() def appeared_func(connection, name, name_owner, proxy): print 'appeared_func' loop.quit() def vanished_func(connection, name): print 'vanished_func', connection, name loop.quit() watcher = Gio.bus_watch_proxy(Gio.BusType.SESSION, 'org.freedesktop.PackageKit', Gio.BusNameWatcherFlags.AUTO_START, '/org/freedesktop/PackageKit', 'org.freedesktop.PackageKit.Query', Gio.DBusProxy.__gtype__, Gio.DBusProxyFlags.NONE, appeared_func, vanished_func) loop.run() Gio.bus_unwatch_proxy(watcher)
Created attachment 163192 [details] [review] Add g_bus_watch_proxy_with_closures() for bindings.
Patch generally looks OK. Some comments - Also want g_bus_watch_proxy_on_connection_with_closures() - The code would probably be easier to read if you renamed proxy_appeared_cb to watch_with_closures_on_proxy_appeared or something - I think the former name is too generic - Let's try and use this bug for all instances of this problem. I think we also want the same for - g_bus_watch_name() - g_bus_watch_name_on_connection() - g_bus_own_name() - g_bus_own_name_on_connection()
Created attachment 163212 [details] [review] Add _with_closures alternative functions for those in GDBus that accept more than one callback. g_bus_own_name_with_closures g_bus_own_name_on_connection_with_closures g_bus_watch_name_with_closures g_bus_watch_name_on_connection_with_closures g_bus_watch_proxy_with_closures g_bus_watch_proxy_on_connection_with_closures
Review of attachment 163212 [details] [review]: You should g_return_val_if_fail (foo_closure != NULL, 0) for all closure arguments.
Created attachment 163264 [details] [review] Add _with_closures alternative functions for those in GDBus that accept more than one callback. g_bus_own_name_with_closures g_bus_own_name_on_connection_with_closures g_bus_watch_name_with_closures g_bus_watch_name_on_connection_with_closures g_bus_watch_proxy_with_closures g_bus_watch_proxy_on_connection_with_closures
Review of attachment 163264 [details] [review]: Looks pretty good. I've added a bunch of style comments - should be easy to resolve. Thanks! ::: docs/reference/gio/gio-sections.txt @@ +2492,3 @@ g_bus_unwatch_proxy </SECTION> Since they are not of great interest to C programmers, it would probably be nicer if the _with_closures() variants came last - specifically after _unown and _unwatch. ::: gio/gdbusnameowning.c @@ +638,3 @@ +own_with_closures_on_bus_acquired (GDBusConnection *connection, + const gchar *name, + gpointer user_data) Please line up parameter names here. @@ +688,3 @@ +bus_own_name_free_func (gpointer user_data) +{ +typedef struct { Why isn't this static? @@ +733,3 @@ + GClosure *name_lost_closure) +{ + OwnNameData *user_data = g_new0 (OwnNameData, 1); Please split into separate lines - the style used in gdbus is that only SomeType *variable = SOME_TYPE (user_data); is OK (and it needs to be the first line). Also, the style in gdbus is to call the variable data, not user_data (because it isn't user_data until it is passed) so please rename it to data. @@ +735,3 @@ + OwnNameData *user_data = g_new0 (OwnNameData, 1); + + if (bus_acquired_closure != NULL) { Braces are wrong here (and elsewhere), should be if (ptr != NULL) { } @@ +737,3 @@ + g_closure_ref (bus_acquired_closure); + user_data->bus_acquired_closure = bus_acquired_closure; +typedef struct { Since g_closure_ref() returns the passed in GClosure, you can do this is one line (also elsewhere). That allows you to also drop the braces since it's only one statement. @@ +755,3 @@ + bus_acquired_closure ? own_with_closures_on_bus_acquired : NULL, + name_acquired_closure ? own_with_closures_on_name_acquired : NULL, + name_lost_closure ? own_with_closures_on_name_lost : NULL, Maybe easier to just use the values on user_data? @@ +806,3 @@ + user_data, + bus_own_name_free_func); +} Same comments as above. ::: gio/gdbusnamewatching.c @@ +703,3 @@ + + if (data->name_vanished_closure != NULL) + g_closure_unref (data->name_vanished_closure); Indentation is wrong here. @@ +735,3 @@ + GClosure *name_vanished_closure) +{ + WatchNameData *user_data = g_new0 (WatchNameData, 1); Please split into separate lines and s/user_data/data/. @@ +745,3 @@ + g_closure_ref (name_vanished_closure); + user_data->name_vanished_closure = name_vanished_closure; + } Style issues plus please collapse to one line. @@ +751,3 @@ + flags, + name_appeared_closure ? watch_with_closures_on_name_appeared : NULL, + name_vanished_closure ? watch_with_closures_on_name_vanished : NULL, Please use data->* to avoid the ternary operator. @@ +802,3 @@ + user_data, + bus_watch_name_free_func); +} Ditto. ::: gio/gdbusproxywatching.c @@ +566,3 @@ + bus_watch_proxy_free_func); +} + Same comments as for name watching / name owning. @@ +629,1 @@ Same comments as for name watching / name owning.
Btw, you also need to add these new symbols to gio/gio.symbols...
Created attachment 163284 [details] [review] Add _with_closures alternative functions for those in GDBus that accept more than one callback. g_bus_own_name_with_closures g_bus_own_name_on_connection_with_closures g_bus_watch_name_with_closures g_bus_watch_name_on_connection_with_closures g_bus_watch_proxy_with_closures g_bus_watch_proxy_on_connection_with_closures
(In reply to comment #6) > Review of attachment 163264 [details] [review]: > > @@ +755,3 @@ > + bus_acquired_closure ? own_with_closures_on_bus_acquired : NULL, > + name_acquired_closure ? own_with_closures_on_name_acquired : NULL, > + name_lost_closure ? own_with_closures_on_name_lost : NULL, > > Maybe easier to just use the values on user_data? > > @@ +751,3 @@ > + flags, > + name_appeared_closure ? watch_with_closures_on_name_appeared : NULL, > + name_vanished_closure ? watch_with_closures_on_name_vanished : NULL, > > Please use data->* to avoid the ternary operator. Sorry, don't see how that would work, I need to pass C callbacks but data contains closures.
(In reply to comment #9) > (In reply to comment #6) > > Review of attachment 163264 [details] [review] [details]: > > > > @@ +755,3 @@ > > + bus_acquired_closure ? own_with_closures_on_bus_acquired : NULL, > > + name_acquired_closure ? own_with_closures_on_name_acquired : NULL, > > + name_lost_closure ? own_with_closures_on_name_lost : NULL, > > > > Maybe easier to just use the values on user_data? > > > > @@ +751,3 @@ > > + flags, > > + name_appeared_closure ? watch_with_closures_on_name_appeared : NULL, > > + name_vanished_closure ? watch_with_closures_on_name_vanished : NULL, > > > > Please use data->* to avoid the ternary operator. > > Sorry, don't see how that would work, I need to pass C callbacks but data > contains closures. Ah, right. Sorry about that. But instead of func ? func : NULL please use func != NULL ? func : NULL, since func is a pointer, not a boolean. Please commit with that small change. Thanks!
Attachment 163284 [details] pushed as 45e604d - Add _with_closures alternative functions for those in GDBus that accept more than one callback.
Created attachment 164046 [details] [review] Sink closures GClosure uses floating references. The _with_closures functions should sink the floating reference.
(In reply to comment #12) > Created an attachment (id=164046) [details] [review] > Sink closures > > GClosure uses floating references. The _with_closures functions should sink the > floating reference. Nice catch. Committed with a s-o-b and bug reference. Thanks! http://git.gnome.org/browse/glib/commit/?id=1ed105b19b31bd5265de7d62156732b343c086e3