[Bug 55920] Backport Avatars1 to master

[bugzilla-daemon at freedesktop.org]

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

--- Comment #2 from Simon McVittie <simon.mcvittie at collabora.co.uk> ---
General comments:

I wonder whether we really need the Clear method and its C implementation? We
could use Set([], ""), analogous to what Mission Control does. In many (most?)
CMs they're going to end up in the same code path anyway?

Something in TpAvatarsMixin should explain the concept of tokens, for people
who haven't read the eventually-to-be-deprecated D-Bus API. Perhaps wording
like this in TpAvatarsInterface:

    Several functions in this interface use the concept of an
    "avatar token". This is an opaque UTF-8 string used as part of the
    avatar's filename in the cache maintained by the TpAvatarsMixin.

    The empty avatar token "" is reserved to mean "no avatar".

    Avatar tokens must be unique within a particular pair of
    (connection manager name, protocol name): if the avatar changes
    then the token must also change. For instance, the SHA-1 of the
    binary data of the image, expressed in hexadecimal, makes a good
    avatar token. If a protocol indicates avatar changes by reporting
    the timestamp of the last change, a string containing the timestamp
    and the contact's identifier, such as "1357234010|alice at example.com",
    would be a reasonable avatar token (but just using the timestamp
    would not be sufficient, since two contacts changing their avatars
    during the same second would get the same token).

    Unlike avatar URIs in the Avatars1 D-Bus interface, avatar tokens
    are not required to be unique between connection managers or
    between protocols: the TpAvatarsMixin is responsible for ensuring
    that the filename in the cache includes the connection manager
    name, the protocol and the token.

    Avatar tokens may contain any UTF-8, but characters outside the
    ASCII alphanumeric range are not stored efficiently; restricting
    avatar tokens to an alphanumeric or mostly-alphanumeric string
    is recommended. Avatar tokens which differ only by the case of
    ASCII letters may cause collisions in the cache (particularly
    on Windows), and should be avoided.

-----

>     Add Avatars1 iface spec

> +  <tp:copyright>Copyright (C) 2005-2008 Collabora Limited</tp:copyright>

I'm pretty sure we touched this file during 2012. (The older additional
copyright dates are fine; they're because it started as a copy of Avatars.)

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

Needs backporting to ofdT (for the <interface> you have to spell it out in
full, for the <tp:requires> you can abbreviate). Also, search the XML for other
instances, in both forms; there are several.

This also needs to land in spec master.

> +    <tp:mapping name="Avatar_URI_Map">
> +      <tp:docstring>
> +        Mapping signalled by <tp:member-ref>AvatarsUpdated</tp:member-ref>,
> +        indicating the avatar of a number of contacts has changed.
> +      </tp:docstring>

Are we ever going to see two contacts' avatars change simultaneously? (We can't
in XMPP, but perhaps we can in other protocols?)

> +        <tp:token-ref>avatar</tp:token-ref> as been omited from the

"has been omitted"

+      <em>XDG_CACHE_HOME</em>/telepathy/avatars/<em>cm</em> where <em>cm</em>
+      is choosen by the Connection Manager in a way that an avatar
+      update always result in a different URI. For example using the image

"in such a way that", "is chosen by", "always results in".

Is <em>cm</em> meant to represent "gabble" or "gabble/jabber/0123456789abcdef"?
Perhaps consider:

    .../<em>cm</em>/<em>opaque</em> where <em>cm</em> SHOULD be the
    <tp:type>Connection_Manager_Name</tp:type>, and <em>opaque</em>
    is chosen by the connection manager in such a way that a change
    to the avatar always results in a different URI.

(Implementation detail: telepathy-glib on the client side currently does the
equivalent of setting <em>opaque</em> to <em>protocol</em>/<em>token</em>.)

> +      if no client explicitely claimed its interest.</p>

explicitly

> + <xi:include href="Protocol_Interface_Avatars1.xml"/>

... does not appear to exist in this branch?

-----

>     TpBaseConnection: Add internal API to get cm_name and protocol_name
>     TpBaseConnection: add internal API to check if a client interest is set

Fine

-----

>     Add TpAvatarsMixin and TpAvatarsInterface

> + * To use the avatars mixin, include a #TpAvatarsMixin somewhere in your
> + * instance structure, and call tp_avatars_mixin_init() from your init function
> + * or constructor, and tp_avatars_mixin_finalize() from your finalize function.

Same comments as TpNamesMixin: this is never going to be usable from g-i as-is,
but if we just have an init(TpBaseConnection *) and store the
TpAvatarsMixinPrivate* directly as qdata, it could be.

>   * 2) no avatar - We know that the contact has no avatar. In that case contact
> +   *    is mapped to a an AvatarData with token=NULL and uri=NULL.

What is the value of token? Is it in fact always the same string as uri?

> +  if (!offset_quark)
> +    offset_quark = g_quark_from_static_string ("TpAvatarsMixinOffsetQuark");

I'd slightly prefer (offset_quark != 0), and it could be G_UNLIKELY.

> +    TpAvatarRequirements *requirements)

const, please

>   tp_base_connection_add_possible_client_interest (base,
> +      TP_IFACE_QUARK_CONNECTION_INTERFACE_AVATARS);

Is this meant to be AVATARS1?

> +  TpAvatarsInterfaceRequestAvatarsFunc request_avatars;

I'd prefer this to be called refresh_avatars, with the same change to the name
of the typedef. Rationale: that's what the modern version of the D-Bus method
is called, and it has Tp "refresh" semantics (start an async operation and
return immediately), not Tp "request" semantics (start an async operation and
call back when it finished, or do a blocking operation).

> +  /* IMPLEMENT(request_avatar); */

I notice that this part of the old API is unimplemented. Do we expect CMs that
don't want to regress to reimplement that entire method? (Not unreasonable,
tbh; by definition, either failure to implement it is not a regression, or they
already have an implementation.)

> build_avatar_filename

Very nearly a duplicate of the one in TpContact. Can we please make this
_tp_build_avatar_filename (cm_name, protocol, ...) so TpContact and this code
can share it?

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