Bug 31686

This is clearly good enough to not block undrafting; thanks.

I'd like a proof-of-concept branch for at least Gabble (the trivial case, where avatar sizes are constant), and preferably also Haze (the non-trivial case, where you can get avatar sizes from prpl_info - see connection-avatars.c).

> +    <tp:added version="0.19.14">(as stable API)</tp:added>

No, it wasn't.

(But this will conflict with the actual undraft when merged, and we can use the correct one as the merge resolution; so, no big deal.)

> + * @max_height: (out): used to return the maximum height in pixels of an
> + *  avatar on this protocol, which may be 0
> + * @max_width: (out): used to return the maximum width in pixels of an avatar
> + *  on this protocol, which may be 0

I think you mean "... on this protocol, or 0 if there is no limit"?

> +  AvatarSpecs *avatar_specs;

Why a pointer? If this was just a struct, it'd automatically be allocated as part of the private struct - less fragmentation. You'd access it like "self->priv->avatar_specs.min_height".

> +    PPA_SUPPORTED_AVATAR_MIME_TYPES,

Surely ProtocolAvatarProp abbreviates to PAP, not PPA?

> +  void (*get_avatar_details) (TpBaseProtocol *self,

If you're going to make a typedef for documentation purposes (as you did here), please use it - "TpBaseProtocolGetAvatarDetailsFunc get_avatar_details;"

>        gchar **vcard_field);
> 
> +  void (*get_avatar_details) (TpBaseProtocol *self,
[...]
> +      guint *max_bytes);
> +
>    const TpPresenceStatusSpec * (*get_statuses) (TpBaseProtocol *self);

Adding a member to a struct is an ABI break. You need to put get_avatar_details after get_statuses, and reduce the number of spare callbacks by 1 at the same time.

I'd personally also use "rec" instead of "recommended" in variable names, but that's a matter of taste.

(In reply to comment #1)
> > +    <tp:added version="0.19.14">(as stable API)</tp:added>
> 
> No, it wasn't.
> 
> (But this will conflict with the actual undraft when merged, and we can use the
> correct one as the merge resolution; so, no big deal.)
> 

I get confused as to how/when this gets changed.
8dd94a3

> > + * @max_height: (out): used to return the maximum height in pixels of an
> > + *  avatar on this protocol, which may be 0
> > + * @max_width: (out): used to return the maximum width in pixels of an avatar
> > + *  on this protocol, which may be 0
> 
> I think you mean "... on this protocol, or 0 if there is no limit"?
> 

I do. e4db32d

> > +  AvatarSpecs *avatar_specs;
> 
> Why a pointer? If this was just a struct, it'd automatically be allocated as
> part of the private struct - less fragmentation. You'd access it like
> "self->priv->avatar_specs.min_height".

Good point! 34b8057

> 
> > +    PPA_SUPPORTED_AVATAR_MIME_TYPES,
> 
> Surely ProtocolAvatarProp abbreviates to PAP, not PPA?
> 

Um, yeah. e879ad7

> > +  void (*get_avatar_details) (TpBaseProtocol *self,
> 
> If you're going to make a typedef for documentation purposes (as you did here),
> please use it - "TpBaseProtocolGetAvatarDetailsFunc get_avatar_details;"
> 

Sure. The other virtual methods don't use the typedef. 3b15af9

> >        gchar **vcard_field);
> > 
> > +  void (*get_avatar_details) (TpBaseProtocol *self,
> [...]
> > +      guint *max_bytes);
> > +
> >    const TpPresenceStatusSpec * (*get_statuses) (TpBaseProtocol *self);
> 
> Adding a member to a struct is an ABI break. You need to put get_avatar_details
> after get_statuses, and reduce the number of spare callbacks by 1 at the same
> time.

Dopes 3b15af9
> 
> I'd personally also use "rec" instead of "recommended" in variable names, but
> that's a matter of taste.

3bbe64d

(In reply to comment #1)
> This is clearly good enough to not block undrafting; thanks.
> 
> I'd like a proof-of-concept branch for at least Gabble (the trivial case, where
> avatar sizes are constant), and preferably also Haze (the non-trivial case,
> where you can get avatar sizes from prpl_info - see connection-avatars.c).
> 

Here is a POC:
http://git.collabora.co.uk/?p=user/eitan/telepathy-haze.git;a=shortlog;h=refs/heads/protocol-avatar-props

(In reply to comment #3)
> Here is a POC:
> http://git.collabora.co.uk/?p=user/eitan/telepathy-haze.git;a=shortlog;h=refs/heads/protocol-avatar-props

Thanks, I've turned that into Bug #31757. The difficult case (Haze) is clearly implementable, so, not a blocker.

> Added avatar protocol test.

This is good to have, but I'm not entirely happy about including P.I.Avatars in an otherwise minimal example of Messages (whose connections do not, in fact, implement Avatars). Perhaps the Protocol test could subclass the object from the Messages CM, and use the subclass?

Alternatively, open a bug for doing that and move on; it'd be a pretty silly reason to block undrafting :-)

The ideal solution would be to have a separate Avatars example one day, and use that for this test (certainly not a merge blocker).

> + * @rec_height: (out): used to return the rec height in pixels

What I meant was:

     * @rec_height: (out): used to return the recommended height in pixels

Other than those two things, I think this is good for merge as soon as we've integrated a spec in which it's undrafted.

(In reply to comment #5)
> > Added avatar protocol test.
> 
> This is good to have, but I'm not entirely happy about including P.I.Avatars in
> an otherwise minimal example of Messages (whose connections do not, in fact,
> implement Avatars). Perhaps the Protocol test could subclass the object from
> the Messages CM, and use the subclass?
> 

The generic protocol properties are tested using this CM in the protocol-objects.c test, so adding the avatar interface to it was natural. If anything, the entire TpBaseProtocol test should be factored out.

> Alternatively, open a bug for doing that and move on; it'd be a pretty silly
> reason to block undrafting :-)

I think I'll do that.

> 
> The ideal solution would be to have a separate Avatars example one day, and use
> that for this test (certainly not a merge blocker).

Or a separate Protocol example..

> 
> > + * @rec_height: (out): used to return the rec height in pixels
> 
> What I meant was:
> 
>      * @rec_height: (out): used to return the recommended height in pixels
> 
> Other than those two things, I think this is good for merge as soon as we've
> integrated a spec in which it's undrafted.

We need to wait for a spec release for that? In the meantime, I rebased the branch and made it ready for a clean merge.

I merged this. Fixed in git for 0.13.7.