[Bug 27948] TpBaseConnection: support AddClientInterest, RemoveClientInterest

[bugzilla-daemon at freedesktop.org]

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

--- Comment #5 from Guillaume Desmottes <guillaume.desmottes at collabora.co.uk> 2010-05-18 03:02:43 PDT ---
(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.

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