Branch implementing this:

http://git.collabora.co.uk/?p=user/xclaesse/telepathy-glib.git;a=shortlog;h=refs/heads/avatar-requirements

Didn't test that code yet, have to use it in empathy side.

Note that we also want to know the avatar requirements when offline, so maybe the TpConnectionAvatarRequirements struct could be renamed to TpAvatarRequirements so it can be used in any other APIs like TpProtocol.

> Or maybe it can be done in CORE since it's just one dbus call with no
> update?

No, I prefer it as a feature. Most TpConnections won't care about this feature (it's only useful to UIs that can set the avatar, like the Empathy main window), so it'd be a waste of time and memory for (say) telepathy-logger, Mission Control, a dedicated VoIP UI or a Tubes app to download and remember this information.

(In reply to comment #2)
> Note that we also want to know the avatar requirements when offline, so maybe
> the TpConnectionAvatarRequirements struct could be renamed to
> TpAvatarRequirements so it can be used in any other APIs like TpProtocol.

Yes please. Also, it should have a boxed type (look at recent patches that make TpIntSet into a boxed type - basically you need a get_type function, a copy function and a free function), and a spare pointer or two in case we need to add more fields (I can't think of any, but I'd have said that before we added the "recommended" ones too).

The struct needs documentation; as a minimum, please `make check` with --enable-gtk-doc and add docs in the source, and entries in sections.txt, until it stops complaining.

> +  ar->supported_mime_types = g_strdupv ((GStrv) tp_asv_get_strv (properties,
> +      TP_PROP_CONNECTION_INTERFACE_AVATARS_SUPPORTED_AVATAR_MIME_TYPES));

This doesn't work, and neither do any of the other properties. The key in the a{sv} returned by GetAll isn't namespaced, so you need to use "SupportedAvatarMIMETypes".

It would be nice if you replaced NULL with { NULL } (i.e. an empty GStrv) after retrieving SupportedAvatarMIMETypes, so that you could guarantee to API users that the GStrv was non-NULL.

> +finally:
> +  _tp_proxy_set_feature_prepared (proxy, TP_CONNECTION_FEATURE_AVATAR_REQUIREMENTS,
> +      TRUE);

The third argument should be FALSE if getting the properties failed. You can use (self->priv->avatar_requirements != NULL), probably.

(In reply to comment #3)
> (In reply to comment #2)
> > Note that we also want to know the avatar requirements when offline, so maybe
> > the TpConnectionAvatarRequirements struct could be renamed to
> > TpAvatarRequirements so it can be used in any other APIs like TpProtocol.
> 
> Yes please. Also, it should have a boxed type (look at recent patches that make
> TpIntSet into a boxed type - basically you need a get_type function, a copy
> function and a free function), and a spare pointer or two in case we need to
> add more fields (I can't think of any, but I'd have said that before we added
> the "recommended" ones too).

done

> The struct needs documentation; as a minimum, please `make check` with
> --enable-gtk-doc and add docs in the source, and entries in sections.txt, until
> it stops complaining.

done

> > +  ar->supported_mime_types = g_strdupv ((GStrv) tp_asv_get_strv (properties,
> > +      TP_PROP_CONNECTION_INTERFACE_AVATARS_SUPPORTED_AVATAR_MIME_TYPES));
> 
> This doesn't work, and neither do any of the other properties. The key in the
> a{sv} returned by GetAll isn't namespaced, so you need to use
> "SupportedAvatarMIMETypes".

done

> It would be nice if you replaced NULL with { NULL } (i.e. an empty GStrv) after
> retrieving SupportedAvatarMIMETypes, so that you could guarantee to API users
> that the GStrv was non-NULL.

done

> > +finally:
> > +  _tp_proxy_set_feature_prepared (proxy, TP_CONNECTION_FEATURE_AVATAR_REQUIREMENTS,
> > +      TRUE);
> 
> The third argument should be FALSE if getting the properties failed. You can
> use (self->priv->avatar_requirements != NULL), probably.

I copied from tp_connection_get_rcc_cb() where it also give TRUE even in case of failure. Is that a bug in both functions?

I updated my branch.

I tested that branch in empathy and it works at least with gabble. But butterfly does not implement those properties. I think we need a fallback to the old DBus call.

fyi here is empathy branch: http://git.collabora.co.uk/?p=user/xclaesse/empathy.git;a=shortlog;h=refs/heads/avatar-req

Can't we just fix Butterfly? :-)

(In reply to comment #4)
> I copied from tp_connection_get_rcc_cb() where it also give TRUE even in case
> of failure. Is that a bug in both functions?

RequestableChannelClasses is a bit different, since the Requests interface is mandatory, and if it doesn't work, then you've lost the ability to do Telepathy correctly - anything we do after that is just a matter of how we recover.

For the CMs with Avatars but without these properties:

* Bug #27776 (in Butterfly) has a patch.
* Bug #27777 is the same thing for Sunshine; a very similar patch should work.
* Bug #22304 is the same thing for Haze; it should be easy to do based on Gabble?

(In reply to comment #7)
> (In reply to comment #4)
> > I copied from tp_connection_get_rcc_cb() where it also give TRUE even in case
> > of failure. Is that a bug in both functions?
> 
> RequestableChannelClasses is a bit different, since the Requests interface is
> mandatory, and if it doesn't work, then you've lost the ability to do Telepathy
> correctly - anything we do after that is just a matter of how we recover.

Ok, updated the patch.

> +TP_TYPE_AVATAR_REQUIREMENTS

This should be in a non-Standard, non-Private section: otherwise it's not obvious that a boxed type exists.

> +                            guint minimum_height,
> +                            guint minimum_width,

Would it be more normal to put widths before heights, so they're in the order (x, y)?

> + * Since: 0.11.FUTURE

Our release scripts don't grep for FUTURE. Please use UNRELEASED.

> + * <!--badger-->

Surely you have more to say about TpAvatarRequirements than that? "The requirements for setting an avatar on a particular protocol", perhaps?

(In reply to comment #10)
> > +TP_TYPE_AVATAR_REQUIREMENTS
> 
> This should be in a non-Standard, non-Private section: otherwise it's not
> obvious that a boxed type exists.

done

> > +                            guint minimum_height,
> > +                            guint minimum_width,
> 
> Would it be more normal to put widths before heights, so they're in the order
> (x, y)?

Just copied the list from the spec, but indeed with usually have width before height.

Fixed.

> > + * Since: 0.11.FUTURE
> 
> Our release scripts don't grep for FUTURE. Please use UNRELEASED.

done

(In reply to comment #11)
> > + * <!--badger-->
> 
> Surely you have more to say about TpAvatarRequirements than that? "The
> requirements for setting an avatar on a particular protocol", perhaps?

done.

patch updated.

Ship it.

Thanks.