Bug 52938 - TpAccount is missing an avatar-changed signal
Summary: TpAccount is missing an avatar-changed signal
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: tp-glib (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Telepathy bugs list
QA Contact: Telepathy bugs list
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-30 06:29 UTC by Xavier Claessens
Modified: 2013-10-23 15:04 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
simple-account: add test API to change the avatar (3.14 KB, patch)
2013-10-23 12:56 UTC, Guillaume Desmottes
Details | Splinter Review
simple-account: claim that we support Avatar (745 bytes, patch)
2013-10-23 12:56 UTC, Guillaume Desmottes
Details | Splinter Review
add TpAccount::avatar-changed signal (3.38 KB, patch)
2013-10-23 12:56 UTC, Guillaume Desmottes
Details | Splinter Review

Description Xavier Claessens 2012-07-30 06:29:43 UTC
There is no high-level API for http://telepathy.freedesktop.org/spec/Account_Interface_Avatar.html#Signal:AvatarChanged

We already have tp_account_get_avatar_async() though.
Comment 1 Guillaume Desmottes 2013-10-23 09:44:02 UTC
I'd like to fix this to simplify Empathy code before porting to next.

My idea was to:
- add TP_ACCOUNT_FEATURE_AVATAR, when preparing this feature connect the 'AvatarChanged' signal and fetch the initial avatar
- Expose the avatar as a TpAccount property
- Fire a property change notification when the avatar is changed

Question: how should we expose the avatar on the TpAccount?
- a (GFile, const gchar *mime_type) as TpContact does (2 properties)? But then we should write the file to disk first.
- a (GArray, const gchar *mime_type) to stay coherent with tp_account_get_avatar_finish()?
- a GVariant packing these 2 values into one?
- something else?
Comment 2 Simon McVittie 2013-10-23 11:37:13 UTC
(In reply to comment #1)
> I'd like to fix this to simplify Empathy code before porting to next.

Does Empathy really want to be tracking the avatar (= pull it into memory whenever it changes) *all the time*, or only when the account editor is actually open?

> Question: how should we expose the avatar on the TpAccount?
> - a (GFile, const gchar *mime_type) as TpContact does (2 properties)? But
> then we should write the file to disk first.

I don't think this should happen unless we replace A.I.Avatar with something URI-based at the D-Bus level (Bug #33409). Random UIs copying the avatar to a separate file on disk seems rather pointless.

> - a (GArray, const gchar *mime_type) to stay coherent with
> tp_account_get_avatar_finish()?
> - a GVariant packing these 2 values into one?

Or a GBytes and a string, maybe? But I don't think random UIs keeping our avatar in memory is desirable, if they're not actually actively using it.

Until Bug #33409 is done, I'd be tempted to do the straightforward binding of the current D-Bus API:

    /**
     * TpAccount::avatar-changed:
     *
     * Emitted when the avatar changes. Call tp_account_get_avatar_async()
     * to get the new avatar data.
     */

If you're going to load a GFile from disk, with current GIO best-practices it's going to be an async operation *anyway*... so we might as well use the current D-Bus API?
Comment 3 Simon McVittie 2013-10-23 11:38:18 UTC
(In reply to comment #2)
> (In reply to comment #1)
> > I'd like to fix this to simplify Empathy code before porting to next.
> 
> Does Empathy really want to be tracking the avatar (= pull it into memory
> whenever it changes) *all the time*, or only when the account editor is
> actually open?

To be clear, my objection here is that features are non-revocable: once you've started, you can't stop. I'm not sure that's a great fit for monitoring accounts' avatars.
Comment 4 Guillaume Desmottes 2013-10-23 12:25:25 UTC
(In reply to comment #2)
> (In reply to comment #1)
> > I'd like to fix this to simplify Empathy code before porting to next.
> 
> Does Empathy really want to be tracking the avatar (= pull it into memory
> whenever it changes) *all the time*, or only when the account editor is
> actually open?

Indeed, only when empathy-accounts (or the equivalent GOA UI) is opened.


>     /**
>      * TpAccount::avatar-changed:
>      *
>      * Emitted when the avatar changes. Call tp_account_get_avatar_async()
>      * to get the new avatar data.
>      */

You're right, that's enough for now, I'll do that.
Comment 5 Guillaume Desmottes 2013-10-23 12:56:13 UTC
Created attachment 88031 [details] [review]
simple-account: add test API to change the avatar
Comment 6 Guillaume Desmottes 2013-10-23 12:56:30 UTC
Created attachment 88032 [details] [review]
simple-account: claim that we support Avatar
Comment 7 Guillaume Desmottes 2013-10-23 12:56:46 UTC
Created attachment 88033 [details] [review]
add TpAccount::avatar-changed signal
Comment 8 Simon McVittie 2013-10-23 14:05:34 UTC
++ for those patches.
Comment 9 Guillaume Desmottes 2013-10-23 15:04:00 UTC
Merged to master, will be in 0.23.0.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.