GNOME Bugzilla – Bug 660128
Most contacts don't have an avatar
Last modified: 2012-09-12 09:51:10 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).
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.
Is this a duplicate of bug #662590?
This may also be related to bug #662616 and bug #662770 -- please double-check when those are fixed.
So is this still an issue?
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.
(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).
(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.
(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)?
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.
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.
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)
(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.
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.
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 :(
(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.
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.
(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.
(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.
So, what are we going with here?
Philip, If your patch fixes both issues Travis and Guillaume pointed out in comments 16 and 17 I'd say ship it.
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?
(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).
(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).
(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).
Oh that's right. That's fine for me then. :)
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(-)
So to make use of this, you need to call IndividualAggregator.unprepare() just before shutting down in Empathy.
Created attachment 219200 [details] [review] add empathy_individual_manager_unprepare_async()
Created attachment 219201 [details] [review] roster-window: unprepare the individual manager before destroying the roster
How does those look to you?
Review of attachment 219200 [details] [review]: Looks good to me.
Review of attachment 219201 [details] [review]: If you’re sure that’s the only code path for quitting Empathy, it looks good to me.
Thanks, I'll merge once the folks bits have been released.
(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.