Bug 27948

(In reply to comment #0)
> Errata:
> 
> * tp_base_connection_add_client_interest C API for Location hasn't been added
> yet - it can be factored out from the D-Bus version

I've refactored to include this, and fixed some leaks and unnecessary subtleties.

  /* g_strdup (unique name) => gsize total count */
+  GHashTable *interested_clients;
+  /* GQuark iface => GHashTable {
+   *    unique name borrowed from interested_clients => gsize count } */
+  GHashTable *client_interests;

A comment explaining the relation between these 2 tables would be useful.

+static void tp_base_connection_interested_name_owner_changed_cb (
+    TpDBusDaemon *it, const gchar *unique_name, const gchar *new_owner,
+    gpointer user_data);

Should be one line per arg.

I'm not sure to properly understand code in _constructor iterating over types.

+#define SUPPORTED_INTERFACE "com.example.rannoch.ChineseHerbalMedicine"
I didn't get this reference ;)

(In reply to comment #2)
>   /* g_strdup (unique name) => gsize total count */
> +  GHashTable *interested_clients;
> +  /* GQuark iface => GHashTable {
> +   *    unique name borrowed from interested_clients => gsize count } */
> +  GHashTable *client_interests;
> 
> A comment explaining the relation between these 2 tables would be useful.

For your insta-reviewing convenience, here's the patch I added:

-  /* g_strdup (unique name) => gsize total count */
+  /* g_strdup (unique name) => gsize total count
+   *
+   * This is derivable from @client_interests: e.g. if
+   *    client_interests = { LOCATION => { ":1.23" => 5, ":1.42" => 2 },
+   *                         MAIL_NOTIFICATION => { ":1.23" => 1 } }
+   * then it implies
+   *    interested_clients = { ":1.23" => 6, ":1.42" => 2 }
+   */

> +static void tp_base_connection_interested_name_owner_changed_cb (
> +    TpDBusDaemon *it, const gchar *unique_name, const gchar *new_owner,
> +    gpointer user_data);
> 
> Should be one line per arg.

I'm still not convinced that doing this for declarations is, in fact, part of our coding style, but if it's how to get stuff through review... changed.

> I'm not sure to properly understand code in _constructor iterating over types.

Its effect is:

    foreach (interface @iter in client_interest_interfaces)
      {
        ensure that priv->client_interests contains an empty map for @iter
      }

However, because you can have a hierarchy, like this hypothetical situation:

    GObject
    \--- TpBaseConnection (c_i_i is assumed to be empty)
         \--- HazeConnection (c_i_i = [NICKNAMES])
              \--- HazeMSNConnection (c_i_i = [MAIL_NOTIFICATION])

we need to walk the hierarchy from HazeMSNConnection (inclusive) up to TpBaseConnection (exclusive), and iterate all of the current class's client_interest_interfaces (which I'll abbreviate c_i_i).

The alternative would be to require HazeMSNConnection to have c_i_i = [NICKNAMES, MAIL_NOTIFICATION], which isn't particularly onerous, but makes it more difficult to introduce a new interface that has a client interest in a superclass and make it work automagically in subclasses - the subclasses would either have to copy the parent's c_i_i manually, which is irritating, or make assumptions about the parent's c_i_i, which will break additions of functionality in superclasses.

I might at some point add a non-empty c_i_i to TpBaseConnection, if we come up with API that requires it on all connections (I've wondered about doing this as a solution to the handle-reffing problem).

If you don't fancy reviewing this bit but aren't particularly opposed to it, can I get Will or Sjoerd to have a look?

> +#define SUPPORTED_INTERFACE "com.example.rannoch.ChineseHerbalMedicine"
> I didn't get this reference ;)

en_GB in-joke - politicians whose other jobs/personal life/etc. could be influencing their political decisions are required to "declare an interest" (i.e. warn people about their potential bias). Since this Telepathy feature could also be called "declaring an interest", I searched the official record of Parliamentary debates for that phrase and used the first couple of interests I found.

I've also pushed new patches up to 1bc13ff to add a client-side, as I proposed in Bug #27835.

(In reply to comment #3)
> (In reply to comment #2)
> >   /* g_strdup (unique name) => gsize total count */
> > +  GHashTable *interested_clients;
> > +  /* GQuark iface => GHashTable {
> > +   *    unique name borrowed from interested_clients => gsize count } */
> > +  GHashTable *client_interests;
> > 
> > A comment explaining the relation between these 2 tables would be useful.
> 
> For your insta-reviewing convenience, here's the patch I added:
> 
> -  /* g_strdup (unique name) => gsize total count */
> +  /* g_strdup (unique name) => gsize total count
> +   *
> +   * This is derivable from @client_interests: e.g. if
> +   *    client_interests = { LOCATION => { ":1.23" => 5, ":1.42" => 2 },
> +   *                         MAIL_NOTIFICATION => { ":1.23" => 1 } }
> +   * then it implies
> +   *    interested_clients = { ":1.23" => 6, ":1.42" => 2 }
> +   */

Looks good!

> > +static void tp_base_connection_interested_name_owner_changed_cb (
> > +    TpDBusDaemon *it, const gchar *unique_name, const gchar *new_owner,
> > +    gpointer user_data);
> > 
> > Should be one line per arg.
> 
> I'm still not convinced that doing this for declarations is, in fact, part of
> our coding style, but if it's how to get stuff through review... changed.

I don't care that much tbh :)

> > I'm not sure to properly understand code in _constructor iterating over types.
> 
> Its effect is:
> 
>     foreach (interface @iter in client_interest_interfaces)
>       {
>         ensure that priv->client_interests contains an empty map for @iter
>       }
> 
> However, because you can have a hierarchy, like this hypothetical situation:
> 
>     GObject
>     \--- TpBaseConnection (c_i_i is assumed to be empty)
>          \--- HazeConnection (c_i_i = [NICKNAMES])
>               \--- HazeMSNConnection (c_i_i = [MAIL_NOTIFICATION])
> 
> we need to walk the hierarchy from HazeMSNConnection (inclusive) up to
> TpBaseConnection (exclusive), and iterate all of the current class's
> client_interest_interfaces (which I'll abbreviate c_i_i).
> 
> The alternative would be to require HazeMSNConnection to have c_i_i =
> [NICKNAMES, MAIL_NOTIFICATION], which isn't particularly onerous, but makes it
> more difficult to introduce a new interface that has a client interest in a
> superclass and make it work automagically in subclasses - the subclasses would
> either have to copy the parent's c_i_i manually, which is irritating, or make
> assumptions about the parent's c_i_i, which will break additions of
> functionality in superclasses.
> 
> I might at some point add a non-empty c_i_i to TpBaseConnection, if we come up
> with API that requires it on all connections (I've wondered about doing this as
> a solution to the handle-reffing problem).
> 
> If you don't fancy reviewing this bit but aren't particularly opposed to it,
> can I get Will or Sjoerd to have a look?

Yeah that would be good, to have at least the general idea reviewed.

> > +#define SUPPORTED_INTERFACE "com.example.rannoch.ChineseHerbalMedicine"
> > I didn't get this reference ;)
> 
> en_GB in-joke - politicians whose other jobs/personal life/etc. could be
> influencing their political decisions are required to "declare an interest"
> (i.e. warn people about their potential bias). Since this Telepathy feature
> could also be called "declaring an interest", I searched the official record of
> Parliamentary debates for that phrase and used the first couple of interests I
> found.

Wow, that was rather subtile :)

I'll review client side code.

+ * @self: a connection
Aren't we using "a #TpConnection" as convention now?

+  tp_connection_add_client_interest_by_id (test->conn,
+      TP_IFACE_QUARK_CONNECTION_INTERFACE_AVATARS);
This is just for the test right? Clients are not supposed to declare they are interested in avatars now, aren't they?

Looks good otherwise.

(In reply to comment #6)
> + * @self: a connection
> Aren't we using "a #TpConnection" as convention now?

IMO: you can if you want, but it's pointless to have as a convention, unless the argument is of a different C type. When the C type is correct, we don't need to hyperlink it again (gtkdoc will already get it right), but when the type is not what you'd expect in order to reduce casting, it's necessary to hyperlink it explicitly. So I think these usages are correct:

    /**
     * tp_proxy_get_stoat:
     * @self: a proxy
     * ...
     */
    Stoat *tp_proxy_get_stoat (TpProxy *self);

    /**
     * tp_proxy_get_badger:
     * @self: (type TelepathyGLib.Proxy): a #TpProxy
     * ...
     */
    Badger *tp_proxy_get_badger (gpointer self);

(In each case, gtk-doc will produce one hyperlink to TpProxy. In the latter case, g-i would ideally pick up Proxy.get_badger as a method, but it doesn't - I think danni filed a bug?)

> +  tp_connection_add_client_interest_by_id (test->conn,
> +      TP_IFACE_QUARK_CONNECTION_INTERFACE_AVATARS);
> This is just for the test right? Clients are not supposed to declare they are
> interested in avatars now, aren't they?

Correct. I'd have used Location and MailNotification (which both actually benefit from this feature), but MailNotification is still in draft.

As currently implemented (and, IMO, as should be spec'd when I revise the spec), it's always OK (albeit pointless) for a client to declare an interest in an arbitrary interface.

(In reply to comment #7)
> (In reply to comment #6)
> > + * @self: a connection
> > Aren't we using "a #TpConnection" as convention now?
> 
> IMO: you can if you want, but it's pointless to have as a convention, unless
> the argument is of a different C type. When the C type is correct, we don't
> need to hyperlink it again (gtkdoc will already get it right), but when the
> type is not what you'd expect in order to reduce casting, it's necessary to
> hyperlink it explicitly. So I think these usages are correct:

Fair enough.

Things that still need review are:

* the spec, on Bug #27835
* this GObject type-hierarchy voodoo:

(In reply to comment #3)
> (In reply to comment #2)
> > I'm not sure to properly understand code in _constructor iterating over types.
> 
> Its effect is:
> 
>     foreach (interface @iter in client_interest_interfaces)
>       {
>         ensure that priv->client_interests contains an empty map for @iter
>       }
> 
> However, because you can have a hierarchy, like this hypothetical situation:
> 
>     GObject
>     \--- TpBaseConnection (c_i_i is assumed to be empty)
>          \--- HazeConnection (c_i_i = [NICKNAMES])
>               \--- HazeMSNConnection (c_i_i = [MAIL_NOTIFICATION])
> 
> we need to walk the hierarchy from HazeMSNConnection (inclusive) up to
> TpBaseConnection (exclusive), and iterate all of the current class's
> client_interest_interfaces (which I'll abbreviate c_i_i).
> 
> The alternative would be to require HazeMSNConnection to have c_i_i =
> [NICKNAMES, MAIL_NOTIFICATION], which isn't particularly onerous, but makes it
> more difficult to introduce a new interface that has a client interest in a
> superclass and make it work automagically in subclasses - the subclasses would
> either have to copy the parent's c_i_i manually, which is irritating, or make
> assumptions about the parent's c_i_i, which will break additions of
> functionality in superclasses.
> 
> I might at some point add a non-empty c_i_i to TpBaseConnection, if we come up
> with API that requires it on all connections (I've wondered about doing this as
> a solution to the handle-reffing problem).

I've pushed a couple of additional patches to bring it up to date with the spec branch (just terminology changes).

Guillaume has reviewed up to and including commit 1bc13ff0d8fe1c8ea47c54ca3b616a16ab93e612, apart from the GObject type-hierarchy stuff noted above.

While looking at Gabble sidecars, I wondered if plugin/sidecars could use this features as they are not connection interfaces?

(In reply to comment #11)
> While looking at Gabble sidecars, I wondered if plugin/sidecars could use this
> features as they are not connection interfaces?

The branch now has 3 more commits than have been reviewed, the third of which would allow sidecars to attach more interests.

13:42 < smcv> cassidy: you wondered about client interests being added at 
              runtime. I've added API by which that can be done
13:44 < smcv> cassidy: I wonder whether to remove the stuff you didn't feel 
              qualified to review, and just say that each subclass must add all 
              its interest-tokens in its constructor() or constructed()?
13:44 < smcv> cassidy: (gabble plugins could gain the ability to be queried for 
              their interest-tokens, and GabbleConnection would add those too)

(In reply to comment #12)
> (In reply to comment #11)
> > While looking at Gabble sidecars, I wondered if plugin/sidecars could use this
> > features as they are not connection interfaces?
> 
> The branch now has 3 more commits than have been reviewed, the third of which
> would allow sidecars to attach more interests.

I've rebased those commits onto a merge from master (the test for this branch had bit-rotted somewhat - Travis re-namespaced all the test code), and added one which removes the class-hierarchy-walking stuff that Guillaume declined to review; now, connection subclasses just have to add their own tokens in constructed(), and they'll automatically trickle through the class hierarchy.

The GabbleConnection constructor could query plugins for the tokens in which they're interested, and push those into TpBaseConnection, if desired.

The top 4 commits of http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=shortlog;h=refs/heads/client-interests looks good to me.
Is there more to review?

(In reply to comment #14)
> Is there more to review?

Only the spec that makes this possible :-)

This needs updating to current telepathy-spec and merging, for the next release. I'll do that tomorrow.

Fixed in git for 0.13.3