Bug 14540 (Conn.I.Names)

The distinction being requested is between "petnames" and "nicknames" as defined by <http://www.skyhunter.com/marcs/petnames/IntroPetNames.html>, although we'd rather not have the name Petnames appear in our API.

This is also required if we want to follow http://www.xmpp.org/extensions/xep-0165.html "Best Practices to Discourage JID Mimicking", or its (possibly unwritten) equivalent on other protocols.

source   |my name for them   |  their name for themselves
---------+-------------------+---------------------------
skyhunter| petname           | nickname
XEP-0165 | handle, petname   | nickname
Pidgin   | private alias     | public alias

Other possibilities for the "my name for them" column include "friendly name" (suggested by Rob) or "appointed alias" (suggested in #pidgin).

"Friendly Name" is MSN-speak for "their name for themselves", so using it for "my name for them" seems unwise.

There seems to be consensus that a Nicknames interface is needed; we should just do it.

The other half of Aliasing (pet names) only makes sense on connections where you *can* set a name for someone, i.e. connections with a roster or contact list, so it's Bug #21787.

Don't know if it should be considered here as well, but we are bitten by the demand for an alias to always be non-empty and fall back to the handle ID string.
If it's the alias for self handle, the client would take it and set it back (I don't know why they must do it, but they can anyway). Without a special heuristic which may produce unexpected results, this non-alias may end up clobbering the real display name which was slow to sync up from server-stored data.

(In reply to comment #4)
> Don't know if it should be considered here as well, but we are bitten by the
> demand for an alias to always be non-empty and fall back to the handle ID
> string.

In my current Nicknames spec draft, which I'll attach shortly, the Nickname is the empty string if the service doesn't tell us anything better, and the *UI* is responsible for falling back to the identifier if it wants to (in practice it does). Does that solve your problem?

(I plan to do the same thing for "pet names" in the ContactList interface)

http://people.freedesktop.org/~smcv/telepathy-spec-nicknames/spec/org.freedesktop.Telepathy.Connection.Interface.Nicknames.DRAFT.html

I've included this in the branch for Bug #21787 (ContactList), since replacing Aliasing on all protocols requires both interfaces.

Updated branch, which depends on the one from Bug #20774:

http://people.freedesktop.org/~smcv/telepathy-spec-names/spec/org.freedesktop.Telepathy.Connection.Interface.Names.DRAFT.html

(Sorry Youness, added you to Cc for clarification about friendly names, but then decided our current mapping of them seemed best anyway...)

(In reply to comment #8)
> Updated branch, which depends on the one from Bug #20774:

No, it depends on Bug #21787. Gah.

No worries Simon, I'd like to give my opinion and explain how it works with MSN anyways, so thanks for CC-ing me.
So, with MSN you have a nickname (the one the contact sets), which you can override (The official client just puts an 'annotation' on the contact's data called 'AB.Nickname' and the UI uses it to replace the contact's nickname).

You can also have a 'friendly name' which is a bit different, it's the nickname to be shown for a *single* message in the chat window.. This is used for example by groups.im (a single bot contact that acts like an IRC channel with bip), where the groups.im contact uses this friendly name to show you the correct nickname of who is sending the message. It can also be used by people who use extremely long nicknames with ascii art, etc.. but don't want to pollute their chat windows at every line. It can also be used by people who want to show a different nickname for each of their contacts. The friendly-name is set as a header field in the message. It does not affect the nickname shown in the contact list at all.. only the message in the chat window.

Anyways, in aMSN, we make a distincition between the 'nickname' and the 'custom nickname' (petname). I also believe the UI needs to have access to both information, because it would be helpful to have the petname (custom nickname) appear in the contact list, but the original nickname showing up in the tooltip for example, etc..

(In reply to comment #11)
> No worries Simon, I'd like to give my opinion and explain how it works with MSN
> anyways, so thanks for CC-ing me.
> So, with MSN you have a nickname (the one the contact sets), which you can
> override (The official client just puts an 'annotation' on the contact's data
> called 'AB.Nickname' and the UI uses it to replace the contact's nickname).

OK, good, that's how I'd understood it too. The one the contact sets on themselves is Telepathy's "nickname", and the override is Telepathy's "local-alias".

> You can also have a 'friendly name' which is a bit different, it's the nickname
> to be shown for a *single* message in the chat window.. This is used for
> example by groups.im (a single bot contact that acts like an IRC channel with
> bip), where the groups.im contact uses this friendly name to show you the
> correct nickname of who is sending the message.

This is inaccessible except on messages, right? So you'd never want it to be shown in a contact list, because you can't display it until you've received at least one message in the current session?

If it was possible to pull this from the server somehow, we could make it available as a third attribute on the Names interface, but at the moment we just map it to a "sender-nickname" key in each message on the Messages interface. Is that basically equivalent to what aMSN does?

> Anyways, in aMSN, we make a distincition between the 'nickname' and the 'custom
> nickname' (petname). I also believe the UI needs to have access to both
> information

Right, making this possible in Telepathy is the purpose of this bug.

(In reply to comment #12)

> 
> This is inaccessible except on messages, right? So you'd never want it to be
> shown in a contact list, because you can't display it until you've received at
> least one message in the current session?
> 
> If it was possible to pull this from the server somehow, we could make it
> available as a third attribute on the Names interface, but at the moment we
> just map it to a "sender-nickname" key in each message on the Messages
> interface. Is that basically equivalent to what aMSN does?
yep, it's not accessible expect on messages, and will never be shown in the contact list, so it's not relevant to the current bug.

Bug #25120 points out that Aliasing makes it impossible to tell whether the alias is server-stored (like XMPP) or transient (like Salut). We should fix that in Names.

Created attachment 56334 [details] [review]
proposed patch

---

Recovered from my git repository and updated to current spec practice (ish).

Comment on attachment 56334 [details] [review]
proposed patch

Review of attachment 56334 [details] [review]:
-----------------------------------------------------------------

::: spec/Connection_Interface_Names.xml
@@ +2,5 @@
> +<node name="/Connection_Interface_Names" xmlns:tp="http://telepathy.freedesktop.org/wiki/DbusSpec#extensions-v0">
> +  <tp:copyright>Copyright © 2009-2010 Collabora Ltd.</tp:copyright>
> +  <tp:copyright>Copyright © 2009 Nokia Corporation</tp:copyright>
> +
> +  <interface name="org.freedesktop.Telepathy.Connection.Interface.Names.DRAFT">

In current spec technology this should be Connection.Interface.Names1

@@ +17,5 @@
> +          account is created or at any time</li>
> +        <li>an alias (<em>local alias</em>) assigned privately by the local
> +          user and stored in the local user's contact list</li>
> +        <li>an abbreviated/less-precise form of their unique identifier
> +          (<em>pretty ID</em>)</li>

I'm less convinced about the pretty ID, see below.

@@ +60,5 @@
> +        display in an address book or list of contacts if no local alias
> +        or nickname is available.</p>
> +    </tp:docstring>
> +
> +    <method name="GetNicknames" tp:name-for-bindings="Get_Nicknames">

Not sure if there's much point in this method, we could just say "use GetContactAttributes". At the moment the contact attr is defined in terms of the method, but it needn't be.

@@ +98,5 @@
> +        <tp:error name="org.freedesktop.Telepathy.Error.InvalidHandle"/>
> +      </tp:possible-errors>
> +    </method>
> +
> +    <method name="RequestNickname" tp:name-for-bindings="Request_Nickname">

This can't be replaced by a contact attr, because it blocks.

@@ +111,5 @@
> +            for the method to return), and can distinguish between various
> +            error conditions.</p>
> +
> +          <p>This method is also appropriate for detection of whether a
> +            locally-configured nickname should be set on a server.</p>

I think this + the semantics of Nickname are sufficient to get a competent implementation in MC, but we need to think about this.

@@ +232,5 @@
> +            >XEP-0165 "Best Practices to Discourage JID Mimicking"</a>).</p>
> +
> +        <p>Connection managers SHOULD NOT use the same storage for names
> +          not assigned by the local user, since this would undermine the
> +          anti-spoofing benefits of storing a user-selected alias.</p>

Gabble currently violates this for performance reasons, by caching remote users' nicknames in the roster (once) to avoid having to download their vCard every time. We should think about whether it can do better.

Perhaps downloading everyone's vCard once, and putting it in ~/.cache, would be sufficient...

@@ +268,5 @@
> +      <tp:docstring>
> +        <p>Sets a user-defined alias for a contact. Clients SHOULD NOT call
> +          this method for names not assigned by the local user, since this
> +          would undermine the anti-spoofing benefits of storing a
> +          user-selected alias.</p>

I think it's OK for UIs to pre-fill an input box for this (e.g. when adding contacts), but they should always show the user the identifier and an input box, and only SetLocalAlias on user confirmation.

@@ +310,5 @@
> +      <tp:docstring xmlns="http://www.w3.org/1999/xhtml">
> +        <p>If this attribute on a contact is present in the result of
> +          GetContactAttributes, its value is a protocol-specific abbreviated or
> +          formatted version of their self-assigned unique identifier, which may
> +          be more appropriate to display in a user interface.</p>

This contact attribute is really two things, and perhaps we should split them.

1) De-normalized identifier

Use cases: the IRC user whose name normalizes to robot101 wants to appear as Robot101 in UIs (IRC is case-insensitive). The AIM user whose name normalizes to pseudorandomcouk wants to appear as "pseudorandom co uk" in UIs (yes, AIM is space-insensitive).

In these cases, if you normalize the user-chosen name, you get back to their "real" identifier. It would be reasonable to have your UI only display "Robot101" or "pseudorandom co uk", and have no way to get back to "robot101" or "pseudorandomcouk".

2) Abbreviated identifier

Use cases: XMPP clients abbreviate magical.trevor@gmail.com to magical.trevor. IkiWiki abbreviates OpenIDs like http://frank.livejournal.com/ to "frank [livejournal.com]" and it would be entirely reasonable for a hypothetical OpenID-based IM protocol to do the same.

In these cases, you're losing information for the sake of UI clarity. This would be appropriate for a "list of participants" sidebar in a chat, or a default display-name in an address book, or something, but you should be able to disambiguate via the real identifier somehow (maybe a tooltip).

It's not necessarily necessary to split this, though, since UIs could have a heuristic: try normalizing the pretty-id. If you don't get back to the identifier, make sure you make the real identifier available too.

MC should special-case Names.Nickname and forbid it from appearing in Account.Parameters, because the Account.Nickname property already exists.

Desired semantics for MC:

* If the protocol does not store nicknames (IRC, SIP), always set
  the nickname at connect time.

* If the protocol stores nicknames (XMPP):

  - if the nickname was changed locally since we were last connected,
    and the stored nickname on the server is the same as it was when
    we were last connected, change it

  - if the nickname was not changed locally since we were last connected,
    get the stored nickname from the server and save it locally

  - if the nickname was changed locally, and was also changed on the server,
    we could go either way; the local change could win, or the server could
    win. I vaguely prefer "local change wins", I think.

Does this pseudocode look correct?

Pseudocode for "local change wins":

* Store an extra flag, NicknameChanged, in the account data.
  It is initially FALSE.

* When we RequestConnection

  - if Names.Nickname is a parameter, that means the protocol does not
    store nicknames. Set NicknameChanged = FALSE if you like (but it
    doesn't really matter). Add { Names.Nickname => Account.Nickname } to
    the value of Parameters that will be used in RequestConnection.

  - else the protocol does store nicknames.

    + if NicknameChanged == TRUE, as soon as possible, call
      Set(Names, Nickname, Account.Nickname). On success, set
      NicknameChanged = FALSE.

    + else, as soon as possible, call RequestNickname(SelfHandle).
      Overwrite Account.Nickname with the result.

* When UI calls Set(Account, Nickname, x):

  - set Account.Nickname = x

  - set NicknameChanged = TRUE

  - if we are currently connected, call
    Set(Names, Nickname, Account.Nickname) on
    the Connection, and on success, set NicknameChanged = FALSE

Pseudocode for "server change wins":

* Store an extra string, OldNickname, in the account data.
  It is initially "".

* When we RequestConnection

  - if Names.Nickname is a parameter, that means the protocol does not
    store nicknames. Set OldNickname = "". Add
    { Names.Nickname => Account.Nickname } to the value of Parameters
    that will be used in RequestConnection.

  - else the protocol does store nicknames. As soon as possible,
    call RequestNickname(SelfHandle).

    + if the result is either "" or the value of OldNickname,
      call Set(Names, Nickname, Account.Nickname). On success,
      set OldNickname = "".

    + else overwrite Account.Nickname with the value from the server

* When UI calls Set(Account, Nickname, x):

  - if OldNickname == "", set it to Account.Nickname

  - set Account.Nickname = x

  - if we are currently connected, call
    Set(Names, Nickname, Account.Nickname) on
    the Connection, and on success, set OldNickname = ""

Comment on attachment 56334 [details] [review]
proposed patch

Review of attachment 56334 [details] [review]:
-----------------------------------------------------------------

Shouldn't we deprecated Aliasing as well?

::: spec/Connection_Interface_Names.xml
@@ +60,5 @@
> +        display in an address book or list of contacts if no local alias
> +        or nickname is available.</p>
> +    </tp:docstring>
> +
> +    <method name="GetNicknames" tp:name-for-bindings="Get_Nicknames">

Agreed, let's drop it.

@@ +172,5 @@
> +        <p>This property MAY be set on a newly-created connection while it is
> +          still in the Disconnected or Connecting state. If this is done, the
> +          Connection must store the given nickname temporarily, until
> +          connecting succeeds or fails, and apply it if the connection attempt
> +          is successful.</p>

Cool; that solves the Idle case.

@@ +177,5 @@
> +
> +        <p>On protocols where nicknames do not persist between connections
> +          (such as <code>local-xmpp</code>), the connection manager SHOULD
> +          support
> +          <code>org.freedesktop.Telepathy.Connection.Interface.Names.DRAFT.Nickname</code>

Names1.Nickname

@@ +199,5 @@
> +
> +    <tp:contact-attribute name="nickname" type="s">
> +      <tp:docstring xmlns="http://www.w3.org/1999/xhtml">
> +        <p>The same string that would be returned by
> +          <tp:member-ref>GetNicknames</tp:member-ref>.

Should be updated if we drop GetNicknames()

@@ +280,5 @@
> +      </tp:docstring>
> +
> +      <arg name="Contact" type="u" tp:type="Contact_Handle" direction="in">
> +        <tp:docstring>
> +          The contact.

I guess we can't do SetLocalAlias(self_handle) right? May be good to say it to be clear.

@@ +310,5 @@
> +      <tp:docstring xmlns="http://www.w3.org/1999/xhtml">
> +        <p>If this attribute on a contact is present in the result of
> +          GetContactAttributes, its value is a protocol-specific abbreviated or
> +          formatted version of their self-assigned unique identifier, which may
> +          be more appropriate to display in a user interface.</p>

1) Wouldn't them use the nickname in our account-free Idle glory future?
2) I would ignore this for now; we can always add it later if we really need it.

(In reply to comment #18)
> Shouldn't we deprecated Aliasing as well?

After this has been implemented, I think.

(I'd be inclined to implement it as a TpNamesMixin which also implements Aliasing, then drop Aliasing in next.)

> I guess we can't do SetLocalAlias(self_handle) right? May be good to say it to
> be clear.

Protocol-dependent. In XMPP, you can, but it's rarely useful (it would add you to your own roster with the given local alias, rather than setting your nickname).

It'd be worth mentioning that it's rarely useful and might not work.

> 1) Wouldn't [pretty-name] use the nickname in our account-free Idle glory
> future?

Yes, I believe this is already how it works in Idle (Rob's identifier is the normalized form "robot101" and his nickname in Aliasing is "Robot101").

I'm not sure whether AIM has nicknames in addition to un-normalized identifiers, though?

> 2) I would ignore this for now; we can always add it later if we really need
> it.

I agree.

I expect that UIs (and, in particular, libfolks) are likely to want the second use of pretty-id at some point, but we can always add it.

I took your patch and updated it for next. I don't think there's any point in looking at master at all for this.

(In reply to comment #16)
> In current spec technology this should be Connection.Interface.Names1

Done.

> I'm less convinced about the pretty ID, see below.

I removed pretty ID for now then.

> Not sure if there's much point in this method, we could just say "use
> GetContactAttributes". At the moment the contact attr is defined in terms of
> the method, but it needn't be.

Yeah I removed that and added a note.

(In reply to comment #18)
> @@ +199,5 @@
> > +
> > +    <tp:contact-attribute name="nickname" type="s">
> > +      <tp:docstring xmlns="http://www.w3.org/1999/xhtml">
> > +        <p>The same string that would be returned by
> > +          <tp:member-ref>GetNicknames</tp:member-ref>.
> 
> Should be updated if we drop GetNicknames()

Done.

(In reply to comment #19)
> (I'd be inclined to implement it as a TpNamesMixin which also implements
> Aliasing, then drop Aliasing in next.)

This sounds good but let's just do this straight on next.

> > I guess we can't do SetLocalAlias(self_handle) right? May be good to say it to
> > be clear.
> 
> Protocol-dependent. In XMPP, you can, but it's rarely useful (it would add you
> to your own roster with the given local alias, rather than setting your
> nickname).
> 
> It'd be worth mentioning that it's rarely useful and might not work.

I added a note about this.

Nothing else changed though; see my branch.

(In reply to comment #19)
> (In reply to comment #18)
> > Shouldn't we deprecated Aliasing as well?
> 
> After this has been implemented, I think.

Any chance to have this done for 1.0? We are having a bunch of alias related issue in Folks so it would be nice to finally clean this mess.

This is the last serious blocker for tp-spec-next and tp-glib-next.

Is the proposed spec in http://cgit.freedesktop.org/~jonny/telepathy-spec/log/?h=next-names mostly what we want? Maybe I could start sketching a mixin now?

I'm wondering if we could let CM store local-alias on disk for protocols that cannot store online.

I see in the spec of Contact_Metadata_Storage_Type:
"""
To make it easier to deal with such protocols, if clients set metadata on a contact who is not in the required state, the Connection MUST cache the metadata for the duration of the session. If clients request the attributes of that contact after the appropriate "set" method has returned successfully, the Connection MUST return the new (cached) value.
"""

This session cache could be extended to a permanent disk cache? It MUST do in memory cache, but COULD do on disk cache. Does that sounds sane?

Here is a mixin for current spec: http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=next-names

I wrote it based on TpAvatarMixin, but did not use it in any CM yet, so it's probably full of bees. But you get the idea :-)

Here is a tp-glib master branch: http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=names

It add a mixin that implement both ifaces, and add TpContact:nickname/local-alias properties. With (incomplete) unit tests of course!

I think it will be convenient to add TpContact::display-name property that follow "local-alias > nickname > id" rule, which is what TpContact::alias is but done CM-side.

(In reply to comment #23)
> This session cache could be extended to a permanent disk cache? It MUST do in
> memory cache, but COULD do on disk cache. Does that sounds sane?

You mean MUST and MAY, or possibly MUST and SHOULD (<https://tools.ietf.org/html/rfc2119> doesn't define COULD), but yes.

If Gabble cached aliases on disk (sqlite?), it wouldn't have to cache them on the roster, which undermines the roster's ability to be used for a reliable local-alias; so that'd be good.

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.

(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

(In reply to comment #28)
> (In reply to comment #27)
> > +/* 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.

Actually mixins are a mess:
1) TpBaseContactList: must be subclassed
2) TpPresenceMixin: no interface but a TpPresenceMixinClass to include in class struct.
3) TpAvatarsMixin: no interface and no class struct, virtual methods are in instance's private.
4) TpNamesMixin: like TpAvatarsMixin because I've copied it.
5) TpGroupMixin: like TpPresenceMixin, but the class struct does not have padding for future extension but it has a private struct (that is leaked, btw)

3 and 4 are easy to change since no CM uses them yet, and they did not get merged in master. So I guess the best to keep a bit consistent is adding a TpNamesMixinClass for the vmethods.

(In reply to comment #29)
> Actually mixins are a mess:
> 1) TpBaseContactList: must be subclassed

It's primarily an object, not a mixin, because that's how we've traditionally done the contact list (and in master it had to be a TpChannelManager for historical reasons[1]; in next it doesn't really need to be).

> 2) TpPresenceMixin: no interface but a TpPresenceMixinClass to include in class
> struct.

My concern about this approach is that it's probably pessimal for gobject-introspection - not that we're introspectable on the CM side at the moment anyway, but we'd like to be one day.

> 3) TpAvatarsMixin: no interface and no class struct, virtual methods are in
> instance's private.

This seems wrong conceptually, if for no other reason: methods are a property of the class, not the instance.

> 5) TpGroupMixin: like TpPresenceMixin, but the class struct does not have
> padding for future extension but it has a private struct (that is leaked, btw)

Bugs/patches welcome :-)

[1] ContactList, ContactGroup channels

I've totally reworked this, branch updated. I hope I've addressed all concerns.

(In reply to comment #28)
> 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?

comments?

(In reply to comment #28)
> 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?

Yes, but that's a bug. It is wrong for a CM to mix up local-alias (a name the local user chose, which can be assumed to be trustworthy) and nickname (a name the remote contact chose, which might be malicious).

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

I think the only solution to this is to distinguish between "get" and "request", have "get" read from cache if possible or kick off a request if not (with either the mixin or the CM implementing a cache), and have "request" always round-trip to the server.

I vaguely feel as though Gabble caching vCards might be better than tp-glib caching nicknames, but perhaps I'm wrong about that.

Since some of the complains against TpNamesMixin implementation were actually copied from TpAvatarsMixin that is already merged into 'next', I've opened bug #55920 to fix them.

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?

Thanks for the review, I'll check point by point tomorrow.

A few points, though:

1) I would like to get this merged without any local cache, to not delay for another 10months. I guess this means that gabble will have to keep is hack (storing nickname in roster) for now. That means that implicit_request_nickname() will have to be no-op in gabble, in a way or another (adding a flag probably, as you suggested).

2) tp_dbus_properties_mixin_set_async() would be great to have, but I would say it's not blocker to merge this. Current implementation is good enough for now, ok?

3) About tp_names_mixin_init(): I had the idea of adding a TpNamesMixinPrivate* in TpBaseConnectionPrivate struct. This wast 64bits per connection on CM that don't have aliases (big deal...) and simplify all those offset/quark weirdness. Like that I can just have:

tp_names_mixin_init(TpBaseConnection *self)
{
  self->priv->names_mixin = g_slice_new0();
  self->priv->names_mixin->foo = NULL;
}

tp_names_mixin_finalize(TpBaseConnection *self)
{
  g_slice_free(self->priv->names_mixin);
  self->priv->names_mixin = NULL;
}

TpBaseConnection::finalize()
{
  g_assert(self->priv->names_mixin == NULL);
}

That way we have perfectly introspectable mixins, I think.

(In reply to comment #35)
> 1) I would like to get this merged without any local cache, to not delay for
> another 10months. I guess this means that gabble will have to keep is hack
> (storing nickname in roster) for now.

It would make me sad to have an implementation in Gabble that still subverted the "local-alias is user-chosen" rule, particularly since Wocky links sqlite already... but I do see your point.

If we don't get this right first time, I would definitely like to have a clear picture of how we're going to Do It Right™ later, so that we know the API doesn't rule it out altogether.

I'm tempted to say "how hard can it be?" and try implementing it, tbh... all we need is a sqlite database (contact => vCard) or (contact => vCard-derived nickname) in ~/.cache?

(Slight complicating factor that the regression tests might need to clear it out once per test or something.)

It's a pity that XMPP doesn't allow us to store metadata on our roster, other than aliases and groups...

(In reply to comment #34)
> 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).

Do you have WiP branches for any of these or would you like me to hack on them?

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

If you're pushing the "implementing it at all is better than not" angle, having the CM declared to be responsible for persistence (if any) shortens the shortest path from here to an implementation, I think.

(In reply to comment #35)
> 3) About tp_names_mixin_init(): I had the idea of adding a
> TpNamesMixinPrivate* in TpBaseConnectionPrivate struct. This wast 64bits per
> connection on CM that don't have aliases (big deal...) and simplify all
> those offset/quark weirdness.

... at the cost of having a conceptual circular dependency: TpBaseConnection has to know (a small amount) about every mixin it supports. Hanging qdata off the TpBaseConnection seems cleaner to me, tbh.

(The resulting API is pretty similar, though.)

> tp_names_mixin_finalize(TpBaseConnection *self)

When would a high-level language call this? Python's __del__ special method, and similar mechanisms, are generally best avoided.

Hanging some qdata off the TpBaseConnection has the advantage that g_object_finalize() will automatically call its GDestroyNotify.

Another possible pattern, if you don't mind conceptual circular dependencies, would be for tp_base_connection_finalize (and dispose, if that's needed) to finalize the known mixins.

Based on prototyping in Salut:

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

This is actually pretty subtle to deal with. There are four possible things that can happen:

* CM can immediately tell that it's not going to work (fail synchronously
  without emitting any signals or having any side-effects)
* CM makes a request, which fails later
* CM makes a request, which succeeds later
* CM makes a request, which succeeds with a name that differs from
  what you wanted (e.g. you asked for U+00B3 SUPERSCRIPT THREE, but the
  protocol stores in NFKD so you actually got U+0033 DIGIT THREE)

We can easily represent immediate failure as later failure, if desired, but I think we should fail immediately without side-effects if there are invalid handles. I think InvalidHandle checking in SetAliases should be the first thing that's done, and the side-effects should only be allowed to happen if every handle is valid. Similarly, if the CM can't set local aliases, SetAliases should raise an error without side-effects if any handle other than the self-handle is given, and if the CM can't set our own nickname, SetAliases should raise an error without side-effects if the self-handle is given.

As currently implemented, SetLocalAlias works like this:

    if handle is invalid:
        fail

    signal that the local alias changed to the desired value

    if we can set local aliases at all:
        asynchronously set the local alias

        if it succeeds:
            succeed (leaving the mixin thinking that it has the desired
            value, unless the CM explicitly changes it to a modified value)
        else:
            fail (leaving the mixin thinking that the local alias has its
            new value, unless the CM explicitly reverts it)
    else:
        "succeed", for some reason (I think this is a bug - the spec says
        TP_CONTACT_METADATA_STORAGE_TYPE_NONE should reject attempts
        to set it)

and Set(Nickname) works similarly.

This means that if the set operation fails, the CM is expected to signal a change back to the previous value before completing its GAsyncResult; and if the set operation succeeds but modifies the value, the CM is expected to signal a change to the modified value before completing its GAsyncResult. Both of those seem subtle.

SetAliases suffers further, from the fact that it's plural but "shards" into several singular requests, each of which can fail asynchronously and independently; so it's basically impossible to implement it correctly. Gabble implements SetAliases by always pretending to succeed (unless at least one handle is invalid, in which case it fails, but still has side-effects! - but that's a bug, as discussed above). I notice that the mixin does the same; I think that's fine.

For the nickname part, Gabble only signals the alias change when the change has actually happened - when the server replies successfully to our 'set' IQ. Specifically: on success, replace_reply_cb copies patched_vcard into the in-memory cache of vCards, then calls observe_vcard to treat that new value as if it had been seen in the reply to a 'get' - it's the same code path.

For the local-alias part, it only signals the alias change when the server responds to the 'set' IQ with a roster push. Again, it does that via the same code path that would respond to any other roster push.

So, it would actually be easier for Gabble if the mixin didn't do anything clever with these methods, and relied on Gabble to call nickname_changed/local_alias_changed whenever appropriate! I think that'd also be easier to document: "you must call this whenever the $foo changes" is easier to understand than "you must call this whenever the $foo changes, except...".

The code currently on this bug will implement SetAliases by calling set_local_alias_async repeatedly. I don't think that can be right, particularly for things like Salut - there should be a special case for the self-handle which would call set_nickname_async instead.

There is an extra wart, which is that if we are on our own roster, Gabble will respond to SetAliases by altering our local-alias *and* our nickname. Haze doesn't (libpurple has no concept of being on your own roster). I think it's reasonable to special-case Gabble's set_nickname_async() so it sets both nickname and local-alias if we are on our own roster, and not have this special case in any other CM - other CMs' SetAliases would just behave like Haze's.

(In reply to comment #34)
> 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.

I've patched this, I'll attach a patch.

> ofdT, not imt1.

Likewise.

> I think InvalidHandle checking in SetAliases should be the first thing
> that's done, and the side-effects should only be allowed to happen if
> every handle is valid. 

This too.

(In reply to comment #28)
> > 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 think we should have that. Perhaps Names_Flags and Names_Flag_Can_Set_Nickname, consistent with ContactInfo?

(We could even have a Pushes_Nicknames flag, just like ContactInfo. It'd be false on XMPP, since PEP is the exception rather than the rule.)

Created attachment 72519 [details] [review]
Fix interface namespacing

---

For master, Avatars1 still needs to be in the ofdT namespace.

In some contexts it's necessary to spell out the whole interface name.

Created attachment 72520 [details] [review]
Names mixin: in legacy SetAliases, early-return on  invalid handles

Regarding this bit in the spec:

> To make it easier to deal with such protocols, if clients set
> metadata on a contact who is not in the required state, the
> Connection MUST cache the metadata for the duration of the session.
> If clients request the attributes of that contact after the
> appropriate "set" method has returned successfully, the Connection
> MUST return the new (cached) value. If the contact is later placed
> in the required state to store metadata (for instance, if subscription
> to the contact's presence is requested, on a protocol like MSN where
> the alias has storage type Subscribed_Or_Pending), the connection
> MUST store the cached metadata at that time.

I'm starting to think the mixin shouldn't try to ensure this: on a CM where this is significant, the CM should compensate.

Haze is such a CM, so it's another useful target for prototyping - it has something resembling TP_CONTACT_METADATA_STORAGE_TYPE_SUBSCRIBED (or at least, it only stores aliases for "buddies" on our contact list, and it represents those as subscribe = TP_SUBSCRIPTION_STATE_YES because it can't know any better). At the moment, setting these contacts' aliases just fails, but we could make it just signal a successful change for them (storing the alias in TpNamesMixin), and copy from TpNamesMixin to the new buddy whenever it adds a buddy.

I also wonder whether the mixin should be a pure "view" of data stored by the CM's "model", and not store anything at all itself? That would mean it would grow these methods:

    TpContactMetadataStorageType (*get_alias_storage) (TpBaseConnection *);
    TpNamesFlags (*get_names_flags) (TpBaseConnection *);
    gchar *(*dup_contact_local_alias) (TpBaseConnection *, TpHandle);
    gchar *(*dup_contact_nickname) (TpBaseConnection *, TpHandle);

and stop needing to store any data of its own. In practice, I suspect most (all?) of our CMs are going to have a "model" that could be used for this: are we gaining anything by duplicating it in the mixin?

(In reply to comment #42)
> I'm starting to think the mixin shouldn't try to ensure this: on a CM where
> this is significant, the CM should compensate.
...
> I also wonder whether the mixin should be a pure "view" of data stored by
> the CM's "model", and not store anything at all itself?

Here is a sketchy implementation of those (tests currently fail but I think I know why):

http://cgit.freedesktop.org/~smcv/telepathy-glib/log/?h=names

(In reply to comment #42)
> In practice, I suspect most
> (all?) of our CMs are going to have a "model" that could be used for this

Gabble has the roster and the nickname cache. The nickanme cache is currently qdata stapled onto handles, which is crazy (and leaky, since handles are immortal now), but it could just be in the vCard manager.

Salut has the SalutContact objects.

Haze has PurpleBuddy objects.

...

In practice, I think any non-trivial CM is going to have something analogous: the test CM is misleading, because it doesn't have the complexity of talking to a real network protocol.

http://cgit.freedesktop.org/~smcv/telepathy-glib/commit/?h=names&id=8a6535c1c238108fc8173e2337ca0c7d000d32e4 --> "This the #TpContact:local-alias," missing a verb.

http://cgit.freedesktop.org/~smcv/telepathy-glib/commit/?h=names&id=f0e8ae7326411f4992be79d7da11b2c0bf600819 --> this is from another commit:
- tp_names_mixin_one_nickname_changed (base, data->contact, nickname);

http://cgit.freedesktop.org/~smcv/telepathy-glib/commit/?h=names&id=414118702d96a042848625552ef6b8b2a168ef6e --> you can use self_handle var in the tp_names_mixin_one_nickname_changed() call.

http://cgit.freedesktop.org/~smcv/telepathy-glib/commit/?h=names&id=156d18599a9b56abe575eb4076e970ba34aa2860 --> adding a new file for that seems overkill, why not keep that with the other names tests? otoh tests/dbus/contacts.c is already huge... maybe move everything to a new tests/dbus/names.c file?

(In reply to comment #45)

Branch updated.

> http://cgit.freedesktop.org/~smcv/telepathy-glib/commit/
> ?h=names&id=8a6535c1c238108fc8173e2337ca0c7d000d32e4 --> "This the
> #TpContact:local-alias," missing a verb.
> 
> http://cgit.freedesktop.org/~smcv/telepathy-glib/commit/
> ?h=names&id=f0e8ae7326411f4992be79d7da11b2c0bf600819 --> this is from
> another commit:
> - tp_names_mixin_one_nickname_changed (base, data->contact, nickname);
> 
> http://cgit.freedesktop.org/~smcv/telepathy-glib/commit/
> ?h=names&id=414118702d96a042848625552ef6b8b2a168ef6e --> you can use
> self_handle var in the tp_names_mixin_one_nickname_changed() call.

Fixed in "fixup!" commits. The second one is reinstated later in the patch series, at the right place.

> http://cgit.freedesktop.org/~smcv/telepathy-glib/commit/
> ?h=names&id=156d18599a9b56abe575eb4076e970ba34aa2860 --> adding a new file
> for that seems overkill, why not keep that with the other names tests? otoh
> tests/dbus/contacts.c is already huge... maybe move everything to a new
> tests/dbus/names.c file?

I renamed self-names.c to names.c and moved test_names() there, with a bit of refactoring to not rely on infrastructure from the regression tests for obsolete TpContact code paths.

The refactoring is in "test_names: disentangle from TpContact old-code-path tests a bit" and prior commits. The commit "Move test_names from test-contacts to test-names" just moves the function as-is, and makes some struct member names match (async_result -> result, base_connection -> base_conn).

I think this is in a state where we can usefully try porting CMs. I'll do Salut, you do Haze, and whoever finishes first can start on Gabble?

Some design bits and pieces I'm not sure about:

The changed() functions redundantly signal a nickname or local-alias that dup_nickname or dup_local_alias should already be returning. We should either g_warn_if_fail(), or make them just take the handles (gsize + const TpHandle * for the plural case) as parameters?

Similarly, request_nickname() redundantly returns a nickname that we (should) already know. We should either g_warn_if_fail() that it matches what dup_nickname() returns, or make it void and use dup_nickname()?

I think we do want a NameFlags property containing at least Can_Set_Nickname, but we can think about what else to put in it (if anything) while working on the CMs.

I ported Salut (http://cgit.freedesktop.org/~smcv/telepathy-salut/log/?h=names) and started on Gabble (http://cgit.freedesktop.org/~smcv/telepathy-salut/log/?h=wip-names, http://cgit.freedesktop.org/~smcv/telepathy-salut/log/?h=wip-less-qdata).

In Gabble, one test fails (roster/groups.py), which I haven't debugged yet, and we still need a nickname cache that doesn't undermine this interface by saving nicknames to the roster, i.e. a local sqlite or something. GabbleVCardManager is a natural place to put that cache, but may require refactoring WockyCapsCache's sqlite code so we don't duplicate most of it.

Feedback from Gabble implementation: we need to document whether set_nickname_async() and set_local_alias_async() will receive "" or NULL as their argument (and we should ensure that it's always one of those, never the other).

(In reply to comment #48)
> and started
> on Gabble

That should of course say:

http://cgit.freedesktop.org/~smcv/telepathy-gabble/log/?h=wip-names
http://cgit.freedesktop.org/~smcv/telepathy-gabble/log/?h=wip-less-qdata

http://cgit.freedesktop.org/~smcv/telepathy-glib/tree/tests/dbus/names.c?h=names&id=e55d7bc56c4af7e80d9be785596844255de7875d --> I was doing the same for avatars, I changed ADD macro to do the 3 g_test_add() instead of repeating them. Dunno if you find that better :-)

http://cgit.freedesktop.org/~smcv/telepathy-glib/commit/?h=names&id=29766a5cfc785bc314b40ec87196e6b78bc0b68a --> you can give NULL callback to g_simple_async_report_error_in_idle(). Early return saves a bit of work, but we don't do it usually... Don't really matters of you prefer like that :-)

http://cgit.freedesktop.org/~smcv/telepathy-glib/commit/?h=names&id=57eaa0241be6ad60ca2a818cb120e4a8243a99ea --> 
+ g_value_set_string (value,
+ iface->dup_nickname (conn,
+ tp_base_connection_get_self_handle (conn)));
--> should be g_value_take_string()

+ str = iface->dup_nickname (base, contact);
+
+ if (!tp_str_empty (str))
--> you should free str in the case it is g_strdup(""); or is that a forbidden return value for dup_nickname? Same for dup_local_alias() later in the patch.

tp_names_mixin_nicknames_changed: Maybe I've misunderstood something, but if nickname != legacy_alias then you don't have to include that contact in the array. Also why did you remove the check if the array is empty, we could still have empty array here and avoid useless signals, no?

http://cgit.freedesktop.org/~smcv/telepathy-gabble/commit/?h=wip-less-qdata&id=e5cd53ee8ebca0d0839ad1345183adc93510335b --> you can replace g_hash_table_lookup_extended() with g_hash_table_contains(), no?

(In reply to comment #51)
> http://cgit.freedesktop.org/~smcv/telepathy-gabble/commit/?h=wip-less-
> qdata&id=e5cd53ee8ebca0d0839ad1345183adc93510335b --> you can replace
> g_hash_table_lookup_extended() with g_hash_table_contains(), no?

True.

(In reply to comment #50)
> http://cgit.freedesktop.org/~smcv/telepathy-glib/tree/tests/dbus/names.
> c?h=names&id=e55d7bc56c4af7e80d9be785596844255de7875d --> I was doing the
> same for avatars, I changed ADD macro to do the 3 g_test_add() instead of
> repeating them. Dunno if you find that better :-)

I think it's perhaps clearer as-is, for now? We can easily change this if we add more modes.

> http://cgit.freedesktop.org/~smcv/telepathy-glib/commit/
> ?h=names&id=29766a5cfc785bc314b40ec87196e6b78bc0b68a --> you can give NULL
> callback to g_simple_async_report_error_in_idle(). Early return saves a bit
> of work, but we don't do it usually... Don't really matters of you prefer
> like that :-)

I'd dimly remembered that GSimpleAsyncResult doesn't always like NULL callbacks, checked in my documentation (for 2.32) and noticed there was no (allow-none). Is that a new thing?

> http://cgit.freedesktop.org/~smcv/telepathy-glib/commit/
> ?h=names&id=57eaa0241be6ad60ca2a818cb120e4a8243a99ea --> 
> + g_value_set_string (value,
> + iface->dup_nickname (conn,
> + tp_base_connection_get_self_handle (conn)));
> --> should be g_value_take_string()

Indeed, this is a leak.

> + str = iface->dup_nickname (base, contact);
> +
> + if (!tp_str_empty (str))
> --> you should free str in the case it is g_strdup(""); or is that a
> forbidden return value for dup_nickname? Same for dup_local_alias() later in
> the patch.

This is in aliasing_dup_alias(), right? I do free it whenever it isn't returned. (There's an unnecessary-but-harmless g_free (NULL) if it returns NULL.)

> tp_names_mixin_nicknames_changed: Maybe I've misunderstood something, but if
> nickname != legacy_alias then you don't have to include that contact in the
> array.

Suppose we have:

    local_alias = empty
    nickname = "Dave"
    identifier = "dave@example.com"
    =>
    legacy alias = "Dave"

and we change nickname to empty. Now the legacy alias changes from "Dave" to "dave@example.com" and we have to emit AliasesChanged.

For simplicity, the code does not currently detect the case where

    local_alias = empty
    nickname = "dave@example.com"
    identifier = "dave@example.com"
    =>
    legacy alias = "dave@example.com"

and we change nickname to empty, which has no practical effect; it will still emit a redundant AliasesChanged signal in this case. If you want to handle this case fully correctly, then yes we need to bring back the one_changed logic.

(Here, "empty" means either "" or NULL.)

> Also why did you remove the check if the array is empty, we could
> still have empty array here and avoid useless signals, no?

The NicknamesChanged hash table can only be empty in the case where the input is empty, which is handled by an early-return now.

The AliasesChanged array can only be empty in the case where we clear a nickname that happens to be identical to the identifier (which I think is enough of a corner-case that I don't mind a redundant signal), or if the input is empty (in which case the same early-return catches it).

(In reply to comment #52)
> For simplicity, the code does not currently detect the case where
> 
>     local_alias = empty
>     nickname = "dave@example.com"
>     identifier = "dave@example.com"
>     =>
>     legacy alias = "dave@example.com"
> 
> and we change nickname to empty, which has no practical effect

In fact, I don't think it *can* detect this case, because by the time we enter this function, we have already forgotten what the old nickname was.

This is the same reason why this function can't filter out no-op changes from NicknameUpdated: dup_nickname() already returns the new version.

I think that way round (dup_nickname() already returns the new version by the time this function is called) seems less astonishing (and far easier to implement!) than documenting that dup_nickname() must still return the old version until just after the change-notification call.

Also, if people mis-implement this and have dup_nickname() still returning the old version, I don't think we actually notice; whereas if we relied on dup_nickname() still returning the old version but someone mis-implemented it to return the new version, we'd never emit a change notification.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/telepathy/telepathy-spec/issues/7.