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 671662 - Renamed alias of Facebook contact is not saved
Renamed alias of Facebook contact is not saved
Status: RESOLVED FIXED
Product: folks
Classification: Platform
Component: general
0.6.x
Other Linux
: Normal normal
: Unset
Assigned To: folks-maint
folks-maint
Depends on:
Blocks:
 
 
Reported: 2012-03-08 16:11 UTC by Reuben Thomas
Modified: 2012-04-18 14:21 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Correctly report writeability of aliases and groups in Tpf.Persona (36.22 KB, patch)
2012-03-26 16:07 UTC, Philip Withnall
none Details | Review
Correctly report writeability of aliases and groups in Tpf.Persona (updated) (22.12 KB, patch)
2012-04-16 16:39 UTC, Philip Withnall
none Details | Review
Correctly report writeability of aliases and groups in Tpf.Persona (updated again) (26.08 KB, patch)
2012-04-16 22:34 UTC, Philip Withnall
none Details | Review
Correctly report writeability of aliases and groups in Tpf.Persona (updated again) (23.04 KB, patch)
2012-04-17 11:02 UTC, Philip Withnall
committed Details | Review

Description Reuben Thomas 2012-03-08 16:11:02 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.
Comment 1 Guillaume Desmottes 2012-03-09 11:21:56 UTC
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
Comment 2 Reuben Thomas 2012-03-09 12:13:32 UTC
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...
Comment 3 Travis Reitter 2012-03-14 17:42:23 UTC
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).
Comment 4 Philip Withnall 2012-03-26 16:07:56 UTC
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.
Comment 5 Xavier Claessens 2012-04-16 15:17:49 UTC
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.
Comment 6 Philip Withnall 2012-04-16 16:39:27 UTC
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!
Comment 7 Philip Withnall 2012-04-16 22:34:00 UTC
Created attachment 212175 [details] [review]
Correctly report writeability of aliases and groups in Tpf.Persona (updated again)
Comment 8 Philip Withnall 2012-04-16 22:36:34 UTC
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.
Comment 9 Philip Withnall 2012-04-17 11:02:16 UTC
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
Comment 10 Xavier Claessens 2012-04-17 11:05:24 UTC
if this is the same as in comment #7 but without the SetAliases code, then +1 (I was already reviewing that patch)
Comment 11 Philip Withnall 2012-04-17 11:19:48 UTC
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(-)
Comment 12 Reuben Thomas 2012-04-18 14:21:54 UTC
Thanks all for your excellent work on this!