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 724058 - Add support to avoid folks auto link
Add support to avoid folks auto link
Status: RESOLVED OBSOLETE
Product: folks
Classification: Platform
Component: libfolks
git master
Other Linux
: Normal normal
: Unset
Assigned To: folks-maint
folks-maint
Depends on:
Blocks:
 
 
Reported: 2014-02-10 18:07 UTC by Renato Araujo Oliveira Filho
Modified: 2018-09-21 16:08 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
proposal patch to add global anti link functionality to AntiLinkable interface (3.86 KB, patch)
2014-02-10 18:07 UTC, Renato Araujo Oliveira Filho
needs-work Details | Review
New patch with all fixes (4.11 KB, patch)
2014-03-01 20:53 UTC, Renato Araujo Oliveira Filho
committed Details | Review
unit test for global anti-link (24.52 KB, patch)
2014-03-01 20:55 UTC, Renato Araujo Oliveira Filho
needs-work Details | Review

Description Renato Araujo Oliveira Filho 2014-02-10 18:07:36 UTC
Created attachment 268703 [details] [review]
proposal patch to add global anti link functionality to AntiLinkable interface

Today there is no easy way to make a persona unlikable with any other persona without know the other persona id.

The idea is make the persona unlikable before folks auto link it, avoid unnecessary emission of the signal "individuals-changed-detailed" and complex code.
Comment 1 Philip Withnall 2014-02-28 23:22:18 UTC
Review of attachment 268703 [details] [review]:

I like this approach, and while it’s a shame we can’t reconcile global and local anti-links better (e.g. removing an anti-link would ideally remove the global anti-link and replace it with the universe of possible anti-links, minus the one just removed [which obviously isn’t possible]) I think it should be OK. Pending a few rewordings of the comments, this should be good to merge.

Travis, what do you think?

::: folks/anti-linkable.vala
@@ +47,3 @@
    *
+   * UIDs can be ``*`` this is used as wildcard to mark the persona as global
+   * anti linkable {@link AntiLinkable.has_global_anti_link}.

“The special UID ``*`` is used as a wildcard to mark the persona as globally
anti-linked. See {@link AntiLinkable.has_global_anti_link}.”

@@ +98,3 @@
     {
+      return (this.has_global_anti_link()) ||
+             (other_persona.uid in this.anti_links);

Space before ‘()’.

@@ +143,3 @@
    *
+   * If the global anti link is setted this will not make any effect until the
+   * global anti link get removed.

“If the global anti-link is set, this will not have any effect until the global anti-link is removed.”

@@ +166,3 @@
+
+  /**
+   * Block persona to be linked with any other persona

“Prevent persona from being linked with any other personas”

@@ +169,3 @@
+   *
+   * This function will add a wildcard '*' on the anti link list, and this will
+   * block the persona to be linked with any other persona.

“This function will add a wildcard ``*`` to the set of anti-links, which will prevent the persona from being linked with any other personas.”

@@ +171,3 @@
+   * block the persona to be linked with any other persona.
+   *
+   * To make the persona linkable again you need to remove the global anti link

s/anti link/anti-link/

@@ +174,3 @@
+   *
+   * This method is safe to call multiple times concurrently (e.g. begin one
+   * asynchronous call, then begin another before the first has finished).

This needs a ‘@throws PropertyError [some description]’ line adding to explain when the PropertyError exception can be thrown.

@@ +188,3 @@
+
+  /**
+   * Unblock pesona to be linked

“Allow persona to be linked with other personas”

@@ +191,3 @@
+   *
+   * This function removes the wildcard '*' from the anti link list, and will allow
+   * the persona to be be linked again.

“This function removes the wildcard ``*`` from the set of anti-links, allowing the persona to be linked again.”

@@ +194,3 @@
+   *
+   * This method is safe to call multiple times concurrently (e.g. begin one
+   * asynchronous call, then begin another before the first has finished).

This needs a ‘@throws PropertyError [some description]’ line adding to explain when the PropertyError exception can be thrown.

@@ +204,3 @@
+           new_anti_links.remove("*");
+           yield this.change_anti_links (new_anti_links);
+         }

Spaces before the ‘(’ in function calls please.

@@ +208,3 @@
+
+  /**
+   * Check if the persona has a global anti link.

s/anti link/anti-link/

@@ +211,3 @@
+   *
+   * If the persona has global anti link this means that the persona can not be
+   * linked with any other persona.

“If the persona has a global anti-link, they must not be linked with any other personas. A global anti-link overrides any other links or anti-links added while it exists.”

@@ +213,3 @@
+   * linked with any other persona.
+   *
+   */

All of these new API comments need ‘Since: UNRELEASED’ adding at the bottom.
Comment 2 Renato Araujo Oliveira Filho 2014-03-01 20:53:21 UTC
Created attachment 270647 [details] [review]
New patch with all fixes
Comment 3 Renato Araujo Oliveira Filho 2014-03-01 20:55:01 UTC
Created attachment 270648 [details] [review]
unit test for global anti-link

This patch uses the patch on Bug #722892 as base for the unit tests.
Comment 4 Philip Withnall 2014-03-03 23:49:57 UTC
Review of attachment 270647 [details] [review]:

Looks good to me. I’d like to get Travis’ opinion before committing though.
Comment 5 Philip Withnall 2014-03-03 23:57:22 UTC
Review of attachment 270648 [details] [review]:

From a quick read through, this all looks good. However, there’s a lot of code here. Do you think it would be possible to factor some of it out so the four tests share code a little more, and the file isn’t so long? Thanks for your work on this! :-)

::: tests/dummy/anti-linking.vala
@@ +1,2 @@
+/*
+ * Copyright (C) 2012 Collabora Ltd.

This needs updating.

@@ +16,3 @@
+ *
+ * Authors: Raul Gutierrez Segales <raul.gutierrez.segales@collabora.co.uk>
+ *          Travis Reitter <travis.reitter@collabora.co.uk>

The authors list needs updating.

::: tests/lib/dummy/test-case.vala
@@ +143,3 @@
+        this.dummy_backend = null;
+        base.final_tear_down ();
+      }

What are all the changes in this file for? Unregistering the personas from the dummy_persona_store shouldn’t be necessary if the store is then finalised by setting this.dummy_persona_store to null.
Comment 6 Travis Reitter 2014-03-04 18:24:07 UTC
(In reply to comment #1)
> Review of attachment 268703 [details] [review]:
> 
> I like this approach, and while it’s a shame we can’t reconcile global and
> local anti-links better (e.g. removing an anti-link would ideally remove the
> global anti-link and replace it with the universe of possible anti-links, minus
> the one just removed [which obviously isn’t possible]) I think it should be OK.
> Pending a few rewordings of the comments, this should be good to merge.
> 
> Travis, what do you think?

Yeah, in theory, I would be on board with what you described as the ideal solution but it's probably not worth the extra complexity, so the patch's approach seems reasonable.
Comment 7 Philip Withnall 2014-03-05 23:17:47 UTC
Comment on attachment 270647 [details] [review]
New patch with all fixes

Thanks Travis. Committed the first patch.
Comment 8 GNOME Infrastructure Team 2018-09-21 16:08:37 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/folks/issues/89.