Here is the branch: http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=avatars

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

I'm very tempted to revert the new avatar stuff from next, then reinstate it via master - it will add even more complexity to the initial port of each CM to next, and I don't want too many things on the critical path from "switch CM to next" to "it compiles again". If the new avatar stuff "misses the boat" for 1.0, we can put it in 1.2.

As far as I can see, the only CM with a 'next' branch is Haze, which hasn't been updated to use this mixin.

(In reply to comment #3)
> I'm very tempted to revert the new avatar stuff from next

I did so. I think we'll come back to this after Bug #69431.

I'm tempted to change the D-Bus API for setting the avatar to look more like my version of Names: see <https://bugs.freedesktop.org/show_bug.cgi?id=14540#c17> for the logic MC would use.

SelfAvatar would be a write-only property of type (ays), probably, and we'd need a RequestAvatar(u: Handle) -> (s: URI, s: MIMEType) with big warnings about "this will eat all the bandwidth unless you really really know what you're doing" - or perhaps just RequestSelfAvatar() with the same result.

-- 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-glib/issues/105.