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 629537 - Support anti-linking
Support anti-linking
Status: RESOLVED FIXED
Product: folks
Classification: Platform
Component: libfolks
git master
Other Linux
: High enhancement
: Future
Assigned To: folks-maint
folks-maint
: 630995 633569 635638 652273 661590 (view as bug list)
Depends on: 639113 645680
Blocks: 628868 629538
 
 
Reported: 2010-09-13 16:39 UTC by Travis Reitter
Modified: 2012-07-07 16:30 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Squashed diff of the 629537-anti-links branch (34.78 KB, patch)
2011-01-22 13:12 UTC, Philip Withnall
none Details | Review
Squashed diff of the 629537-anti-links branch (updated) (39.29 KB, patch)
2011-01-22 13:37 UTC, Philip Withnall
rejected Details | Review
Squashed diff of the 629537-anti-links-attempt3 branch (39.02 KB, patch)
2012-05-07 21:50 UTC, Philip Withnall
none Details | Review
Squashed diff of the 629537-anti-links-attempt3 branch (updated) (43.67 KB, patch)
2012-05-07 23:06 UTC, Philip Withnall
committed Details | Review

Description Travis Reitter 2010-09-13 16:39:26 UTC
As we add more linking automation, we'll need to be able to explicitly break automatic links.

These unwanted links could occur do to bad data in a source that we cannot modify. For example, if the user mistakenly linked two unrelated people in Pidgin, the Pidgin backend would suggest the unwanted link. The only way to correct this would be to add an anti-link in the Folks-controlled (key-file) backend.

Similarly, unwanted links could come from a bug in the algorithm for auto-linking across multiple backends. We'll err on the side of fewer links, since, ideally, the user should never need to anti-link. However, we need the functionality there just in case.

Implementation detail: of course, when adding an anti-link, we should first remove any corresponding "positive" links.
Comment 1 Philip Withnall 2010-09-30 12:02:16 UTC
*** Bug 630995 has been marked as a duplicate of this bug. ***
Comment 2 Philip Withnall 2010-11-01 00:01:08 UTC
*** Bug 633569 has been marked as a duplicate of this bug. ***
Comment 3 Philip Withnall 2010-11-22 22:55:45 UTC
(I'm working on this this week and next week.)
Comment 4 Philip Withnall 2010-11-23 20:34:15 UTC
*** Bug 635638 has been marked as a duplicate of this bug. ***
Comment 5 Philip Withnall 2010-12-24 16:52:50 UTC
Here's a work-in-progress branch: http://git.collabora.co.uk/?p=user/pwith/folks;a=shortlog;h=refs/heads/629537-anti-links-WIP.

The bulk of the work is done and is ready for a cursory review. It doesn't particularly work at the moment, but I'm fairly confident that's just a result of an accumulation of small bugs which I'll fix later (after Christmas!).
Comment 6 Travis Reitter 2010-12-27 20:37:56 UTC
(In reply to comment #5)
> Here's a work-in-progress branch:
> http://git.collabora.co.uk/?p=user/pwith/folks;a=shortlog;h=refs/heads/629537-anti-links-WIP.
> 
> The bulk of the work is done and is ready for a cursory review. It doesn't
> particularly work at the moment, but I'm fairly confident that's just a result
> of an accumulation of small bugs which I'll fix later (after Christmas!).

The code generally looks good (I haven't tested it). Some comments:

check_anti_link() would be better as anti_link_exists() (for consistency, and I think its intent is slightly clearer)


I really think we should split up add_personas() so it's a simple overview of what's going on. As it is now, it's very long and somewhat hard to follow (even if it's largely comments).


Simpler as    if (p2 != candidate_p) p2.unref ();:

        /* The personas share an anti-link, so this entire
         * individual has to be removed as a candidate. */
        /* Unref all the personas we've reffed so far */
        foreach (unowned Persona p2 in individual.personas)
          {
            if (p2 == candidate_p)
              break;
            p2.unref ();
          }


As it is, this code prevents splitting up user personas -- is this intentional?


Since this is within an async function, this should be a yield instead of a begin():
+          if (current_etag != this.anti_link_etag)
+            this.load_anti_links.begin ();


But more than all of these bits, I think we really need a handful of tests (comparable to the linking tests) for a feature this big (especially if we're running into hard-to-debug errors). I'd like that to be the general requirement for feature addition, so we grow our safety net as we grow our code complexity.
Comment 7 Philip Withnall 2011-01-17 16:16:19 UTC
(In reply to comment #6)
> check_anti_link() would be better as anti_link_exists() (for consistency, and I
> think its intent is slightly clearer)

Done (locally).

> I really think we should split up add_personas() so it's a simple overview of
> what's going on. As it is now, it's very long and somewhat hard to follow (even
> if it's largely comments).

I have some ideas floating around for rewriting the aggregation code to be a lot tidier, but I haven't had time to test them out or implement them. I'd rather get this in first, as it's a more important feature.

> Simpler as    if (p2 != candidate_p) p2.unref ();:
> 
>         /* The personas share an anti-link, so this entire
>          * individual has to be removed as a candidate. */
>         /* Unref all the personas we've reffed so far */
>         foreach (unowned Persona p2 in individual.personas)
>           {
>             if (p2 == candidate_p)
>               break;
>             p2.unref ();
>           }

Changed locally.

> As it is, this code prevents splitting up user personas -- is this intentional?

I can't see what would cause that. What code are you talking about?

> Since this is within an async function, this should be a yield instead of a
> begin():
> +          if (current_etag != this.anti_link_etag)
> +            this.load_anti_links.begin ();

yield causes the caller to wait until the callee returns (i.e. it's a synchronous call in a different thread), which we don't want here: we want the caller to return as soon as possible, and the loading of anti-links to happen concurrently in a different thread. We don't care about the return value of the call, so I think using .begin() is correct.
Comment 8 Travis Reitter 2011-01-17 18:50:21 UTC
(In reply to comment #7)
> > As it is, this code prevents splitting up user personas -- is this intentional?
> 
> I can't see what would cause that. What code are you talking about?

Hmm, neither can I. Maybe yesterme was referring to actual behavior. Just double-check that it's possible to split up user Individual(s) in the next instance of your branch.

> > Since this is within an async function, this should be a yield instead of a
> > begin():
> > +          if (current_etag != this.anti_link_etag)
> > +            this.load_anti_links.begin ();
> 
> yield causes the caller to wait until the callee returns (i.e. it's a
> synchronous call in a different thread), which we don't want here: we want the
> caller to return as soon as possible, and the loading of anti-links to happen
> concurrently in a different thread. We don't care about the return value of the
> call, so I think using .begin() is correct.

You're right.
Comment 9 Philip Withnall 2011-01-22 13:07:25 UTC
(In reply to comment #8)
> (In reply to comment #7)
> > > As it is, this code prevents splitting up user personas -- is this intentional?
> > 
> > I can't see what would cause that. What code are you talking about?
> 
> Hmm, neither can I. Maybe yesterme was referring to actual behavior. Just
> double-check that it's possible to split up user Individual(s) in the next
> instance of your branch.

You were actually correct: the code, as it stood, couldn't split up user personas, as they (erroneously) shared the same UID. The earlier commits in my branch now fix this.
Comment 10 Philip Withnall 2011-01-22 13:12:15 UTC
Created attachment 179030 [details] [review]
Squashed diff of the 629537-anti-links branch

http://git.collabora.co.uk/?p=user/pwith/folks;a=shortlog;h=refs/heads/629537-anti-links

This branch is basically complete, and ready for review. The only things left to do are:
 • add test cases; and
 • change IndividualStore._link_map to map string → LinkedList<Individual>.

The former is pretty self-explanatory. The latter is needed to allow personas to be correctly re-linked after the anti-links preventing them from being linked are removed; in the case that two personas share (e.g.) the same linkable IM address property, the second one will overwrite the first one's mapping in the link map, so when re-linking occurs, only the second one will be found by its linkable properties.

This can be demonstrated by unlinking the user's individual (which in my case contained four personas from two Jabber accounts), then re-linking it. Only two personas will be re-linked correctly. Restarting Empathy fixes the problem.
Comment 11 Philip Withnall 2011-01-22 13:37:12 UTC
Created attachment 179031 [details] [review]
Squashed diff of the 629537-anti-links branch (updated)

http://git.collabora.co.uk/?p=user/pwith/folks;a=shortlog;h=refs/heads/629537-anti-links

Updated patch which includes the commit I just pushed to the branch to convert IndividualAggregator._link_map to be a HashMultiMap, addressing my second point in comment #10.
Comment 12 Travis Reitter 2011-01-24 18:53:59 UTC
(In reply to comment #11)
> Created an attachment (id=179031) [details] [review]
> Squashed diff of the 629537-anti-links branch (updated)
> 
> http://git.collabora.co.uk/?p=user/pwith/folks;a=shortlog;h=refs/heads/629537-anti-links
> 
> Updated patch which includes the commit I just pushed to the branch to convert
> IndividualAggregator._link_map to be a HashMultiMap, addressing my second point
> in comment #10.

Looks good except for this error:
==============================================
individual-aggregator.vala:1347.38-1347.50: error: The name `index_of' does not exist in the context of `string'
                  var equals_index = line.index_of ("=");
                                     ^^^^^^^^^^^^^
individual-aggregator.vala:1347.23-1347.56: error: var declaration not allowed with non-typed initializer
                  var equals_index = line.index_of ("=");
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
individual-aggregator.vala:1348.23-1348.34: error: The name `equals_index' does not exist in the context of `Folks.IndividualAggregator._load_anti_links'
                  if (equals_index == -1 ||
                      ^^^^^^^^^^^^
individual-aggregator.vala:1349.23-1349.35: error: The name `index_of' does not exist in the context of `string'
                      line.index_of ("=", equals_index + 1) != -1)
                      ^^^^^^^^^^^^^
Compilation failed: 4 error(s), 0 warning(s)
==============================================

I guess you're using a bleeding-edge Vala string method? In that case, we'll need to bump the Vala version requirement, of course (I don't seem to satisfy it currently).

==============================================

+              if (candidate_inds_set != null)
                 {
-                  debug ("    Found candidate individual '%s' by IID '%s'.",
-                      candidate_ind.id, persona.iid);
-                  candidate_inds.add (candidate_ind);
-                  candidate_ind_set.add (candidate_ind);
+                  foreach (var candidate_ind in candidate_inds_set)
+                    {

I'm guessing Vala doesn't cleanly treat (candidate_inds_set == NULL) as a no-op for the foreach? I can never remember. If it does, cut this outer check. If not, we should probably file it as a bug against Vala.
Comment 13 Philip Withnall 2011-01-25 17:20:40 UTC
(In reply to comment #12)
> (In reply to comment #11)
> > Created an attachment (id=179031) [details] [review] [details] [review]
> > Squashed diff of the 629537-anti-links branch (updated)
> > 
> > http://git.collabora.co.uk/?p=user/pwith/folks;a=shortlog;h=refs/heads/629537-anti-links
> > 
> > Updated patch which includes the commit I just pushed to the branch to convert
> > IndividualAggregator._link_map to be a HashMultiMap, addressing my second point
> > in comment #10.
> 
> Looks good except for this error:
> ==============================================
> individual-aggregator.vala:1347.38-1347.50: error: The name `index_of' does not
> exist in the context of `string'
>                   var equals_index = line.index_of ("=");
>                                      ^^^^^^^^^^^^^
> individual-aggregator.vala:1347.23-1347.56: error: var declaration not allowed
> with non-typed initializer
>                   var equals_index = line.index_of ("=");
>                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> individual-aggregator.vala:1348.23-1348.34: error: The name `equals_index' does
> not exist in the context of `Folks.IndividualAggregator._load_anti_links'
>                   if (equals_index == -1 ||
>                       ^^^^^^^^^^^^
> individual-aggregator.vala:1349.23-1349.35: error: The name `index_of' does not
> exist in the context of `string'
>                       line.index_of ("=", equals_index + 1) != -1)
>                       ^^^^^^^^^^^^^
> Compilation failed: 4 error(s), 0 warning(s)
> ==============================================
> 
> I guess you're using a bleeding-edge Vala string method? In that case, we'll
> need to bump the Vala version requirement, of course (I don't seem to satisfy
> it currently).

As we discussed on IRC, it looks like string.index_of() has recently been added to 0.11.x, and wasn't present in 0.10.x. However, string.str() is deprecated by it in 0.11.x.

I've pushed the following commit (to be squashed later) which conditionally compiles different code for 0.10.x and 0.11.x:

http://git.collabora.co.uk/?p=user/pwith/folks;a=commit;h=2a29dd259adeb4289ef079fb8f66e40c288c0b03

> ==============================================
> 
> +              if (candidate_inds_set != null)
>                  {
> -                  debug ("    Found candidate individual '%s' by IID '%s'.",
> -                      candidate_ind.id, persona.iid);
> -                  candidate_inds.add (candidate_ind);
> -                  candidate_ind_set.add (candidate_ind);
> +                  foreach (var candidate_ind in candidate_inds_set)
> +                    {
> 
> I'm guessing Vala doesn't cleanly treat (candidate_inds_set == NULL) as a no-op
> for the foreach? I can never remember. If it does, cut this outer check. If
> not, we should probably file it as a bug against Vala.

I hadn't actually tested this before (just assumed it), but it turns out that libgee crashes with an assertion failure if NULL is passed to gee_iterable_iterator() (which is what happens in the foreach{} loop), so the outer checks have to stay.

I've filed bug #640555 against libgee.
Comment 14 Travis Reitter 2011-01-29 00:53:26 UTC
This is failing test cases. I've fixed one of them (which hadn't been adjusted for the change in Individual.id), but others are still broken. See my latest branch here:

http://git.collabora.co.uk/?p=user/treitter/folks.git;a=shortlog;h=refs/heads/629537-anti-links-3

(Note that I added an unrelated commit to fix the style in test-case.vala which changed the hashes of your commits, in case you were tracking those carefully).
Comment 15 Philip Withnall 2011-01-29 15:09:56 UTC
(In reply to comment #14)
> This is failing test cases. I've fixed one of them (which hadn't been adjusted
> for the change in Individual.id), but others are still broken. See my latest
> branch here:

Aargh, sorry.

> http://git.collabora.co.uk/?p=user/treitter/folks.git;a=shortlog;h=refs/heads/629537-anti-links-3

I've fixed the breakage to the test suites with two commits on top of your branch. I've pushed the whole lot here:

http://git.collabora.co.uk/?p=user/pwith/folks;a=shortlog;h=refs/heads/629537-anti-links-4
Comment 16 Travis Reitter 2011-01-29 23:09:05 UTC
(In reply to comment #15)
> I've fixed the breakage to the test suites with two commits on top of your
> branch. I've pushed the whole lot here:
> 
> http://git.collabora.co.uk/?p=user/pwith/folks;a=shortlog;h=refs/heads/629537-anti-links-4

Nice fix with the last 2 patches. But now I'm hitting this (not sure why - haven't had time to dig much yet; this happens both with the version of tp-glib I had and now the latest from git master. The second message is from tpf-persona-store.vala):

/Aggregation/linkable properties:different stores: telepathy-Message: Get Channels property failed: User requested disconnection

telepathy-WARNING **: Failed to determine whether we can set aliases on Telepathy account 'Fake Account': User requested disconnection
aborting...
Trace/breakpoint trap
FAIL: aggregation
Comment 17 Philip Withnall 2011-01-30 13:49:59 UTC
It's working fine for me with tp-glib git master (a0ef503aa657aa46ff53accf2980404c9e5d4bbd). I haven't ever seen that error, and I'm not sure what could be causing it. :-\

Is it definitely introduced by the 629537-anti-links-4 branch?
Comment 18 Travis Reitter 2011-01-31 06:58:51 UTC
(In reply to comment #17)
> It's working fine for me with tp-glib git master
> (a0ef503aa657aa46ff53accf2980404c9e5d4bbd). I haven't ever seen that error, and
> I'm not sure what could be causing it. :-\
> 
> Is it definitely introduced by the 629537-anti-links-4 branch?

I hit a different error in that test in the anti-links-3 branch; these fail:

assert (individual1 != null);
assert (individual2 != null);

The "same store" test also fails an assertion, but I commented it out to see whether this test failed the same way as in the anti-links-4 branch. All the other Aggregation tests run fine in the anti-links-3 branch.

These tests also fail on assertions (the sub-tests after them may also fail; I havne't tried commenting out the failing sub-tests):

/IndividualRetrieval/aggregator
/IndividualProperties/individual properties
Comment 19 Philip Withnall 2011-01-31 19:51:03 UTC
(In reply to comment #18)
> (In reply to comment #17)
> > It's working fine for me with tp-glib git master
> > (a0ef503aa657aa46ff53accf2980404c9e5d4bbd). I haven't ever seen that error, and
> > I'm not sure what could be causing it. :-\
> > 
> > Is it definitely introduced by the 629537-anti-links-4 branch?
> 
> I hit a different error in that test in the anti-links-3 branch; these fail:

Yeah, I fixed those in 629537-anti-links-4.

I meant “is it definitely not present in current master vs. 629537-anti-links-4?”, sorry.
Comment 20 Travis Reitter 2011-01-31 20:24:27 UTC
(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > It's working fine for me with tp-glib git master
> > > (a0ef503aa657aa46ff53accf2980404c9e5d4bbd). I haven't ever seen that error, and
> > > I'm not sure what could be causing it. :-\
> > > 
> > > Is it definitely introduced by the 629537-anti-links-4 branch?
> > 
> > I hit a different error in that test in the anti-links-3 branch; these fail:
> 
> Yeah, I fixed those in 629537-anti-links-4.
> 
> I meant “is it definitely not present in current master vs.
> 629537-anti-links-4?”, sorry.

Master passes all of its tests just fine.
Comment 21 Travis Reitter 2011-01-31 20:28:21 UTC
Bumping these to the next release.
Comment 22 Philip Withnall 2011-02-06 11:45:31 UTC
I've done some more probing, and I still can't reproduce those test failures. What version of Vala are you using?

Here are the relevant versions I'm using:
 • Vala 0.11.4.5-1352b
 • tp-glib (master, a0ef503aa657aa46ff53accf2980404c9e5d4bbd)
 • libgee (0.6, 07a8677a2e1c0ba76db1be3df2aa18b5d8336686)
 • dbus-glib 0.82
 • GLib (master, 5d9f5cdc5a9d45b80e728b2609966af1d5f70c52)

Here's a branch rebased against master, and with your commits squashed in for convenience:

http://git.collabora.co.uk/?p=user/pwith/folks;a=shortlog;h=refs/heads/629537-anti-links-5
Comment 23 Travis Reitter 2011-02-08 18:26:32 UTC
With these versions of the packages:

 • Vala 0.11.5
 • tp-glib (master, 58bf150c70bee50ca69c7b7bed632aed66e72eae, Feb 1)
 • libgee 0.6 release
 • dbus-glib 0.82
 • GLib (master, 9823ff1d203166f33302dce2a26e1dee86c4d569, Feb 4?)

I bisected your latest anti-links-5 branch and came up with this as the culprit:

0c3ba4a1ea4e17c0dd66f1edf37517e9fab35421 is the first bad commit
commit 0c3ba4a1ea4e17c0dd66f1edf37517e9fab35421
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Sun Nov 21 15:35:01 2010 +0000

    Use gee's LinkedList for candidate_inds in the IndividualAggregator
    
    This is necessary for the anti-linking work (bgo#629537), and also removes
    an unnecessary lambda callback function.

:040000 040000 3212e85912df8ccb89a7e162a9e2e02853a8c597 8616923af2bdf3c83be645b18cccc80c3d79b208 M	folks

======================================

Note that I'm hitting the same problem with bug #640092, which also monkeys around with the type of candidate_inds (though to LinkedHashSet instead of LinkedList). No obvious idea why that would end up causing the issue with the Telepathy backend seeing a disconnection request.
Comment 24 Philip Withnall 2011-02-14 23:14:10 UTC
Punting to 0.3.7.
Comment 25 Philip Withnall 2011-02-20 11:47:03 UTC
(In reply to comment #23)
> I bisected your latest anti-links-5 branch and came up with this as the
> culprit:

*snip*

> Note that I'm hitting the same problem with bug #640092, which also monkeys
> around with the type of candidate_inds (though to LinkedHashSet instead of
> LinkedList). No obvious idea why that would end up causing the issue with the
> Telepathy backend seeing a disconnection request.

Since LinkedHashSet is just a wrapper around LinkedList, I think we can assume that it's LinkedList which is causing the problem.

I'm wondering if the problem might be being caused because that commit changes the order in which individuals are added to the list: with GList, individuals were prepended to the list; but with LinkedList/LinkedHashSet, they're appended.

I've pushed http://git.collabora.co.uk/?p=user/pwith/folks;a=shortlog;h=refs/heads/629537-anti-links-6 which changes the commit[1] to prepend individuals onto the LinkedList. It continues to pass all the tests in the test suite for me, but I'm wondering if it'll fix your disconnection issue?

Apart from that, this updated branch is also rebased against today's master.

[1]: http://git.collabora.co.uk/?p=user/pwith/folks;a=commitdiff;h=054791b7b312ce09d7231b718a990393c5883a66
Comment 26 Travis Reitter 2011-02-21 18:32:35 UTC
(In reply to comment #25)

> I've pushed
> http://git.collabora.co.uk/?p=user/pwith/folks;a=shortlog;h=refs/heads/629537-anti-links-6
> which changes the commit[1] to prepend individuals onto the LinkedList. It
> continues to pass all the tests in the test suite for me, but I'm wondering if
> it'll fix your disconnection issue?

No such luck :-/

It's a bit of a stretch (and hopefully not too much active work), but could you set up a Debian-unstable virtual machine, jhbuild to folks and try to reproduce this bug?
Comment 27 Philip Withnall 2011-02-22 17:59:05 UTC
(In reply to comment #26)
> It's a bit of a stretch (and hopefully not too much active work), but could you
> set up a Debian-unstable virtual machine, jhbuild to folks and try to reproduce
> this bug?

As I said by e-mail, I'm not going to be able to find time to set up a virtual machine just to test this. Who do we know is on Debian-unstable and could spare half an hour to compile and test folks master?
Comment 28 Philip Withnall 2011-06-10 12:26:12 UTC
*** Bug 652273 has been marked as a duplicate of this bug. ***
Comment 29 Philip Withnall 2011-06-30 18:24:46 UTC
I've just had a thought about a major change to the way I've implemented anti-linking which may prove better in the long run: instead of storing the anti-links in a special local file, anti-linking capabilities should be exposed as an interface on Personas whose persona stores can support it. That way, we can get network transparency for anti-links.

e.g. An interface like the following:

public interface AntiLinkable {
    public async void add_anti_link (Persona the_persona_i_dont_want_to_be_linked_to);
    public async void remove_anti_link (Persona my_new_best_friend);
}

For example, Kf.Persona would implement the interface, and when either of the two methods are called it just modifies a local file similarly to how the current anti-linking branch does it.

Conversely, Eds.Persona would set a X-FOLKS-ANTI-LINK property on its underlying vCard which is then saved to e-d-s. For Google Contacts address books, this would result in network transparency of anti-links.

I haven't put much more thought than this into this approach, but if it works I think it's more natural than what I have already.

Comments welcome. I'll try and work on this when I get the chance.
Comment 30 Travis Reitter 2011-07-19 23:12:27 UTC
(In reply to comment #29)
> I've just had a thought about a major change to the way I've implemented
> anti-linking which may prove better in the long run: instead of storing the
> anti-links in a special local file, anti-linking capabilities should be exposed
> as an interface on Personas whose persona stores can support it. That way, we
> can get network transparency for anti-links.
> 
> e.g. An interface like the following:
> 
> public interface AntiLinkable {
>     public async void add_anti_link (Persona
> the_persona_i_dont_want_to_be_linked_to);
>     public async void remove_anti_link (Persona my_new_best_friend);
> }
> 
> For example, Kf.Persona would implement the interface, and when either of the
> two methods are called it just modifies a local file similarly to how the
> current anti-linking branch does it.
> 
> Conversely, Eds.Persona would set a X-FOLKS-ANTI-LINK property on its
> underlying vCard which is then saved to e-d-s. For Google Contacts address
> books, this would result in network transparency of anti-links.
> 
> I haven't put much more thought than this into this approach, but if it works I
> think it's more natural than what I have already.
> 
> Comments welcome. I'll try and work on this when I get the chance.

This sounds generally fine to me. I'd like to see how it works out so we'd probably need some auto-linking in place (not necessarily in master yet) to make sure the API/behavior is approximately right.
Comment 31 Travis Reitter 2011-08-01 18:39:28 UTC
Punting bugs that won't be fixed by Folks 0.6.0.
Comment 32 Philip Withnall 2011-10-12 21:04:52 UTC
*** Bug 661590 has been marked as a duplicate of this bug. ***
Comment 33 Philip Withnall 2012-05-03 19:14:58 UTC
Comment on attachment 179031 [details] [review]
Squashed diff of the 629537-anti-links branch (updated)

Rejecting old patch due to the re-think in comment #29.
Comment 34 Philip Withnall 2012-05-07 21:50:03 UTC
Created attachment 213624 [details] [review]
Squashed diff of the 629537-anti-links-attempt3 branch

https://www.gitorious.org/folks/folks/commits/629537-anti-links-attempt3

Here’s a branch implementing the approach given in comment #29. This is a lot simpler than the previous approach, and passes all the tests. Linking and unlinking individuals works for me.

The branch doesn’t implement any new test cases. I don’t think it’s worth it to implement them in this branch itself, since I plan to revamp folks’ test suite next so that it’s not so racy (and so that we have a proper dummy backend for testing against). Tests can be implemented then.

Comments welcome!
Comment 35 Philip Withnall 2012-05-07 23:06:19 UTC
Created attachment 213628 [details] [review]
Squashed diff of the 629537-anti-links-attempt3 branch (updated)

https://www.gitorious.org/folks/folks/commits/629537-anti-links-attempt3

Updated to fix a bug whereby it wouldn’t work when unlinking two non-writeable Personas (such as two Jabber contacts representing the user on two different accounts — a common situation).
Comment 36 Jeremy Whiting 2012-06-26 15:14:17 UTC
Philip,

What's the status of this branch?  Is it dependent on the dummy backend, or only for unit testing of the functionality in here?
Comment 37 Philip Withnall 2012-06-26 18:17:15 UTC
(In reply to comment #36)
> What's the status of this branch?  Is it dependent on the dummy backend, or
> only for unit testing of the functionality in here?

This probably needs to be rebased against master, but there shouldn’t be many conflicts. It’s fairly current, and needs reviewing urgently before it bit-rots again (and also because the sooner we get it in, the more testing it’ll get, since it’s likely to break things). It doesn’t depend on a dummy backend — but test cases can’t be written for it until we have a dummy backend. I don’t want to block this branch on writing a dummy backend though, since then it’ll never get in.
Comment 38 Philip Withnall 2012-07-06 18:14:52 UTC
(In reply to comment #36)
> What's the status of this branch?  Is it dependent on the dummy backend, or
> only for unit testing of the functionality in here?

It’s also still waiting for a review. :-)
Comment 39 Jeremy Whiting 2012-07-06 20:54:32 UTC
Philip, the code looks good, I got a few conflicts when I rebased it on master here, but it builds fine after that.  As I still haven't been able to get gnome-contacts to run (and now that I ported folks to newer libgee it wont even build :*| I'm not sure how to test this.  Assuming it works for you I say ship it.

By the way, you didn't make unit tests because of the dummy backend not being there, but couldn't unit tests be added for this to the keyfile and edsf backend unit tests?
Comment 40 Philip Withnall 2012-07-07 16:30:34 UTC
Rebased, merged and pushed to master. Thanks. :-)

commit 9ccd3a11b8baef9378afd453b96df0e7396117f5
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Sun May 6 22:59:45 2012 +0100

    key-file: Add anti-linking support
    
    Helps: https://bugzilla.gnome.org/show_bug.cgi?id=629537

 backends/key-file/kf-persona-store.vala |    5 ++-
 backends/key-file/kf-persona.vala       |   70 ++++++++++++++++++++++++++++++-
 2 files changed, 73 insertions(+), 2 deletions(-)

commit a87ba87ae231f2c2f511fad804c8cfd8da4c82e3
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Sun May 6 22:59:34 2012 +0100

    eds: Add anti-linking support
    
    Helps: https://bugzilla.gnome.org/show_bug.cgi?id=629537

 backends/eds/lib/edsf-persona-store.vala |   53 +++++++++++++++++++++++++--
 backends/eds/lib/edsf-persona.vala       |   59 ++++++++++++++++++++++++++++++
 2 files changed, 109 insertions(+), 3 deletions(-)

commit 1441ae9ddda3dfba8a67d18ecc6a75e16d389553
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Mon Jul 25 20:27:29 2011 +0100

    core: Add core anti-linking support
    
    This adds the core of the anti-linking support, based around a new
    AntiLinkable interface. This will be implemented by Persona subclasses which
    can store anti-linking information (in the form of a set of Persona UIDs
    which the given Persona should never be linked to).
    
    This approach allows anti-linking information to be stored with the personas
    (presumably in the primary persona store) and thus it should be network
    transparent. i.e. Using folks on two different computers with a Google
    Contacts address book as primary should cause the anti-linking data to be
    shared.
    
    This also includes the necessary IndividualAggregator changes.
    
    Sadly, no unit tests are included.
    
    Closes: https://bugzilla.gnome.org/show_bug.cgi?id=629537

 NEWS                             |    3 +-
 folks/Makefile.am                |    1 +
 folks/anti-linkable.vala         |  157 ++++++++++++++++++++++++++++++++++++++
 folks/individual-aggregator.vala |  117 ++++++++++++++++++++++++-----
 folks/individual.vala            |   60 ++++++++++++++
 folks/persona-store.vala         |   12 +++-
 folks/potential-match.vala       |   10 ++-
 tools/inspect/utils.vala         |    3 +-
 8 files changed, 339 insertions(+), 24 deletions(-)

commit 2f442b4906a9c0f2c44a7d65e379f695e74cf701
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Tue May 8 00:01:42 2012 +0100

    core: Replace the linking step in IA.ensure_individual_property_writeable()
    
    It’s a waste of time to create a new Persona to ensure the property is
    writeable, then create _another_ new Persona to link it to the Individual.
    Why not just create the writeable Persona so that it’s automatically linked
    to the Individual?
    
    Helps: https://bugzilla.gnome.org/show_bug.cgi?id=629537

 folks/individual-aggregator.vala |   31 ++++++++++++++-----------------
 1 files changed, 14 insertions(+), 17 deletions(-)

commit 34e0b4428332291896d10285aa3f610537a8cf1b
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Mon May 7 23:59:56 2012 +0100

    core: Split details table code out of IA.link_personas()
    
    Helps: https://bugzilla.gnome.org/show_bug.cgi?id=629537

 folks/individual-aggregator.vala |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

commit 787c0769dee8f31f8d9372eaf7a69057677832b0
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Thu Dec 8 10:18:25 2011 +0000

    aggregator: Convert the link map to use a MultiMap
    
    When it's possible to have personas sharing IM addresses but not being
    linked (due to anti-links), the link map's assumption that one IM address
    (or other linkable property) maps to one individual will be invalid. This
    commit converts the link map to be a HashMultiMap, effectively changing it
    from a mapping of string → Individual to a mapping of
    string → Set<Individual>.
    
    Helps: bgo#629537

 folks/individual-aggregator.vala |  123 +++++++++++++++++++++++---------------
 1 files changed, 74 insertions(+), 49 deletions(-)