Hey, thanks for the patch, I just read through it and it generally looks good.

However, I don't think we want to deprecate anything - the 99% case is when using a bus connection so supporting the 1% case should not make the 99% case more difficult.

IOW, I think what we want is

 - docs for g_dbus_interface_skeleton_export() should say that
   it can be called multiple times but the object path must be
   the same (return G_IO_ERROR_INVALID_ARGUMENT if it isn't)

 - new method: GList *g_dbus_interface_skeleton_get_connections()

 - docs for g_dbus_interface_skeleton_get_connection() to say
   that the GDBusConnection from the first GDBusConnection
   is returned

 - docs for g_dbus_interface_skeleton_unexport() should say that
   it unexports on all the connections

 - new method: g_dbus_interface_skeleton_unexport_from_connection() 

Also

 - GDBus code is explicit when testing pointers, e.g.
   +  while (interface->priv->connections)
   needs to be
   +  while (interface->priv->connections != NULL)
   (this is in other places too)

 - if we are adding this functionality to GDBusInterfaceSkeleton
   we should also add it to GDBusObjectManagerServer - for this
   I suggest

   - deprecating
     - g_dbus_object_manager_server_get_connection()
     - g_dbus_object_manager_server_set_connection()

   - adding
     - g_dbus_object_manager_server_get_connections()
     - g_dbus_object_manager_server_add_connection()
     - g_dbus_object_manager_server_remove_connection()

 - need a test case for all this - probably in gio/tests/gdbus-peer.c
   - this should use GDBusObjectManagerServer and the existing
     generated types - this will also make the code relatively
     small

 - Not sure we need an example like gdbus-example-codegen-peer.c
   since peer-to-peer connections are relatively rare ... and
   the test case will already serve as a good test case

Finally I'm curious what you want to use this for (if you can disclose it please do - otherwise no problem).

Created attachment 200075 [details] [review]
Patch to allow GDBusInterfaceSkeleton to export itself on multiple GDBusConnections [take 2]

Ok I started by handling all of the api changes for GDBusInterfaceSkeleton.

Then I spent an embarrassingly long time trying to accomplish the same
with GDBusObjectManagerServer.

What I ended up with is a modified version of the first patch with
you API changes included and an added test to gdbus-peer.c.

The first challenge was that the object manager server does not seem
to emit the InterfacesAdded signal every time a new connection
is set (I had to adjust the code so that was done, this cost me
a few hours...).

And then when I finally put it all together in the test case,
after debugging and figuring why it doesn't work... I found
that GDBusObjectManagerClient is extremely 'bus' centric, it
wont go and query for the objects on the connection until there
is a "name-owner" that appears on it's internal proxy.

not an insurmountable problem, but much code to re-write in
GDBusObjectManagerClient I think to make it happen...

I do have a patch sitting on the side for GDBusObjectManagerServer
but it seems pointless unless you have a good full day or two to
sort out GDBusObjectManagerClient as well.

Personally I'm very satisfied with being able to use these
generated skeletons to handle my incoming connections (and I
unfortunately can't afford to go off on a tangent here anymore
than I already have).

Review of attachment 200075 [details] [review]:

Generally looks good even though there's a ton of comments from me about stylistic things and random unrelated changes (such as whitespace and formatting). Please don't take the comments the wrong way - I just really like code to follow the existing style and I want the patch itself to be easy to read and minimal.

We can wait with GDBusObjectManagerServer/GDBusObjectManagerClient support for now.

::: gio/gdbus-2.0/codegen/codegen.py
@@ +2245,3 @@
                      '  %sSkeleton *skeleton = %s%s_SKELETON (_skeleton);\n'
                      %(i.name_lower, i.camel_name, i.ns_upper, i.name_upper))
+

Please clean up the patch so it doesn't add extra whitespace.

@@ +2258,3 @@
                      '        {\n'
                      '          GVariant *value;\n'
+                     '          value = _%s_skeleton_handle_get_property (NULL, NULL, g_dbus_interface_skeleton_get_object_path (G_DBUS_INTERFACE_SKELETON (skeleton)), "%s", info->name, NULL, skeleton);\n'

Why pass NULL instead of "the first" GDBusConnection?

@@ +2312,3 @@
+                         '  GSList     *connections, *l;\n'
+                         '  connections = g_dbus_interface_skeleton_get_connections (G_DBUS_INTERFACE_SKELETON (skeleton));\n'
+                         '  for (l = connections; l; l = l->next)\n'

As I said in comment 1, need explicit "l != NULL" tests

@@ +2318,3 @@
+            self.c.write('      g_dbus_connection_emit_signal (connection,\n'
+                         '        NULL, g_dbus_interface_skeleton_get_object_path (G_DBUS_INTERFACE_SKELETON (skeleton)), "%s", "%s",\n'
+                         '        g_variant_new ("('

As I said on IRC, it's wasteful to construct a new GVariant every time. Instead just do it like this

 value = g_variant_ref_sink (g_variant_new (..));
 for (..)
   connection_emit_signal (.., value, ..);
 g_variant_unref (value);

which works because g_variant_new() always returns a floating variant.

@@ +2321,2 @@
                          %(i.name, s.name))
+

Extra whitespace.

@@ +2396,3 @@
                          '\n'
                          '  g_mutex_lock (&skeleton->priv->lock);\n'
+                         '\n'

Extra whitespace.

@@ +2420,3 @@
+                         '      signal_variant = g_variant_new ("(sa{sv}as)", "%s",\n'
+                         '                                      &builder, &invalidated_builder);\n'
+                         '      g_variant_ref_sink (signal_variant);'

Suggest to combine both lines into one e.g. g_variant_ref_sink (g_variant_new ())

@@ +2435,3 @@
+                         '      g_variant_unref (signal_variant);\n'
+                         '      g_slist_foreach (connections, (GFunc)g_object_unref, NULL);\n'
+                         '          GDBusConnection *connection = list->data;\n'

Ah, like you are already doing here.

@@ +2448,3 @@
             self.c.write('  skeleton->priv->changed_properties_idle_source = NULL;\n')
             self.c.write('  g_mutex_unlock (&skeleton->priv->lock);\n')
+                         

Extra whitespace.

@@ +2481,3 @@
+                         '    {\n'
+                         '      g_value_copy (orig_value, &cp->orig_value);\n'
+                         '    }\n'

Why this?

@@ +2522,3 @@
                          '  if (!_g_value_equal (value, &skeleton->priv->properties->values[prop_id - 1]))\n'
                          '    {\n'
+                         '      if (g_dbus_interface_skeleton_get_connection (G_DBUS_INTERFACE_SKELETON (skeleton)))\n'

why remove the '!= NULL'?

::: gio/gdbusinterfaceskeleton.c
@@ +55,3 @@
 };
 
+typedef struct {

curly brace goes on separate line

@@ +100,1 @@
+  /* Do we need to hold the lock here ?? */

We don't need to hold the lock, no, but it doesn't hurt, in fact, it's good to do in case some of the functions called verifies that the lock is held. Suggest to change it to

 /* Take the lock in case code we calls validates that we hold the lock /*

@@ -85,1 @@
-  g_assert (interface->priv->connection == NULL);

Why remove this?

@@ +118,3 @@
   if (interface->priv->object != NULL)
+    g_object_remove_weak_pointer (G_OBJECT (interface->priv->object),
+				  (gpointer *) &interface->priv->object);

This change looks like noise

@@ +267,3 @@
+				 G_TYPE_DBUS_INTERFACE_SKELETON,
+				 GDBusInterfaceSkeletonPrivate);
+

This one too looks like noise

@@ +668,3 @@
+free_connection (ConnectionData *data)
+{
+  data->connection      = g_object_ref (connection);

missing "!= NULL"

@@ +680,3 @@
+		       GError                **error)
+{
+static void

Don't do this, use interface_->priv directly

@@ +684,3 @@
+  guint                          registration_id;
+
+{

use "== NULL", it's not a boolean

@@ +693,3 @@
+	g_memdup (g_dbus_interface_skeleton_get_vtable (interface_), sizeof (GDBusInterfaceVTable));
+      priv->hooked_vtable->method_call = skeleton_intercept_handle_method_call;
+    }

OK, so we now allocate hooked_vtable the first time we use it and then we keep it around until finalized... Suggest to add a comment for this. Also, avoid splitting the first line in two...

@@ +723,3 @@
+  GDBusInterfaceSkeletonPrivate *priv;
+  ConnectionData                *data;
+       * ::g-authorize-method and for dispatching in thread vs

I generally prefer using the single-letter variables for (list) iterators (such as l for lists and n for scalar types).

@@ +728,3 @@
+
+  /* Get the connection in the list and unregister ... */
+       */

s/list/list != NULL/

@@ -627,3 @@
  * not exported anywhere. Do not free, the object belongs to @interface_.
- *
- * Since: 2.30

Why remove this?

@@ +775,3 @@
 {
+  ConnectionData  *data;
+  GDBusConnection *connection;

I prefer using the name 'ret' for the variable that the function returns - please don't change this to connection. Thanks.

@@ +779,2 @@
   g_return_val_if_fail (G_IS_DBUS_INTERFACE_SKELETON (interface_), NULL);
+

extra whitespace

@@ +782,3 @@
+
+  data = interface_->priv->connections->data;
+  if (data)

missing != NULL

@@ +817,3 @@
+  connections = NULL;
+
+ *

missing != NULL

@@ +821,3 @@
+      data        = list->data;
+      connections = g_slist_prepend (connections,
+ *   A newly allocated #GSList of #GDBusConnections all of wich

No need to state this (it's already stated in the docs for the function)

@@ +847,3 @@
+{
+  GSList *list;
+

initialize to FALSE here instead of in the for() loop

@@ +853,3 @@
+
+  for (ret = FALSE, list = interface_->priv->connections;
+

Please use !ret instead of 'ret == FALSE'

@@ +860,3 @@
+      ret = (data->connection == connection);
+    }
+      connections = g_slist_prepend (connections,

I think it would be nicer like this

 for (l = interface_->priv->connections; l != NULL; l = l->next)
   {
     ConnectionData *data = l->data;
     if (data->connection == connection)
       {
         ret = TRUE;
         goto out;
       }
   }

where out is just before unlocking the mutex.

@@ +869,3 @@
  * @interface_: A #GDBusInterfaceSkeleton.
  *
+ * Gets the object path that @interface_ is configured to export itself on.

why remove ", if any"? I mean, it still could be that interface_ is not yet exported.

@@ +933,2 @@
   g_mutex_unlock (&interface_->priv->lock);
+

extra whitespace

@@ -746,3 @@
 {
   g_return_if_fail (G_IS_DBUS_INTERFACE_SKELETON (interface_));
-  g_return_if_fail (interface_->priv->registration_id > 0);

Why remove this check? Instead I think we need

 g_return_if_fail (interface_->priv->connections != NULL);

@@ +953,3 @@
   g_mutex_lock (&interface_->priv->lock);
 
+  /* Remove the connection */

Should probably say "Remove all connections"

@@ -750,3 @@
   g_mutex_lock (&interface_->priv->lock);
 
-  g_assert (interface_->priv->connection != NULL);

why remove this assert?

@@ -752,3 @@
-  g_assert (interface_->priv->connection != NULL);
-  g_assert (interface_->priv->object_path != NULL);
-  g_assert (interface_->priv->hooked_vtable != NULL);

Why remove the asserts?

::: gio/gdbusinterfaceskeleton.h
@@ +107,2 @@
 GDBusConnection             *g_dbus_interface_skeleton_get_connection  (GDBusInterfaceSkeleton      *interface_);
+GSList                      *g_dbus_interface_skeleton_get_connections (GDBusInterfaceSkeleton      *interface_);

Use GList, not GSList.

::: gio/tests/gdbus-peer.c
@@ +1707,3 @@
+
+  example_animal_call_poke_sync (animal1, TRUE, FALSE, NULL, &error);
+

Should check here that both animal1 and animal2 are sad now ... but for this you need to be sure that the generated PropertiesChanged signal have been delivered. A nice way to ensure this is simply by pinging the object.. like this

  error = NULL;
  value = g_dbus_proxy_call_sync (G_DBUS_PROXY (animal1),
                                  "org.freedesktop.DBus.Peer.Ping",
                                  NULL, G_DBUS_CALL_FLAGS_NONE, -1,
                                  NULL, &error);
  g_assert_no_error (error);
  g_assert (value != NULL);
  g_variant_unref (value);
  g_assert_cmpstr (animal_get_mood (animal1), ==, "Sad");

and ditto for animal2.

@@ +1710,3 @@
+
+  example_animal_call_poke_sync (animal2, FALSE, TRUE, NULL, &error);
+  /* Handle Poke() D-Bus method invocations on the .Animal interface */

and here check that both animals are happy

Created attachment 200147 [details] [review]
Patch to allow GDBusInterfaceSkeleton to export itself on multiple GDBusConnections [take 3]

Ok I think this new patch covers all of your comments.

I replied to them inline while applying the changes to the patch:

(In reply to comment #3)
> Review of attachment 200075 [details] [review]:
> 
> Generally looks good even though there's a ton of comments from me about
> stylistic things and random unrelated changes (such as whitespace and
> formatting). Please don't take the comments the wrong way - I just really like
> code to follow the existing style and I want the patch itself to be easy to
> read and minimal.
> 
> We can wait with GDBusObjectManagerServer/GDBusObjectManagerClient support for
> now.
> 
> ::: gio/gdbus-2.0/codegen/codegen.py
> @@ +2245,3 @@
>                       '  %sSkeleton *skeleton = %s%s_SKELETON (_skeleton);\n'
>                       %(i.name_lower, i.camel_name, i.ns_upper, i.name_upper))
> +
> 
> Please clean up the patch so it doesn't add extra whitespace.

Got it.

> @@ +2258,3 @@
>                       '        {\n'
>                       '          GVariant *value;\n'
> +                     '          value = _%s_skeleton_handle_get_property
> (NULL, NULL, g_dbus_interface_skeleton_get_object_path
> (G_DBUS_INTERFACE_SKELETON (skeleton)), "%s", info->name, NULL, skeleton);\n'
> 
> Why pass NULL instead of "the first" GDBusConnection?

I thought it needless to pass in the first GDBusConnection as it's
unused and probably a bad idea to use, but I'm removing that diff
to keep the patch shorter as it's probably doesnt really change anything...

> 
> @@ +2312,3 @@
> +                         '  GSList     *connections, *l;\n'
> +                         '  connections =
> g_dbus_interface_skeleton_get_connections (G_DBUS_INTERFACE_SKELETON
> (skeleton));\n'
> +                         '  for (l = connections; l; l = l->next)\n'
> 
> As I said in comment 1, need explicit "l != NULL" tests

Ok I tried to use 'l' instead of 'list' and use '!= NULL' checks for
pointers and '!ret' checks for booleans, if I understand that's what you
want... I hope I have it the way you like this time...

> 
> @@ +2318,3 @@
> +            self.c.write('      g_dbus_connection_emit_signal (connection,\n'
> +                         '        NULL,
> g_dbus_interface_skeleton_get_object_path (G_DBUS_INTERFACE_SKELETON
> (skeleton)), "%s", "%s",\n'
> +                         '        g_variant_new ("('
> 
> As I said on IRC, it's wasteful to construct a new GVariant every time. Instead
> just do it like this
> 
>  value = g_variant_ref_sink (g_variant_new (..));
>  for (..)
>    connection_emit_signal (.., value, ..);
>  g_variant_unref (value);
> 
> which works because g_variant_new() always returns a floating variant.
> 

Got it, I thought I caught it the first time but as you can see
I only did it for one of the two signal emission loops...

> @@ +2481,3 @@
> +                         '    {\n'
> +                         '      g_value_copy (orig_value, &cp->orig_value);\n'
> +                         '    }\n'
> 
> Why this?

Ah, my bad mistake, I had that temporarily and forgot to remove it, gone.

> 
> @@ +2522,3 @@
>                           '  if (!_g_value_equal (value,
> &skeleton->priv->properties->values[prop_id - 1]))\n'
>                           '    {\n'
> +                         '      if (g_dbus_interface_skeleton_get_connection
> (G_DBUS_INTERFACE_SKELETON (skeleton)))\n'
> 
> why remove the '!= NULL'?

Fallout from my last patch when it became a boolean '_exported()' function
and the other one was deprecated, i.e. no good reason, putting it back
the original way.

> ::: gio/gdbusinterfaceskeleton.c
> @@ +55,3 @@
>  };
> 
> +typedef struct {
> 
> curly brace goes on separate line
> 

Done...

> @@ +100,1 @@
> +  /* Do we need to hold the lock here ?? */
> 
> We don't need to hold the lock, no, but it doesn't hurt, in fact, it's good to
> do in case some of the functions called verifies that the lock is held. Suggest
> to change it to
> 
>  /* Take the lock in case code we calls validates that we hold the lock /*
> 

Gotcha...

> @@ -85,1 @@
> -  g_assert (interface->priv->connection == NULL);
> 
> Why remove this?
> 

Because before the code did
  if (priv->connection) set_connection (NULL);
  assert (!connection)

Now it already does:
  while (connections != NULL)
    remove_connection ();

So the assert() becomes pointless after that loop
(in this particular case in finalize at least).

> @@ +118,3 @@
>    if (interface->priv->object != NULL)
> +    g_object_remove_weak_pointer (G_OBJECT (interface->priv->object),
> +                  (gpointer *) &interface->priv->object);
> 
> This change looks like noise
> 
> @@ +267,3 @@
> +                 G_TYPE_DBUS_INTERFACE_SKELETON,
> +                 GDBusInterfaceSkeletonPrivate);
> +
> 
> This one too looks like noise
> 

Removed noise...

> @@ +668,3 @@
> +free_connection (ConnectionData *data)
> +{
> +  data->connection      = g_object_ref (connection);
> 
> missing "!= NULL"
> 

Gotcha...

> @@ +680,3 @@
> +               GError                **error)
> +{
> +static void
> 
> Don't do this, use interface_->priv directly
> 

Ok, changed the source file entirely to that effect...

> @@ +693,3 @@
> +    g_memdup (g_dbus_interface_skeleton_get_vtable (interface_), sizeof
> (GDBusInterfaceVTable));
> +      priv->hooked_vtable->method_call =
> skeleton_intercept_handle_method_call;
> +    }
> 
> OK, so we now allocate hooked_vtable the first time we use it and then we keep
> it around until finalized... Suggest to add a comment for this. Also, avoid
> splitting the first line in two...

Alright, added comment saying that we need to wait for subclasses
to initialize in order to fetch a usable vtable (which is why we 
put it off instead of doing it right away in the instance initializer).

> @@ +728,3 @@
> +
> +  /* Get the connection in the list and unregister ... */
> +       */
> 
> s/list/list != NULL/
> 
> @@ -627,3 @@
>   * not exported anywhere. Do not free, the object belongs to @interface_.
> - *
> - * Since: 2.30
> 
> Why remove this?

More fallout, was deprecated in last patch and brought back 
without the since tag, fixed...

> 
> @@ +775,3 @@
>  {
> +  ConnectionData  *data;
> +  GDBusConnection *connection;
> 
> I prefer using the name 'ret' for the variable that the function returns -
> please don't change this to connection. Thanks.
> 

Got it, actually I also caught a possible null pointer dereference
I had in that function... fixed.

> @@ +821,3 @@
> +      data        = list->data;
> +      connections = g_slist_prepend (connections,
> + *   A newly allocated #GSList of #GDBusConnections all of wich
> 
> No need to state this (it's already stated in the docs for the function)
>

Got it... simplified comment...
 
> @@ +847,3 @@
> +{
> +  GSList *list;
> +
> 
> initialize to FALSE here instead of in the for() loop
> 
> @@ +853,3 @@
> +
> +  for (ret = FALSE, list = interface_->priv->connections;
> +
> 
> Please use !ret instead of 'ret == FALSE'
> 
> @@ +860,3 @@
> +      ret = (data->connection == connection);
> +    }
> +      connections = g_slist_prepend (connections,
> 
> I think it would be nicer like this
> 
>  for (l = interface_->priv->connections; l != NULL; l = l->next)
>    {
>      ConnectionData *data = l->data;
>      if (data->connection == connection)
>        {
>          ret = TRUE;
>          goto out;
>        }
>    }
> 
> where out is just before unlocking the mutex.
> 

Done.

> @@ +869,3 @@
>   * @interface_: A #GDBusInterfaceSkeleton.
>   *
> + * Gets the object path that @interface_ is configured to export itself on.
> 
> why remove ", if any"? I mean, it still could be that interface_ is not yet
> exported.

Put it back, again more fallout from my first patch..

> @@ -746,3 @@
>  {
>    g_return_if_fail (G_IS_DBUS_INTERFACE_SKELETON (interface_));
> -  g_return_if_fail (interface_->priv->registration_id > 0);
> 
> Why remove this check? Instead I think we need
> 
>  g_return_if_fail (interface_->priv->connections != NULL);

Done.

> 
> @@ +953,3 @@
>    g_mutex_lock (&interface_->priv->lock);
> 
> +  /* Remove the connection */
> 
> Should probably say "Remove all connections"

Done..

> 
> @@ -750,3 @@
>    g_mutex_lock (&interface_->priv->lock);
> 
> -  g_assert (interface_->priv->connection != NULL);
> 
> why remove this assert?
> 
> @@ -752,3 @@
> -  g_assert (interface_->priv->connection != NULL);
> -  g_assert (interface_->priv->object_path != NULL);
> -  g_assert (interface_->priv->hooked_vtable != NULL);
> 
> Why remove the asserts?

Well, now I added the:
   g_return_if_fail (interface_->priv->connections != NULL)

as you suggested, so the assert on ->connections is gone,
I put back the other 2 asserts in _unexport() (and replicated
that in _unexport_from_connection).

> 
> ::: gio/gdbusinterfaceskeleton.h
> @@ +107,2 @@
>  GDBusConnection             *g_dbus_interface_skeleton_get_connection 
> (GDBusInterfaceSkeleton      *interface_);
> +GSList                      *g_dbus_interface_skeleton_get_connections
> (GDBusInterfaceSkeleton      *interface_);
> 
> Use GList, not GSList.

Done.

> 
> ::: gio/tests/gdbus-peer.c
> @@ +1707,3 @@
> +
> +  example_animal_call_poke_sync (animal1, TRUE, FALSE, NULL, &error);
> +
> 
> Should check here that both animal1 and animal2 are sad now ... but for this
> you need to be sure that the generated PropertiesChanged signal have been
> delivered. A nice way to ensure this is simply by pinging the object.. like
> this
> 
>   error = NULL;
>   value = g_dbus_proxy_call_sync (G_DBUS_PROXY (animal1),
>                                   "org.freedesktop.DBus.Peer.Ping",
>                                   NULL, G_DBUS_CALL_FLAGS_NONE, -1,
>                                   NULL, &error);
>   g_assert_no_error (error);
>   g_assert (value != NULL);
>   g_variant_unref (value);
>   g_assert_cmpstr (animal_get_mood (animal1), ==, "Sad");
> 
> and ditto for animal2.
> 

Gotcha, this was a bit of a challenge to get the test working
but I got it.

The rules of course are
  a.) The skeleton must be created after pushing the default server thread context
  b.) After poking the animal from the proxy, the client thread needs to run a main
      loop to pick up property change notification messages before asserting
  c.) The server needs to also run the main loop to notify changes, so a message
      that causes the server to quit it's mainloop does not result in property
      change notifications on the proxy (even if the skeleton calls animal_set_mood())

I suppose this last part 'c.)' might be considered a bug by some, perhaps
pending signals should be flushed before destroying an interface skeleton
or the like, at any rate its unrelated to this patch...

Created attachment 200148 [details] [review]
Patch to make GDBusObjectManagerServer manage multiple connections

I'm posting this patch for reference as it did cost me some good effort and
should be useful.

This patch makes GDBusObjectManagerServer export it's objects over multiple
connections.

When applying this patch locally, one problem you will find when
trying to use GDBusObjectManagerClient without the 'name-owner' (i.e. no
bus, just a connection)... is that when creating the derived proxies
(i.e. ExampleAnimalProxy) the constructor method will assign "name" to
the proxy with a passed NULL value.

Going into gdbusproxy.c and fixing set_property() so that NULL is an
exceptable and ignorable value for the "name" property will get you
by this obstacle.

Next the only thing missing is to make the GDBusObjectManagerClient aware
that it can call GetObjects and the like on a GDBusConnection even if there
is no bus... 

Anyway, hope this patch might be helpful...

Hey, Tristan, sorry for the silence - been busy with other work. Just a headsup that I will be reviewing / merging the patch later this week. Thanks.

(In reply to comment #1)
[...]
> Finally I'm curious what you want to use this for (if you can disclose it
> please do - otherwise no problem).

Hi, 
   I'm not sure if I may have replied this on irc, anyway when you
asked the question I was not sure if the information was public or
not yet (and I had recently signed an NDA for this).

But I can now tell you that it's for a karaoke player application
which will display the karaoke visual both on the local 'player'
host (which is a TouchTunes Jukebox) and also on some remote displays
(which are driven by some networked devices which are also running
the same client program which can run on the local host).

TouchTunes has already demoed a prototype for this application
at a trade show and announced it's release for early 2012
(so at least this particular information is not a secret).

I've been trying to catch you on IRC but since I moved back to Seoul
Korea last week I think our time zones have went out of sync.

Can we please try to land this soon ? I'll be much more comfortable
if I can tell my boss that we don't depend on a downstream patch
for this player...

(In reply to comment #7)
> But I can now tell you that it's for a karaoke player application
> which will display the karaoke visual both on the local 'player'
> host (which is a TouchTunes Jukebox) and also on some remote displays
> (which are driven by some networked devices which are also running
> the same client program which can run on the local host).

Interesting.

> Can we please try to land this soon ? I'll be much more comfortable
> if I can tell my boss that we don't depend on a downstream patch
> for this player...

Certainly. Sorry for being slow, I've been very busy with another project. I'll review your patch this week, promise!

Review of attachment 200147 [details] [review]:

Committed with a bunch of small changes, mostly stylistic (comments, s/tab/space/, trailing whitespace), see

 http://git.gnome.org/browse/glib/commit/?id=a00530ecb0e8576e7a023f37a97528da4d0dfce5

In the future please use 'git format-patch' to generate patches. Thanks.

::: gio/gdbusinterfaceskeleton.c
@@ +722,3 @@
+
+  /* Get the connection in the list and unregister ... */
+       * We need to wait until subclasses have had time to initialize

Not really safe to use l->next since we may be deleting @l below. But note how we can (and should) exit the loop once matched because of the fact that connection will appear at most one time in the list.

@@ +751,3 @@
+    }
+  return FALSE;
+				       NULL, /* user_data_free_func */

No need to return a gboolean since none of the call-sites use it.

::: gio/tests/gdbus-peer.c
@@ +1732,3 @@
+  /* Give the proxies a chance to refresh in the defaul main loop */
+  g_timeout_add (100, codegen_quit_mainloop_timeout, NULL);
+					   test_guid,

Not at all thrilled with this "sleep" ... much better to wait for property changes. I guess we can fix it up later.

@@ +1753,3 @@
+  /* Give the proxies a chance to refresh in the defaul main loop */
+  g_timeout_add (1000, codegen_quit_mainloop_timeout, NULL);
+  service_loop = g_main_loop_new (service_context, FALSE);

Ditto.

Thank you for taking the time to do this David :)

Shall we keep this one open for the prospect of extending
this to GDBusObjectManagerServer & GDBusObjectManagerClient ?

If you prefer, I could open up a separate bug for that... 
(FWIW I still don't personally need that functionality)

(In reply to comment #10)
> If you prefer, I could open up a separate bug for that... 
> (FWIW I still don't personally need that functionality)

Let's just keep this bug open. Thanks.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/472.