After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 660128 - Most contacts don't have an avatar
Most contacts don't have an avatar
Status: RESOLVED FIXED
Product: folks
Classification: Platform
Component: general
git master
Other Linux
: Normal enhancement
: Unset
Assigned To: folks-maint
folks-maint
Depends on:
Blocks:
 
 
Reported: 2011-09-26 12:42 UTC by Guillaume Desmottes
Modified: 2012-09-12 09:51 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Persist avatar data from offline to online in the Telepathy backend (18.41 KB, patch)
2012-06-22 18:29 UTC, Philip Withnall
committed Details | Review
avatar fetcher - WARNING: servers are going to hate you (2.84 KB, text/x-csrc)
2012-06-25 07:51 UTC, Guillaume Desmottes
  Details
add empathy_individual_manager_unprepare_async() (3.05 KB, patch)
2012-07-19 08:42 UTC, Guillaume Desmottes
reviewed Details | Review
roster-window: unprepare the individual manager before destroying the roster (1.44 KB, patch)
2012-07-19 08:42 UTC, Guillaume Desmottes
reviewed Details | Review

Description Guillaume Desmottes 2011-09-26 12:42:26 UTC
When you start gnome-contacts for the first time your list of contacts look kinda lame because most contacts don't have an avatar. Actually, in my case at least, only my *online* XMPP contacts had avatars.

I guess that's probably we have to pull avatars from offline contacts in TP. But as Folks now have an avatar cache, maybe gnome-contacts could pull the avatars of all contacts at first run? This would greatly improve the first run experience as loads of contacts will then have an avatar (Facebook contacts at least).
Comment 1 Travis Reitter 2011-09-27 16:47:31 UTC
I thought this issue was related to bug #658531, but apparently that's more focused on storage.

I'll debug a bit to see if it's a Folks bug.
Comment 2 Travis Reitter 2011-10-25 23:48:04 UTC
Is this a duplicate of bug #662590?
Comment 3 Travis Reitter 2011-10-26 16:48:25 UTC
This may also be related to bug #662616 and bug #662770 -- please double-check when those are fixed.
Comment 4 Tobias Mueller 2012-02-11 14:22:38 UTC
So is this still an issue?
Comment 5 Guillaume Desmottes 2012-02-13 08:02:31 UTC
Strictly speaking no as Telepathy contacts are not displayed by default any more (which I think is a bad idea but that's another question).

But when searching those contacts are displayed and most of them don't have an avatar.
Comment 6 Travis Reitter 2012-02-13 17:13:11 UTC
(In reply to comment #5)
> Strictly speaking no as Telepathy contacts are not displayed by default any
> more (which I think is a bad idea but that's another question).

I don't think that's intentional -- we've got an avatar cache in Folks and use that for displaying TpPersona-based Individuals when we're offline.

So, unless gnome-contacts started filtering these Individuals out manually, this sounds like a different bug.

> But when searching those contacts are displayed and most of them don't have an
> avatar.

Even after they've been fetched once? Our cache needs to be populated, but once a TpPersona's avatar is retrieved, it should remain (even when we're offline). Of course, we won't have avatars for TpPersonas who haven't been online since we've installed the first version of Folks that supported the caching (I can't remember exactly which version, but it's been a while).
Comment 7 Guillaume Desmottes 2012-02-14 07:54:19 UTC
(In reply to comment #6)
> (In reply to comment #5)
> > Strictly speaking no as Telepathy contacts are not displayed by default any
> > more (which I think is a bad idea but that's another question).
> 
> I don't think that's intentional -- we've got an avatar cache in Folks and use
> that for displaying TpPersona-based Individuals when we're offline.

Is is, see bug #668347

> > But when searching those contacts are displayed and most of them don't have an
> > avatar.
> 
> Even after they've been fetched once? Our cache needs to be populated, but once
> a TpPersona's avatar is retrieved, it should remain (even when we're offline).
> Of course, we won't have avatars for TpPersonas who haven't been online since
> we've installed the first version of Folks that supported the caching (I can't
> remember exactly which version, but it's been a while).

Indeed, contacts having their avatar in the cache (~/.cache/folks/avatars) are properly displayed. But looking at this cache I don't see any avatar from my Facebook contacts or Telepathy accounts. Actually it looks like only my Google+ contacts have their avatar cached.
Comment 8 Travis Reitter 2012-02-15 22:28:28 UTC
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Strictly speaking no as Telepathy contacts are not displayed by default any
> > > more (which I think is a bad idea but that's another question).
> > 
> > I don't think that's intentional -- we've got an avatar cache in Folks and use
> > that for displaying TpPersona-based Individuals when we're offline.
> 
> Is is, see bug #668347

So, do the Individuals which don't have show avatars actually contain Personas which /do/ have avatars?

> Indeed, contacts having their avatar in the cache (~/.cache/folks/avatars) are
> properly displayed. But looking at this cache I don't see any avatar from my
> Facebook contacts or Telepathy accounts. Actually it looks like only my Google+
> contacts have their avatar cached.

I'll have to investigate more.

In the mean time, is there any hint from the date stamps within ~/.cache/folks/avatars? Did they suddenly stop being updated at some point, or do avatars continue to be updated for your Google+ contacts?

If they did just suddenly stop, could you please try to work out if it corresponds to an update of Folks (maybe some version has a regression)?
Comment 9 Guillaume Desmottes 2012-02-16 08:36:57 UTC
gnome-contacts display avatars for:
- all individuals being linked to a G+ persona (as their avatar is cached)
- individuls beings linked to an *online* Telepathy persona (as it's avatar is available).

Looks like I'm not the only one not having TP avatars in Folks's cache. Frederic Peters told me he was experiencing the same.

The cache doesn't contact *any* avatar from a not G+ contact and has been updated today so I don't think that's the issue.
Comment 10 Guillaume Desmottes 2012-06-18 14:31:12 UTC
Re-assigning to Folks as I'm experiencing this issue with Empathy as well (which is pretty bad when displaying offline contacts as the roster is now full of 'default avatar' icons).

- Start Empathy, X is offline: I don't have his avatar
- X is now online: I do have his avatar
- X is now offline: I still have his avatar, all good
- Restart Empathhy, X is still offline: I don't have this avatar any more

This is with Folks master.
Comment 11 Guillaume Desmottes 2012-06-18 14:38:46 UTC
This can also be reproduced with folks-inspect. When starting and the contact is offline:

  avatar                (null)

and once he's online:

avatar                0x28fda60 (file: file:///home/cassidy/.cache/telepathy/avatars/gabble/jabber/_30913075080dbdec5682938297d1da0216cd51431)
Comment 12 Philip Withnall 2012-06-18 17:42:43 UTC
(In reply to comment #10)
> Re-assigning to Folks as I'm experiencing this issue with Empathy as well
> (which is pretty bad when displaying offline contacts as the roster is now full
> of 'default avatar' icons).
> 
> - Start Empathy, X is offline: I don't have his avatar
> - X is now online: I do have his avatar
> - X is now offline: I still have his avatar, all good
> - Restart Empathhy, X is still offline: I don't have this avatar any more

The problem is that at step 1 the cache file doesn’t contain the URI of X’s avatar. Restarting Empathy (without manually going offline first) doesn’t update the cache file, so the URI of X’s avatar which we got in step 2 never makes it into the cache file.

I guess Tpf.PersonaStore should update the cache when it’s destroyed if any avatars have been modified in the mean time. I’ll see if I can come up with a patch later.
Comment 13 Philip Withnall 2012-06-22 18:29:21 UTC
Created attachment 217042 [details] [review]
Persist avatar data from offline to online in the Telepathy backend

https://www.gitorious.org/folks/folks/commits/660128-tpf-avatars

That was fun. As usual, I forgot to update the NEWS file, so that needs doing before this is pushed.

Criticism of this hacky approach is welcome.
Comment 14 Guillaume Desmottes 2012-06-25 07:51:13 UTC
Created attachment 217166 [details]
avatar fetcher - WARNING: servers are going to hate you 

I hacked this simple app manually fetching avatars of all my TP contacts in
order to populate my cache and test this patch.

Unfortunatelly it doesn't seem to work:
- Start Empathy and display offline contacts
- Your offline Facebook contacts won't have an avatar
- Run my script and freeze your laptop for a short while
- All Facebook contacts now have an avatar
- Restart Empathy
- Their avatar is gone :(
Comment 15 Philip Withnall 2012-06-27 20:10:58 UTC
(In reply to comment #14)
> Created an attachment (id=217166) [details]
> avatar fetcher - WARNING: servers are going to hate you 
> 
> I hacked this simple app manually fetching avatars of all my TP contacts in
> order to populate my cache and test this patch.
> 
> Unfortunatelly it doesn't seem to work:
> - Start Empathy and display offline contacts
> - Your offline Facebook contacts won't have an avatar
> - Run my script and freeze your laptop for a short while
> - All Facebook contacts now have an avatar
> - Restart Empathy
> - Their avatar is gone :(

Thanks for the test case. If I restart Empathy as you say, the avatars disappear as before. However, if I go offline (explicitly) in Empathy first, then restart it, the avatars remain.

This is because folks currently has no way of knowing when to flush things to its cache other than when going offline. (Flushing in the finalize() functions of anything is prone to failure because: the main loop is probably about to disappear and flushing is an async operation; and that makes the cache very prone to reference counting bugs.)

The approach I took in the patch was to add an explicit IndividualAggregator.unprepare() method which Empathy would need to call when quitting. I should’ve mentioned that in my comment, though it is described in the commit messages in the patch.

The alternative would be to flush the cache every x seconds or so (if it’s dirty). I would be happy with either.
Comment 16 Guillaume Desmottes 2012-06-28 07:33:41 UTC
Humm I don't think that's a very good approach as we can be online only using other client (like the Shell only). Which data are stored in this cache? Avatar, alias, contact info?  We could setup a timer each time one of those data change and save the cache when it times out. If the timer is long enough (one or two minutes or something) we shouldn't waste too much time flushing the cache as most of these data don't change much.
Comment 17 Travis Reitter 2012-06-28 15:57:02 UTC
(In reply to comment #16)
> Humm I don't think that's a very good approach as we can be online only using
> other client (like the Shell only). Which data are stored in this cache?
> Avatar, alias, contact info?  We could setup a timer each time one of those
> data change and save the cache when it times out. If the timer is long enough
> (one or two minutes or something) we shouldn't waste too much time flushing the
> cache as most of these data don't change much.

I think, even with the periodic flush, clients should still flush before they completely exit.

Eg, if someone goes online briefly, quits, and then opens the client while they don't have internet access, they'll wonder where all the avatars and aliases went (especially if they'd seen the caching work before on a longer run).

Also, not that this is a critical point, but I foresee an embarrassing/difficult demo moment otherwise. Imagine showing off the cache and not managing to wait long enough for the periodic flush, then re-opening the client and showing it all empty.
Comment 18 Philip Withnall 2012-06-28 20:29:39 UTC
(In reply to comment #16)
> Humm I don't think that's a very good approach as we can be online only using
> other client (like the Shell only). Which data are stored in this cache?
> Avatar, alias, contact info?  We could setup a timer each time one of those
> data change and save the cache when it times out. If the timer is long enough
> (one or two minutes or something) we shouldn't waste too much time flushing the
> cache as most of these data don't change much.

Data stored in the cache: http://git.gnome.org/browse/folks/tree/backends/telepathy/lib/tpf-persona-store-cache.vala#n94
(Basically what you said, plus a little bit more.)

(In reply to comment #17)
> Eg, if someone goes online briefly, quits, and then opens the client while they
> don't have internet access, they'll wonder where all the avatars and aliases
> went (especially if they'd seen the caching work before on a longer run).

If they’ve seen the caching work before on a longer run, then all the data will already be in the cache and there won’t be a problem.
Comment 19 Philip Withnall 2012-07-06 18:14:17 UTC
So, what are we going with here?
Comment 20 Jeremy Whiting 2012-07-06 21:13:00 UTC
Philip,

If your patch fixes both issues Travis and Guillaume pointed out in comments 16 and 17 I'd say ship it.
Comment 21 Guillaume Desmottes 2012-07-09 08:44:38 UTC
If I have to add a unprepare() call to Empathy to have this bug fixed, that's a price I'm ok to pay. :)

Are you also flushing when the connection is disconnected?
Comment 22 Philip Withnall 2012-07-09 20:23:43 UTC
(In reply to comment #21)
> If I have to add a unprepare() call to Empathy to have this bug fixed, that's a
> price I'm ok to pay. :)

Is the flushing on a timeout also necessary then?

> Are you also flushing when the connection is disconnected?

That’s the current behaviour (before and after this patch is applied).
Comment 23 Guillaume Desmottes 2012-07-11 07:47:23 UTC
(In reply to comment #22)
> > Are you also flushing when the connection is disconnected?
> 
> That’s the current behaviour (before and after this patch is applied).

You're right, this works without your patch
- Connect accounts
- Run my script to fetch avatars
-  Disconnect accounts
- Everyone in  gnome-contacts have avatars

That's cool.
But as soon as I reconnect the IM accounts they loose their avatar in gnome-contacts. Your patch is not going to fix that right? (with or without timeout).
Comment 24 Philip Withnall 2012-07-11 20:19:20 UTC
(In reply to comment #23)
> But as soon as I reconnect the IM accounts they loose their avatar in
> gnome-contacts. Your patch is not going to fix that right? (with or without
> timeout).

The first commit in the patch should fix exactly that (with or without timeout).
Comment 25 Guillaume Desmottes 2012-07-12 07:14:51 UTC
Oh that's right. That's fine for me then. :)
Comment 26 Philip Withnall 2012-07-19 03:56:17 UTC
Comment on attachment 217042 [details] [review]
Persist avatar data from offline to online in the Telepathy backend

commit c2338c651edd3fcdc3ef28c94d87351b8c6bae27
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Fri Jun 22 19:17:42 2012 +0100

    telepathy: Preserve Tpf.Persona avatars from the cache for online contacts
    
    This is a fairly hacky way of fixing the problem in bug #660128: maintaining
    a map of IIDs to avatar files for all personas in the Telepathy backend, and
    referring to it in case Telepathy doesn’t know the current avatar for a
    TpContact. This can happen if that contact is currently offline while we’re
    online, for example. This allows use of the avatars cached from last time
    contacts were online, even though those contacts are now offline.
    
    A more comprehensive fix, which I would have implemented if I hadn’t just got
    my degree classification and weren’t just heading out to celebrate, would
    involve rearchitecting the caching of the Telepathy backend so that the
    Tpf.Personas weren’t destroyed and re-created whenever the backend went
    offline/online; instead, they would have their properties updated from the
    cache/online contact list. This would eliminate spurious personas-changed
    signals, unify the code implementing the different properties (currently it’s
    split between the properties and separate implementations in
    Tpf.Persona.from_cache()), and allow property values to persist from offline
    to online without the need for hacks like this.
    
    Closes: https://bugzilla.gnome.org/show_bug.cgi?id=660128

 backends/telepathy/lib/tpf-persona-store.vala |   35 +++++++++++++++++++
 backends/telepathy/lib/tpf-persona.vala       |   46 ++++++++++++++++++++++---
 2 files changed, 76 insertions(+), 5 deletions(-)

commit 36ecd01a58b952c40c985166aa74ededda7fcefc
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Fri Jun 22 19:09:57 2012 +0100

    telepathy: Allow for updated Tpf.Personas properties to update the cache
    
    Currently, the cache is only written when going offline, which means that any
    properties of Tpf.Personas which are updated at runtime aren’t written to the
    cache unless the client explicitly goes offline before being closed. This
    doesn’t often happen, meaning the property updates are lost and the cache
    becomes stale.
    
    This commit keeps track of the clean/dirty state of the cache and writes it
    out when PersonaStore.flush() is called. It also adds a new method,
    IndividualAggregator.unprepare(), which ensures all the persona stores handled
    by the aggregator are flushed. Clients should call this method before closing
    their main loop.
    
    (Calling flush() in the finalise function of the PersonaStore doesn’t work
    because Tpf.PersonaStores are often never finalised due to the implementation
    of Tpf.PersonaStore.dup_for_account(). Furthermore, calling flush() in the
    finalise function of the IndividualAggregator doesn’t work because the client
    will typically quit the main loop immediately afterwards, which will cancel
    the asynchronous flush call.)
    
    New API:
     • IndividualAggregator.unprepare()
    
    Helps: https://bugzilla.gnome.org/show_bug.cgi?id=660128

 NEWS                                          |    2 +
 backends/telepathy/lib/tpf-persona-store.vala |   72 +++++++++++++++++++-----
 backends/telepathy/lib/tpf-persona.vala       |   24 ++++++++
 folks/individual-aggregator.vala              |   43 +++++++++++++++
 4 files changed, 126 insertions(+), 15 deletions(-)

commit 6f743fdc074910ff8f4d0b91753e82a0b2c92867
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Fri Jun 22 19:03:55 2012 +0100

    core: Don’t keep references to PersonaStores in each Individual
    
    Pointless and it makes debugging reference counting bugs a real pain.

 folks/individual.vala |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Comment 27 Philip Withnall 2012-07-19 03:57:11 UTC
So to make use of this, you need to call IndividualAggregator.unprepare() just before shutting down in Empathy.
Comment 28 Guillaume Desmottes 2012-07-19 08:42:10 UTC
Created attachment 219200 [details] [review]
add empathy_individual_manager_unprepare_async()
Comment 29 Guillaume Desmottes 2012-07-19 08:42:13 UTC
Created attachment 219201 [details] [review]
roster-window: unprepare the individual manager before destroying the roster
Comment 30 Guillaume Desmottes 2012-07-19 08:42:48 UTC
How does those look to you?
Comment 31 Philip Withnall 2012-07-19 18:25:34 UTC
Review of attachment 219200 [details] [review]:

Looks good to me.
Comment 32 Philip Withnall 2012-07-19 18:26:00 UTC
Review of attachment 219201 [details] [review]:

If you’re sure that’s the only code path for quitting Empathy, it looks good to me.
Comment 33 Guillaume Desmottes 2012-07-20 07:33:18 UTC
Thanks, I'll merge once the folks bits have been released.
Comment 34 Guillaume Desmottes 2012-09-12 09:51:10 UTC
(In reply to comment #33)
> Thanks, I'll merge once the folks bits have been released.

Looks like I forgot to merged this to Emapthy. It's in master now.