GNOME Bugzilla – Bug 671662
Renamed alias of Facebook contact is not saved
Last modified: 2012-04-18 14:21:54 UTC
When I change the alias of a Facebook contact in Empathy, the change is shown in the Contact List, but when I quit and restart Empathy, the change is undone. When I rename contacts with other protocols, or even a linked contact containing a Facebook contact, the alias rename persists across a restart of Empathy. I'm using 3.3.5.
That's because Facebook's XMPP servers doesn't allow us to set aliases. But even so, maybe Folks should set the alias locally anyway? Should it always do it or only if the Telepathy connection doesn't support setting aliases? Note that atm Gabble claims that's supported; see https://bugs.freedesktop.org/show_bug.cgi?id=26823
The most sensible thing from a user interface point of view seems to me to act in the same way as when the Facebook contact is part of a linked contact: change the alias and store that information in Empathy, so for the user it just looks like they changed the alias. Alternatively, raise an error, advising the user that the alias can't be changed. But then users won't understand why it works for linked contacts...
I think the best general solution to not being able to modify details on one Persona (like a Telepathy contact) would be to create a new Persona in the primary PersonaStore and link it to the original Persona. The new details can be added to that new primary Persona and everything will be reflected transparently in the UIs. It would be a little different in a case like this, though, where it would create a new primary Persona in response to the error from Facebook. That seems a little ugly. Maybe it would be better to track expected failures (like setting the alias on a Facebook XMPP Persona) and follow the process above and just continue treating failures as failures (not as opportunities to create new Personas).
Created attachment 210640 [details] [review] Correctly report writeability of aliases and groups in Tpf.Persona Here's a patch which fixes all the problems in the Telepathy backend relating to this (as far as I can see). Branch available here: https://www.gitorious.org/folks/folks/commits/671662-tp-property-feedback It: • fixes change_alias(), change_group(), change_groups() and change_is_favourite() to only return once the underlying Telepathy operation is complete, and correctly propagate errors to the caller; • stops Tpf.Persona updating property values (aliases and groups) before the underlying Telepathy operation has completed, so the UI doesn't get updated until the change has successfully completed on the server; and • fixes reporting of writeable-properties in Tpf.Persona to only say aliases and groups are writeable if Telepathy says they are. Even with these fixes applied, the bug isn't fixed: changing the alias of a Facebook contact still appears successful and then gets undone when Empathy is restarted. I think the remaining fixes needed are: • Gabble needs to correctly report writeability of aliases (https://bugs.freedesktop.org/show_bug.cgi?id=26823). • Empathy might want to call Folks.IndividualAggregator.ensure_individual_property_writeable(my_individual, "alias") before trying to change aliases. This will create (and link) a persona in the primary persona store if necessary, to try and ensure the updated alias will be stored *somewhere*. This is what Travis was suggesting in comment #3.
The patch looks good overall. But I don't think you should translate error messages. Also this won't apply on master, but the good news is that on master this patch will be muuuuuuch easier.
Created attachment 212156 [details] [review] Correctly report writeability of aliases and groups in Tpf.Persona (updated) https://www.gitorious.org/folks/folks/commits/671662-tp-property-feedback-rebase1 So much cleaner now that contact-list has landed!
Created attachment 212175 [details] [review] Correctly report writeability of aliases and groups in Tpf.Persona (updated again)
This is: https://www.gitorious.org/folks/folks/commits/671662-tp-property-feedback-rebase2 Now with SetAliases implemented in the Tp fake CM, so that the individual-properties test works again. Also is-favourite is now only marked as writeable if the Telepathy logger can be connected to. Finally, I changed the logger warning to a debug message, since users will get proper errors from attempting to change is-favourite anyway if the logger isn’t running.
Created attachment 212192 [details] [review] Correctly report writeability of aliases and groups in Tpf.Persona (updated again) Rebased against folks master from this morning, which contains Xavier’s implementation of SetAliases in the Tp fake CM. https://www.gitorious.org/folks/folks/commits/671662-tp-property-feedback-rebase3
if this is the same as in comment #7 but without the SetAliases code, then +1 (I was already reviewing that patch)
Merged to master. Folks now does the best it can by only updating aliases (etc.) when notified by the CM. However, the bug is still present because the Gabble CM still advertises Facebook connections as having writeable aliases. That’s https://bugs.freedesktop.org/show_bug.cgi?id=26823. commit 55c37843df726147bff087311f261fdf5f317b2e Author: Philip Withnall <philip@tecnocode.co.uk> Date: Mon Apr 16 18:11:31 2012 +0100 telepathy: Only allow is-favourite to be writeable if the logger initialised This prevents setting favourites falling into a black hole. backends/telepathy/lib/tpf-persona-store.vala | 16 ++++++++++++++-- tests/telepathy/individual-properties.vala | 8 ++++++-- 2 files changed, 20 insertions(+), 4 deletions(-) commit 834cb05e09c75d1ed0100af3a495217b04a20670 Author: Philip Withnall <philip@tecnocode.co.uk> Date: Mon Mar 26 16:58:50 2012 +0100 telepathy: Correctly advertise the writeability of alias and group properties Previously, Tpf.Personas were always advertising aliases and groups as being writeable properties, when they're actually not writeable for Facebook XMPP connections. Aliases were previously not writeable in the Telepathy fake CM (so the test was previously in error), but they’re now writeable, so everything’s fine. Closes: https://bugzilla.gnome.org/show_bug.cgi?id=671662 NEWS | 1 + backends/telepathy/lib/tpf-persona-store.vala | 70 ++++++++++++++----------- backends/telepathy/lib/tpf-persona.vala | 62 +++++++++++----------- 3 files changed, 70 insertions(+), 63 deletions(-) commit 74bb1d2dc0f8e891cc3c90ddb5e4002ed4b7db2e Author: Philip Withnall <philip@tecnocode.co.uk> Date: Mon Mar 26 16:21:56 2012 +0100 telepathy: Fix notification of is-favourite changes This ensures that the UI reflects errors in changing the is-favourite status of a Tpf.Persona, rather than just blindly assuming they succeed. backends/telepathy/lib/tpf-persona.vala | 9 +++++++-- 1 files changed, 7 insertions(+), 2 deletions(-) commit 46ca881a18febf9848ea96379cfd3ee004c6ab6f Author: Philip Withnall <philip@tecnocode.co.uk> Date: Mon Mar 26 15:07:31 2012 +0100 telepathy: Make group changes truly asynchronous and report errors properly Take advantage of GroupDetails.change_group() being async, and wait for the underlying Telepathy operation to complete before returning from it. This allows us to propagate errors properly, rather than just printing them as warnings on the terminal. This also includes changes to not notify of changes to Tpf.Persona.groups until Telepathy has notified us of the change. This should prevent groups changing in the UI if the underlying operation has actually failed. Helps: https://bugzilla.gnome.org/show_bug.cgi?id=671662 backends/telepathy/lib/tpf-persona-store.vala | 24 ------------- backends/telepathy/lib/tpf-persona.vala | 45 +++++++++++++++++++----- 2 files changed, 35 insertions(+), 34 deletions(-) commit 9a7015cac1fedee8154bce6d267a0a0a89a6d40b Author: Philip Withnall <philip@tecnocode.co.uk> Date: Mon Mar 26 15:00:49 2012 +0100 telepathy: Remove some unused internal signals backends/telepathy/lib/tpf-persona-store.vala | 3 --- 1 files changed, 0 insertions(+), 3 deletions(-) commit 197cda29d89b59cfdaafd8eda6754f82d93feb05 Author: Philip Withnall <philip@tecnocode.co.uk> Date: Mon Mar 26 14:56:21 2012 +0100 telepathy: Make alias changes truly asynchronous and report errors properly Take advantage of AliasDetails.change_alias() being async, and wait for the underlying Telepathy operation to complete before returning from it. This allows us to propagate errors properly, rather than just printing them as warnings on the terminal. This also includes changes to not notify of changes to Tpf.Persona.alias until Telepathy has notified us of the change. This should prevent aliases changing in the UI if the underlying operation has actually failed. Helps: https://bugzilla.gnome.org/show_bug.cgi?id=671662 backends/telepathy/lib/tp-lowlevel.c | 65 ++++++++++++++++++++++--- backends/telepathy/lib/tp-lowlevel.h | 11 ++++- backends/telepathy/lib/tpf-persona-store.vala | 19 +++++-- backends/telepathy/lib/tpf-persona.vala | 3 +- 4 files changed, 81 insertions(+), 17 deletions(-)
Thanks all for your excellent work on this!