[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 #27 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2012-09-14 16:27:05 UTC ---
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 :/

+ <interface name="im.telepathy1.Connection.Interface.Names1">
+ <tp:requires interface="im.telepathy1.Connection"/>

Should be o.fd.T for master.

+ 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 :-)

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

Mismatch. Choose one.

+ * 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.

+#ifndef __TP_NAMES_MIXIN_H__
+#define __TP_NAMES_MIXIN_H__

You probably want to put single-include runes on this.

+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?

+ /* 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.

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

+ 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.)

+ 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)

+ * 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".)

+ /* 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.

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

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

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