GNOME Bugzilla – Bug 628376
Unlinking/editing links for an Individual is complicated
Last modified: 2011-08-29 10:12:37 UTC
Sjoerd brought up a few issues with the current workflow to adjust links for an Individual: 1. the Unlink button seems slightly out-of-place in the Edit dialog and has no context for what it means 2. removing a contact from an Individual requires finding the Unlink button, knowing what that means, and then linking together all the other contacts that you want in the final Individual Solution: A. Add checkboxes to the list of Personas on the right side of the Link dialog. Unchecking a Persona adjusts the previewed Individual above the list and changes the checkmark for the source Individual(s) in the left-side list to minus (-) signs. B. Don't make the originally-selected Individual's checkbox insensitive so it can be fully removed (partly to satisfy C.) C. Remove the unlink button from the Edit dialog. The equivalent could be achieved by opening the "Link…" item (which would be renamed in this case - I'll file a follow-up bug) and unchecking all of the Individuals. I think that's a fairly rare use case anyhow (vs. removing one or two Personas from an Individual) ======================== Since this would require some UI adjustments, we'll need to request permission from the Release Team and notify the Documentation Team.
Philip, I know there were reasons why we didn't do linking and unlinking in a single place before - could you list those again? All I can think of is needing to maintain a temporary Individual until hitting the Link button (in a way that the Aggregator won't notify other Folks users of the new Individual until the dialog is resolved). How much would we even need to add infrastructure to the Aggregator to support the preview Individual? Just expose Individual.replace() through the Aggregator? It seems like we could just build up our own Individual from the selected Personas, and then upon hitting Link, do the equivalent of calling Individual.replace() for each of the existing Individuals (including any unselected that were originally-selected).
Agreed we should improve that for 2.32.
(In reply to comment #0) > 1. the Unlink button seems slightly out-of-place in the Edit dialog and has no > context for what it means I suggest changing its label to "Unlink Contacts" and ensuring it's mentioned in the documentation. > 2. removing a contact from an Individual requires finding the Unlink button, > knowing what that means, and then linking together all the other contacts that > you want in the final Individual I disagree with this approach. It's far too close to release to be making large UI and code changes like this — we're two weeks into UI freeze and two weeks away from the final release, and we're still fixing bugs in the original implementation of the linking UI. Furthermore, I don't think there's actually that much need to complicate the linking dialogue with unlinking functionality. Unlinking is something which should hardly ever be used; I imagine it would be used only to fix one or two mistakes made during linking. Conversely, linking is something which will be used a lot initially (when one first links one's contacts together), and then hardly at all. As I see it, the linking UI should be optimised for linking, and unlinking should be a secondary feature which is there when you need it, but not in the way otherwise. Adding another set of checkboxes in the linking UI is therefore just going to get in the way and be confusing for the vast majority of times the dialogue's used. (In reply to comment #1) > Philip, I know there were reasons why we didn't do linking and unlinking in a > single place before - could you list those again? It would require a way to deal with "anti-links" in libfolks; not just adding them, but ensuring they're up-to-date and/or removed/changed correctly when links are added and removed. This is necessary because otherwise you could open the linking dialogue on an Individual containing Personas which have been auto-linked (e.g. the same contact added on two accounts), and unticking one of the Personas would cause absolutely nothing to happen, since (I imagine) it would get immediately re-linked to the same Individual. > All I can think of is needing to maintain a temporary Individual until hitting > the Link button (in a way that the Aggregator won't notify other Folks users of > the new Individual until the dialog is resolved). That's already the case. See EmpathyIndividualLinker.new_individual. Note that this is a little trickier than it sounds, since one needs to be very careful not to cause any changes on the Individual (such as alias changes) which would be propagated down to its Personas (and then actually pushed to a backing store). See bug #628131, for example. > How much would we even need to add infrastructure to the Aggregator to support > the preview Individual? Just expose Individual.replace() through the > Aggregator? We wouldn't need to add any extra API for the preview Individual, since this is what's done (and supported) already. We just change the Individual:personas property of our temporary Individual, and let the Individual::personas-changed signal update the linking dialogue's UI. Infrastructure would need to be added to support unlinking in the same dialogue though, since you're moving from an operation where the output (set of Personas to form an Individual) is a superset of the input (set of Personas in the start Individual); to an operation where the two sets could be anything from equal to disjoint. I guess the best way to do that would be to change IndividualAggregator.link_personas() to remove each Persona it's given from the Persona's current Individual, then link all the Personas together as it currently does. This assumes that the signalling magic emitted when a Persona is removed from an Individual (i.e. Individual::personas-changed) would correctly cause all the necessary updates around the Individuals which have provided Personas to the new Individual — something that would probably need some work. This would require changing IndividualAggregator.unlink_individual(), or adding an IndividualAggregator.unlink_personas(), to take a list of Personas and force them to be unlinked from their Individuals. We'd need to be very careful about corner-cases such as the one where the input set of Personas is disjoint from the output set; in this case, the start Individual would actually remain untouched unless IndividualAggregator.unlink_personas() was called on all of its Personas, since none of them would be dragged into the new Individual.
(In reply to comment #3) > (In reply to comment #0) > > 1. the Unlink button seems slightly out-of-place in the Edit dialog and has no > > context for what it means > > I suggest changing its label to "Unlink Contacts" and ensuring it's mentioned > in the documentation. Sounds like a good first step. > > 2. removing a contact from an Individual requires finding the Unlink button, > > knowing what that means, and then linking together all the other contacts that > > you want in the final Individual > > I disagree with this approach. It's far too close to release to be making large > UI and code changes like this — we're two weeks into UI freeze and two weeks > away from the final release, and we're still fixing bugs in the original > implementation of the linking UI. This point is certainly compelling to defer changes. > Furthermore, I don't think there's actually that much need to complicate the > linking dialogue with unlinking functionality. Unlinking is something which > should hardly ever be used; I imagine it would be used only to fix one or two > mistakes made during linking. Conversely, linking is something which will be > used a lot initially (when one first links one's contacts together), and then > hardly at all. As I see it, the linking UI should be optimised for linking, and > unlinking should be a secondary feature which is there when you need it, but > not in the way otherwise. Adding another set of checkboxes in the linking UI is > therefore just going to get in the way and be confusing for the vast majority > of times the dialogue's used. I completely agree in choice of optimization here. I think my only concern is how hard it is to figure out how to correct mistakes here. > (In reply to comment #1) > > Philip, I know there were reasons why we didn't do linking and unlinking in a > > single place before - could you list those again? > > It would require a way to deal with "anti-links" in libfolks; not just adding > them, but ensuring they're up-to-date and/or removed/changed correctly when > links are added and removed. This is necessary because otherwise you could open > the linking dialogue on an Individual containing Personas which have been > auto-linked (e.g. the same contact added on two accounts), and unticking one of > the Personas would cause absolutely nothing to happen, since (I imagine) it > would get immediately re-linked to the same Individual. Right. I was planning to avoid anti-links until after this Gnome cycle. > > All I can think of is needing to maintain a temporary Individual until hitting > > the Link button (in a way that the Aggregator won't notify other Folks users of > > the new Individual until the dialog is resolved). > > That's already the case. See EmpathyIndividualLinker.new_individual. > > Note that this is a little trickier than it sounds, since one needs to be very > careful not to cause any changes on the Individual (such as alias changes) > which would be propagated down to its Personas (and then actually pushed to a > backing store). See bug #628131, for example. Another point for post-Gnome-2.32 would probably be to move to a model where no changes are propagated until commit() is called (like in e-d-s). That would make this point much less of a concern and let us coalesce signals and other work. > > How much would we even need to add infrastructure to the Aggregator to support > > the preview Individual? Just expose Individual.replace() through the > > Aggregator? > > We wouldn't need to add any extra API for the preview Individual, since this is > what's done (and supported) already. We just change the Individual:personas > property of our temporary Individual, and let the Individual::personas-changed > signal update the linking dialogue's UI. > > Infrastructure would need to be added to support unlinking in the same dialogue > though, since you're moving from an operation where the output (set of Personas > to form an Individual) is a superset of the input (set of Personas in the start > Individual); to an operation where the two sets could be anything from equal to > disjoint. Right. > I guess the best way to do that would be to change > IndividualAggregator.link_personas() to remove each Persona it's given from the > Persona's current Individual, then link all the Personas together as it > currently does. This assumes that the signalling magic emitted when a Persona > is removed from an Individual (i.e. Individual::personas-changed) would > correctly cause all the necessary updates around the Individuals which have > provided Personas to the new Individual — something that would probably need > some work. > > This would require changing IndividualAggregator.unlink_individual(), or adding > an IndividualAggregator.unlink_personas(), to take a list of Personas and force > them to be unlinked from their Individuals. Right. We'll probably want to change this eventually, though it's a question of whether it really needs to be done now or not (and I'm leaning toward not). > We'd need to be very careful about corner-cases such as the one where the input > set of Personas is disjoint from the output set; in this case, the start > Individual would actually remain untouched unless > IndividualAggregator.unlink_personas() was called on all of its Personas, since > none of them would be dragged into the new Individual. My inclination is to unlink the original Individual fully if it's not included in the final Individual -- the dialog was launched with it as a target, so if it's no longer included, I'd think the intention would be to split it out. I wouldn't want to encourage people to launch the dialog on a random Individual if they want to adjust links on someone else.
As discussed on IRC, we've decided to move the "Unlink" button from the Edit dialogue to the linking dialogue, and place it on the bottom left. Unlinking would be protected by a confirmation dialogue; accepting the confirmation would unlink the Individual and close the linking dialogue, whereas declining the confirmation dialogue would keep the linking dialogue open. The "Unlink" button should have a tooltip explaining that it completely unlinks the Individual. We're going ahead with the change in bug #628377, but not the one in bug #628380.
Sounds good to me. Don't forget to ask for a freeze break before merging the branch.
Created attachment 169224 [details] [review] Squashed diff of the 628376-move-unlink-button branch http://git.collabora.co.uk/?p=user/pwith/empathy;a=shortlog;h=refs/heads/628376-move-unlink-button
Before: http://people.collabora.co.uk/~pwith/empathy-before-moving-unlink-button.png After: http://people.collabora.co.uk/~pwith/empathy-after-moving-unlink-button.png
Review of attachment 169224 [details] [review]: Isn't the "Unlink" button a bit lost alone in the left corner? I'm not sure if that apply with confirmation dialog, but we generally tend to use meaningul verbs on button instead of just "Apply". Once again, not sure if that apply here but shouldn't it be "Unlink..." as there is an extra dialog?
Created attachment 169230 [details] [review] Squashed diff of the 628376-move-unlink-button-rebase1 branch http://git.collabora.co.uk/?p=user/pwith/empathy;a=shortlog;h=refs/heads/628376-move-unlink-button-rebase1
(Screenshots updated.)
Review of attachment 169230 [details] [review]: looks good
Freeze break request sent.
Permission from the release team here: http://mail.gnome.org/archives/release-team/2010-September/msg00011.html. Rebased and merged. commit 60d158ac379d725e0ecd91c74ea7de7a65b212bc Author: Philip Withnall <philip.withnall@collabora.co.uk> Date: Wed Sep 1 11:12:09 2010 +0100 Add a tooltip to the “Unlink” button Closes: bgo#628376 libempathy-gtk/empathy-linking-dialog.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) commit a32e3cde4a0f551169367baeadfc76587eae2b29 Author: Philip Withnall <philip.withnall@collabora.co.uk> Date: Wed Sep 1 11:05:20 2010 +0100 Add a confirmation dialogue to the unlink process This makes it harder to accidentally remove a linked Individual now that the “Unlink” button is easier to find, and also makes it more obvious that Empathy hasn't just crashed when unlinking. Helps: bgo#628377 libempathy-gtk/empathy-linking-dialog.c | 27 ++++++++++++++++++++++++--- 1 files changed, 24 insertions(+), 3 deletions(-) commit d7c3afd92a4851669316aeb352235643002004e7 Author: Philip Withnall <philip.withnall@collabora.co.uk> Date: Wed Sep 1 10:42:00 2010 +0100 Move the “Unlink” button from the Edit dialogue to the linking dialogue This is a more logical place for it, making unlinking more discoverable. Helps: bgo#628376 libempathy-gtk/empathy-individual-edit-dialog.c | 53 +---------------------- libempathy-gtk/empathy-linking-dialog.c | 40 +++++++++++++++++ 2 files changed, 41 insertions(+), 52 deletions(-) commit ee0ed7f28d8a567675d434e6d9b8d13789b72292 Author: Philip Withnall <philip.withnall@collabora.co.uk> Date: Wed Sep 1 10:28:17 2010 +0100 Fix code formatting in EmpathyLinkingDialog libempathy-gtk/empathy-linking-dialog.c | 17 +++++++++-------- 1 files changed, 9 insertions(+), 8 deletions(-)