After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 621092 - Add with_closures() variants for bindings
Add with_closures() variants for bindings
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2010-06-09 11:53 UTC by Tomeu Vizoso
Modified: 2010-06-18 23:30 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add g_bus_watch_proxy_with_closures() for bindings. (6.56 KB, patch)
2010-06-09 11:53 UTC, Tomeu Vizoso
none Details | Review
Add _with_closures alternative functions for those in GDBus that accept more than one callback. (23.88 KB, patch)
2010-06-09 16:44 UTC, Tomeu Vizoso
needs-work Details | Review
Add _with_closures alternative functions for those in GDBus that accept more than one callback. (25.93 KB, patch)
2010-06-10 09:40 UTC, Tomeu Vizoso
needs-work Details | Review
Add _with_closures alternative functions for those in GDBus that accept more than one callback. (29.45 KB, patch)
2010-06-10 12:26 UTC, Tomeu Vizoso
committed Details | Review
Sink closures (3.58 KB, patch)
2010-06-18 21:52 UTC, Jürg Billeter
committed Details | Review

Description Tomeu Vizoso 2010-06-09 11:53:15 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)
Comment 1 Tomeu Vizoso 2010-06-09 11:53:18 UTC
Created attachment 163192 [details] [review]
Add g_bus_watch_proxy_with_closures() for bindings.
Comment 2 David Zeuthen (not reading bugmail) 2010-06-09 14:16:14 UTC
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()
Comment 3 Tomeu Vizoso 2010-06-09 16:44:05 UTC
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
Comment 4 Christian Dywan 2010-06-10 05:07:19 UTC
Review of attachment 163212 [details] [review]:

You should g_return_val_if_fail (foo_closure != NULL, 0) for all closure arguments.
Comment 5 Tomeu Vizoso 2010-06-10 09:40:22 UTC
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
Comment 6 David Zeuthen (not reading bugmail) 2010-06-10 11:40:24 UTC
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.
Comment 7 David Zeuthen (not reading bugmail) 2010-06-10 11:43:29 UTC
Btw, you also need to add these new symbols to gio/gio.symbols...
Comment 8 Tomeu Vizoso 2010-06-10 12:26:56 UTC
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
Comment 9 Tomeu Vizoso 2010-06-10 12:28:28 UTC
(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.
Comment 10 David Zeuthen (not reading bugmail) 2010-06-10 15:41:46 UTC
(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!
Comment 11 Tomeu Vizoso 2010-06-10 16:32:28 UTC
Attachment 163284 [details] pushed as 45e604d - Add _with_closures alternative functions for those in GDBus that accept more than one callback.
Comment 12 Jürg Billeter 2010-06-18 21:52:13 UTC
Created attachment 164046 [details] [review]
Sink closures

GClosure uses floating references. The _with_closures functions should sink the floating reference.
Comment 13 David Zeuthen (not reading bugmail) 2010-06-18 23:30:14 UTC
(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