Bug 31757

(If you're not going to be working on this beyond proof-of-concept, let me know and I can pick it up.)

> +haze_protocol_get_avatar_details (TpBaseProtocol *base,

This duplicates the logic from _get_acceptable_mime_types in connection-avatars.c, complete with the noted bug about image/ico. I'd prefer it if you added a function like this:

GStrv haze_prpl_info_get_mime_types (PurplePluginProtocolInfo *prpl_info)
{
  ...
}

(I'd personally put it in protocol.[ch] but that's a matter of taste) and then had both _get_acceptable_mime_types and haze_protocol_get_avatar_details call it.

Bonus penguin points if you add a special case to fix the documented bug too: Windows-style .ico files are officially of MIME type "image/vnd.microsoft.icon".

*** Bug 36169 has been marked as a duplicate of this bug. ***

Guillaume wrote:

> http://telepathy.freedesktop.org/spec/Protocol_Interface_Avatars.html
>
> I *think* it's just a matter of:
> - implement TpBaseProtocolClass->get_avatar_details
> - update TpBaseProtocolClass->get_interfaces
> - update the manager file

Created attachment 86040 [details] [review]
protocol: claim that we implement Avatars

Created attachment 86041 [details] [review]
avatars: factor out dup_mime_types()

Created attachment 86042 [details] [review]
avatars: factor out haze_connection_get_icon_spec_requirements()

Created attachment 86043 [details] [review]
protocol: implement get_avatar_details()

(In reply to comment #3)
> Guillaume wrote:
> 
> > http://telepathy.freedesktop.org/spec/Protocol_Interface_Avatars.html
> >
> > I *think* it's just a matter of:
> > - implement TpBaseProtocolClass->get_avatar_details
> > - update TpBaseProtocolClass->get_interfaces
> > - update the manager file

It was basically as simple as this (modulo some refactoring to share code with connection-avatar) except the manager part as we don't ship one any more.

Comment on attachment 86040 [details] [review]
protocol: claim that we implement Avatars

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

::: tests/twisted/cm/protocols.py
@@ +30,5 @@
>  
> +        # Protocol is supposed to implement Interface.Avatars iff the
> +        # connection implements Avatars as well.
> +        if cs.CONN_IFACE_AVATARS in flat_props['ConnectionInterfaces']:
> +            assertEquals([cs.PROTOCOL_IFACE_AVATARS], props[cs.PROTOCOL + '.Interfaces'])

OK, but you'll probably want to turn this into assertContains...

@@ +32,5 @@
> +        # connection implements Avatars as well.
> +        if cs.CONN_IFACE_AVATARS in flat_props['ConnectionInterfaces']:
> +            assertEquals([cs.PROTOCOL_IFACE_AVATARS], props[cs.PROTOCOL + '.Interfaces'])
> +        else:
> +            assertEquals([], props[cs.PROTOCOL + '.Interfaces'])

... and this into assertNotContains or whatever it's called.

Comment on attachment 86041 [details] [review]
avatars: factor out dup_mime_types()

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

OK

Comment on attachment 86042 [details] [review]
avatars: factor out haze_connection_get_icon_spec_requirements()

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

OK with a trivial change:

::: src/connection-avatars.c
@@ +170,5 @@
> +    guint *max_width,
> +    guint *max_bytes)
> +{
> +    if (mime_types != NULL)
> +      *mime_types = dup_mime_types (icon_spec);

Indent 2 more spaces, if that's the indentation convention you're using (or indenting this function in Telepathy style would also be OK, IMO). No need to ask for re-review for that.

Comment on attachment 86043 [details] [review]
protocol: implement get_avatar_details()

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

::: src/protocol.c
@@ +1003,5 @@
> +  PurpleBuddyIconSpec *icon_spec;
> +
> +  icon_spec = &(self->priv->prpl_info->icon_spec);
> +
> +  if (icon_spec == NULL || icon_spec->format == NULL)

icon_spec will never be NULL: it's the address of something?

::: tests/twisted/cm/protocols.py
@@ +173,5 @@
> +            assertEquals(32, protocol_avatar_props['MinimumAvatarHeight'])
> +            assertEquals(32, protocol_avatar_props['MinimumAvatarWidth'])
> +            assertEquals(0, protocol_avatar_props['RecommendedAvatarHeight'])
> +            assertEquals(0, protocol_avatar_props['RecommendedAvatarWidth'])
> +            assertEquals(['image/png'], protocol_avatar_props['SupportedAvatarMIMETypes'])

assertContains would be more future-proof?

(In reply to comment #12)
> Comment on attachment 86043 [details] [review] [review]
> protocol: implement get_avatar_details()
> 
> Review of attachment 86043 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: src/protocol.c
> @@ +1003,5 @@
> > +  PurpleBuddyIconSpec *icon_spec;
> > +
> > +  icon_spec = &(self->priv->prpl_info->icon_spec);
> > +
> > +  if (icon_spec == NULL || icon_spec->format == NULL)
> 
> icon_spec will never be NULL: it's the address of something?

You're right, I copied it from old code that I changed afterwards. Fixed.

> ::: tests/twisted/cm/protocols.py
> @@ +173,5 @@
> > +            assertEquals(32, protocol_avatar_props['MinimumAvatarHeight'])
> > +            assertEquals(32, protocol_avatar_props['MinimumAvatarWidth'])
> > +            assertEquals(0, protocol_avatar_props['RecommendedAvatarHeight'])
> > +            assertEquals(0, protocol_avatar_props['RecommendedAvatarWidth'])
> > +            assertEquals(['image/png'], protocol_avatar_props['SupportedAvatarMIMETypes'])
> 
> assertContains would be more future-proof?

How so? We want to check the actual value, not just that the key is there.

Created attachment 86062 [details] [review]
86040: protocol: claim that we implement Avatars

Created attachment 86063 [details] [review]
protocol: implement get_avatar_details()

Comment on attachment 86062 [details] [review]
86040: protocol: claim that we implement Avatars

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

++

Comment on attachment 86063 [details] [review]
protocol: implement get_avatar_details()

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

OK.

What I meant about the assertion was: perhaps we should be asserting ("image/png" in protocol_avatar_props['SupportedAvatarMIMETypes']) rather than asserting that it's the entire list. Seems fine though, we can change it if Pidgin does.

Merged to master.