[Bug 14540] Names interface - Aliasing replacement with separate nickname, local alias etc.

[bugzilla-daemon at freedesktop.org]

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

--- Comment #34 from Simon McVittie <simon.mcvittie at collabora.co.uk> ---
Review of xclaesse/names at 9a1c817c92.

General comments:

I would like to see an implementation of this specification for Gabble (a CM
where nicknames are server-stored and local aliases exist) and either Salut or
Rakia (a CM where they aren't, and they don't, respectively). The Salut/Rakia
case is the easier one. Ideally, I'd also like to see a MC implementation
(which shouldn't be too hard with my latest MC refactoring).

It's unclear at what point "local-alias" becomes something you can rely on, and
how you wait for it. In Gabble, it happens to be the point at which
ContactListState reaches SUCCESS.

Should Gabble cache the last known local-aliases, and make them available
before ContactListState is SUCCESS? (IMO: no, UIs that want that should use
Folks.)

Is local-alias going to be inextricably linked to ContactList on every
protocol? If it is, should we just document that? If not, should there be a
RequestLocalIDs method which returns when the CM's cache is up to date?

Open questions about caching, which I'm going to have to think about more I'm
afraid:

Is getting the attribute enough to trigger a refresh in general? (IMO: not in
XMPP at least; it's too expensive.)

Is getting the attribute enough to trigger an initial refresh if there is
nothing cached? (IMO: probably OK even in XMPP, as long as we have a cache.)

Who caches - telepathy-glib or the CM? I vaguely lean towards the CM because
Gabble (or eventually Wocky) could cache entire vCards (or
entire-vCard-except-PHOTO since avatars are separate?), not just the nickname,
which would give us "cheap" cached ContactInfo too. Also, if the CM does it,
I'd feel more comfortable about a sqlite dependency. I certainly don't want
"it's in sqlite at such-and-such a location" to be API!

One way in which Gabble could implement "don't make network requests as a
result of implicit_request_nickname, unless we don't know any nickname at all"
would be for it to push all cached nicknames into the mixin on startup; then 
implicit_request_nickname would only be called for contacts with no cached
nickname.

Another way would be for the C API presented to CMs to tell them how hard to
try, perhaps via a flags parameter: TP_NAMES_MIXIN_FLAG_ASK_SERVER for the
explicit request case, or 0 for the only-read-from-cache case.

------

> Add Names1 spec

> +++ b/spec/Connection_Interface_Aliasing.xml

These changes can go to spec master whenever, with a commit message mentioning
"in preparation for Names1".

> +++ b/spec/Connection_Interface_Names1.xml

> +  <tp:copyright>Copyright © 2009-2010 Collabora Ltd.</tp:copyright>
> +  <tp:copyright>Copyright © 2009 Nokia Corporation</tp:copyright>

I'm pretty sure we touched this file during 2012.

> +  <interface name="ofdT.Connection.Interface.Names1">

This is not valid D-Bus introspection. Spell it out in full here, please. The
ofdT shortcut is only OK in elements or attributes in the tp: namespace, IMO.

> +        <p>On connections managed by the <tp:dbus-ref
> +            namespace="imt1">AccountManager</tp:dbus-ref>,
> +          clients other than the account manager SHOULD set the <tp:dbus-ref
> +            namespace="imt1">Account.Nickname</tp:dbus-ref>

ofdT, not imt1.

-----

> Add TpNamesInterface and TpNamesMixin

>  <INCLUDE>telepathy-glib/telepathy-glib.h</INCLUDE>
> +<INCLUDE>telepathy-glib/telepathy-glib.h</INCLUDE>

Redundant

> + * To use the names mixin, include a #TpNamesMixin somewhere in your
> + * instance structure, and call tp_names_mixin_init() from your init function
> + * or constructor, and tp_names_mixin_finalize() from your finalize function.

I wonder whether this can be made (closer to being) introspectable?

tp_names_mixin_init() clearly can't be introspected as-is; but if we stored the
private struct (as opposed to its offset) as qdata, created it in init, and
relied on g_object_finalize to free it, then it wouldn't have to be.

We'd end up with something like this:

  /* init */
  {
    priv = g_slice_new0 (...);
    ...
    g_object_set_qdata_full (self, get_quark (), priv, priv_finalize);
  }

  /* everything else */
  {
    TpNamesMixinPrivate *priv = g_object_get_qdata (self, get_quark ());

    g_return_val_if_fail (priv != NULL, NULL);

    ... (priv->nicknames, ...);
  }

(I do recognize that this makes it inconsistent with our existing mixins.)

> + * Signature of a callback to be used to request nicknames. Implementation must
> + * use a #GSimpleAsyncResult and set the nickname as op_res_gpointer before
> + * completing it.

If true, this is harmful. I believe we now use a user-specified finish function
that makes this documentation untrue, though. (The same applies to the other
two async vfuncs.)

> + * Returns: The requested nickname, or %NULL if @error is set.

What transfer is this? If it's (transfer full) (which IIRC is the default), I'd
prefer it to be specified.

> + * It is not necessary (but not harmful either) to call
> + * tp_names_mixin_one_nickname_changed() with self Handle, the mixin will do it
> + * iself.

Typos: "with self Handle", "iself".

I would phrase this as:

    It is not necessary for the implementation to call t_n_m_o_n_c()
    for the self-handle - the mixin will do that itself.

> + * It is not necessary (but not harmful either) to call
> + * tp_names_mixin_one_local_alias_changed(), the mixin will do it iself.

Typo "iself".

> _finish
> * functions have a default implementation that assume the _async implementation
> * uses a #GSimpleAsyncResult and put result using
> * g_simple_async_result_set_op_res_gpointer().

I'd be tempted to omit these default implementations, while we see what happens
with GTask in GLib 2.35.

If we keep them, this comment is not strictly true. How about this?

    The @request_nickname_finish function has a default implementation,
    which assumes that @request_nickname_async uses a #GSimpleAsyncResult
    on which it will store the resulting string using
    g_simple_async_result_set_op_res_gpointer() before completion.

    The @set_nickname_finish and @set_local_alias functions have a default
    implementation, which assumes that the corresponding @async function
    uses a #GSimpleAsyncResult.

> +static gboolean
> +tp_names_mixin_set_dbus_property (GObject *object,

It's unfortunate that this can't raise a D-Bus error. I should open a bug about
introducing tp_dbus_properties_mixin_set_async() - Mission Control really wants
failable asynchronous property-setting, too.

> +void
> +tp_names_mixin_aliasing_iface_init (gpointer g_iface,
> +    gpointer iface_data)

This doesn't need the second argument, and the first argument might as well
have type TpSvcConnectionInterfaceAliasingClass to be self-documenting.
G_IMPLEMENT_INTERFACE will cast it anyway.

> + * tp_names_mixin_local_aliases_changed: (skip)
...
> + * If more than one nickname changed at once, it is recommended to use
> + * tp_names_mixin_local_aliases_changed() instead.

That second part is attached to the wrong function.

This function, and its nickname counterpart, could in fact be introspectable:
they just need (element-type guint utf8) or something.

-----

> TpContact: Add support for Names interface

On CMs from the distant future, supporting Names1 but not Aliasing, get_alias()
will now always return the identifier.

I think get_alias() should return legacy alias or local-alias or nickname or
identifier (whichever is the first one that's set - identifier always is), and
the property getter should call get_alias().

This has slightly awkward semantics for notification: a change to the
local-alias should notify("alias") if alias is unset, and a change to the
nickname should notify("alias") if both alias and local-alias are unset.

contact_maybe_set_alias, contact_maybe_set_nickname and
contact_maybe_set_local_alias should all be a no-op if the string parameter is
NULL, so that it's safe for tp_contact_set_attributes() to call them.

contacts_bind_to_aliases_changed should be renamed to bind_to_names_changed or
something, since it is no longer specific to AliasesChanged?

-- 
You are receiving this mail because:
You are the QA Contact for the bug.