[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 #38 from Simon McVittie <simon.mcvittie at collabora.co.uk> ---
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.

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