[Bug 27948] TpBaseConnection: support AddClientInterest, RemoveClientInterest

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=27948

--- Comment #3 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-05-17 09:10:38 PDT ---
(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.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.