[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 #28 from Xavier Claessens <xclaesse at gmail.com> 2012-09-15 15:14:44 UTC ---
(In reply to comment #27)
> 17:22 < smcv> just doing "conceptual" review so far, I'll check for memory 
>               leaks and stuff later
> 
> 17:17 < xclaesse> smcv, Hmm, facebook... so AliasStorage is known only after 
>                   CONNECTED
> 17:18 < smcv> yeah, that was my point
> 17:18 < smcv> it's unfortunate, I know
> 17:18 < xclaesse> I've made it immutable since mixin creation :/

fixed.

> + <interface name="im.telepathy1.Connection.Interface.Names1">
> + <tp:requires interface="im.telepathy1.Connection"/>
> 
> Should be o.fd.T for master.

Fixed.

> + User interfaces MAY also store a "pet name" locally; this is necessary
> + if they are to support protocols with no server-stored contact list,
> + like SIP.</p>
> 
> This is what you said in Comment #23. Great minds (in this case you, and my
> past self) think alike :-)

Still wondering how this will affect gabble. If I understand correctly, gabble
needs to fetch vCard to get the nickname, so it cache the value on the roster.
So the nickname becomes a local-alias for all contacts after first time the
account is connected using gabble?

With my mixin, each time NICKNAME attribute is asked, request_nickname_async()
is called, so that will do a vCard fetch all the time, right? How would a
on-disk cache in mixin avoid that? Should mixin avoid calling
request_nickname_async() if it already has a local-alias?

> + Connection_Interface_Names.xml \
> + <xi:include href="Connection_Interface_Names1.xml"/>
> 
> Mismatch. Choose one.

Fixed.

> + * 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.
> 
> Don't do that; require the implementor to supply their own finish function.
> 
> If you really strongly want the finish function to be optional, you can provide
> a default implementation that assumes the result is a GSimpleAsyncResult and
> its op_res_gpointer is the nickname, but you must still let the implementor
> provide their own finish function if they don't like that idea.
> 
> I would be inclined to say "keep it mandatory", tbh.

Ok, will do

> +#ifndef __TP_NAMES_MIXIN_H__
> +#define __TP_NAMES_MIXIN_H__
> 
> You probably want to put single-include runes on this.

done

> +typedef void (*TpNamesMixinRequestNicknameAsyncFunc) (GObject *object,
> 
> Why not TpBaseConnection? You require the implementor to use a TpBaseConnection
> already, afaics.
> 
> +/* Initialisation */
> +void tp_names_mixin_init (GObject *object,
> + glong offset,
> + TpNamesMixinRequestNicknameAsyncFunc request_nickname_async,
> + TpNamesMixinSetNicknameAsyncFunc set_nickname_async,
> + TpNamesMixinSetLocalAliasAsyncFunc set_local_alias_async,
> + TpContactMetadataStorageType alias_storage);
> 
> I would be tempted to require the TpBaseConnection to implement a GInterface
> (TpNamingConnection?) that has these as ordinary virtual methods?

Because other mixin I've copied does that. But I agree taking a
TpBaseConnection is nicer. Adding an iface (TpNamesInterface?) for the vmethods
and taking that iface in args would be even better :-)

Will do that.

> + /* Even if setting the nickname fails, we want to keep it for at least
> + * the session */
> + tp_names_mixin_nickname_changed (object,
> + tp_base_connection_get_self_handle (conn),
> + nickname);
> +
> + self->priv->set_nickname_async (object, nickname, NULL, NULL);
> 
> Shouldn't set_nickname_async() be nullable? It looks as though it'd be pretty
> easy.

We would need a way to tell we don't support setting user's nickname, a
property similar to AliasStorage?

> I feel as though there ought to be some indication that setting it failed -
> possibly a signal?

I could add the machinary to make async implementations of property setter.
It's possible to return an error that way, no?

> + if (!tp_strdiff (old_local_alias, alias))
> + {
> + tp_svc_connection_interface_names_return_from_set_local_alias (context);
> + return;
> + }
> 
> I would prefer not to second-guess the UI. If the user types something and
> clicks "change", we should make a round-trip to the server to force it to be
> stored, even if we think we know better. (The user might be undoing something
> they did on another connected device, which hasn't reached us yet.)

fixed.

> + self->priv->request_nickname_async (object, contact, NULL, NULL);
> 
> Shouldn't this be nullable? (There is at least one protocol where we have
> aliases but no ability to download nicknames, namely SIP)

done.

> + * tp_names_mixin_nickname_changed: (skip)
> 
> Are you expected to call this before request_nickname_async completes?
> (Implementation says "yes".)
> 
> Are you expected to call this before starting the real work of
> set_nickname_async? (Implementation says "no".)
> 
> Are you expected to call this before set_nickname_async completes?
> (Implementation says "only if what you got differs from what you asked for,
> e.g. different Unicode normalization".)

I've changed the mixin to be smart and call it itself. Updated doc to be
explicit.

> + /* FIXME: Could wait to aggregate signals */
> + table = g_hash_table_new (NULL, NULL);
> 
> I would prefer the CM to do aggregation and call a "plural" things-changed
> method, rather than having telepathy-glib run some sort of timer.

Right, I should rename them to tp_names_mixin_one_nickname_changed() and
tp_names_mixin_one_local_alias_changed() and add plurials. Btw, I should
probably do that in TpAvatarsMixin as well...

> On XMPP, we probably want to disallow setting local-alias on MUC contacts. Is
> that something you've allowed for?

It is allowed and local alias will be kept in cache for the session life time,
or until CM calls tp_names_mixin_drop(). It could even go into the disk cache
once that's implemented. Do you disagree?

> I haven't reviewed "Implement Aliasing iface as well", TpContact or tests yet.

You now have tones if !fixup stacked on the branch :D

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