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 660908 - Add favourites support to EDS backend
Add favourites support to EDS backend
Status: RESOLVED FIXED
Product: folks
Classification: Platform
Component: e-d-s backend
git master
Other Linux
: Normal normal
: Future
Assigned To: folks-maint
folks-maint
Depends on:
Blocks:
 
 
Reported: 2011-10-04 18:11 UTC by Travis Reitter
Modified: 2011-10-25 21:22 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (7.16 KB, patch)
2011-10-25 16:00 UTC, Raul Gutierrez Segales
needs-work Details | Review
extend add persona (2.38 KB, patch)
2011-10-25 16:20 UTC, Raul Gutierrez Segales
reviewed Details | Review
test setting is-favourite (5.74 KB, patch)
2011-10-25 16:22 UTC, Raul Gutierrez Segales
needs-work Details | Review
patch (7.05 KB, patch)
2011-10-25 19:57 UTC, Raul Gutierrez Segales
reviewed Details | Review
test setting is-favourite (5.74 KB, patch)
2011-10-25 19:58 UTC, Raul Gutierrez Segales
accepted-commit_now Details | Review

Description Travis Reitter 2011-10-04 18:11:53 UTC
As suggested in bug #660281 comment #14, it would be useful for Folks to store favorites instead of Telepathy Logger.

To ensure any arbitrary Individual can be marked as a favorite, we'd need the primary store (EDS, in most cases) to support marking personas as favorites.

Unfortunately, there isn't any well-defined (or commonly-used) way to label a contact as a favorite, so we'll need to make up an attribute like X-FOLKS-FAVORITE.
Comment 1 Raul Gutierrez Segales 2011-10-25 16:00:38 UTC
Created attachment 199944 [details] [review]
patch

Implementation of FavouriteDetails for Edsf.Persona. I'll follow up with a test case.
Comment 2 Raul Gutierrez Segales 2011-10-25 16:20:16 UTC
Created attachment 199947 [details] [review]
extend add persona

Extend the add-persona test case so it checks for is-favourite too.
Comment 3 Raul Gutierrez Segales 2011-10-25 16:22:17 UTC
Created attachment 199948 [details] [review]
test setting is-favourite 

Test setting is-favourite on an existing persona.
Comment 4 Travis Reitter 2011-10-25 18:45:44 UTC
Review of attachment 199944 [details] [review]:

backends/eds/lib/edsf-persona-store.vala

+              new_attr.add_value ("yes");

backends/eds/lib/edsf-persona.vala

+          if (val.down () == "yes")

vCard boolean values need to be in {"true", "false"} (case-insensitive)


backends/eds/lib/edsf-persona-store.vala
+      unowned VCardAttribute attr = contact.get_attribute ("X-FOLKS-FAVORITE");

+              var new_attr = new VCardAttribute (null, "X-FOLKS-FAVORITE");

backends/eds/lib/edsf-persona.vala
+      var fav = this.contact.get_attribute ("X-FOLKS-FAVORITE");

Note that "X-FOLKS-FAVORITE" uses en_US/C as its locale, though we use en_GB for the rest of our code. The latest vCard specification draft doesn't seem to state any preference for a specific locale and no well-defined vCard attributes differ between en_US and en_GB.

Since this is supposed to only be used by Folks, I think we should make this en_GB as "X-FOLKS-FAVOURITE"

Next library, I'm going to put my foot down for the C locale, so we follow the scheme most other code uses :-p
Comment 5 Travis Reitter 2011-10-25 18:46:17 UTC
Review of attachment 199948 [details] [review]:

tests/eds/set-is-favourite.vala

+      assert (this._is_favourite_before_update);

+              if (!i.is_favourite)
+                {
+                  this._is_favourite_before_update = true;
+                }

These seem wrong -- shouldn't we assert that it's false and set this._is_favourite_before_update = i.is_favourite?
Comment 6 Travis Reitter 2011-10-25 18:46:45 UTC
Review of attachment 199947 [details] [review]:

Seems fine (but please wait to apply along with the final other patches)
Comment 7 Raul Gutierrez Segales 2011-10-25 19:39:24 UTC
(In reply to comment #5)
> Review of attachment 199948 [details] [review]:
> 
> tests/eds/set-is-favourite.vala
> 
> +      assert (this._is_favourite_before_update);
> 
> +              if (!i.is_favourite)
> +                {
> +                  this._is_favourite_before_update = true;
> +                }
> 
> These seem wrong -- shouldn't we assert that it's false and set
> this._is_favourite_before_update = i.is_favourite?

I guess the variable name is misleading and its use not explained: I just wanted to use it to flag that I encountered the is-favourite property of the individual with the expected value before the update (in this case, false).

Anyway, I'll change it as you suggest since that makes it much more readable.
Comment 8 Raul Gutierrez Segales 2011-10-25 19:57:59 UTC
Created attachment 199964 [details] [review]
patch

Updated according to review.
Comment 9 Raul Gutierrez Segales 2011-10-25 19:58:40 UTC
Created attachment 199965 [details] [review]
test setting is-favourite

Updated version of the test case.
Comment 10 Philip Withnall 2011-10-25 20:42:45 UTC
Review of attachment 199964 [details] [review]:

A few minor points, but other than that it looks good to me. As long as Travis is happy, I guess it can be committed with these changes.

::: NEWS
@@ +14,3 @@
 * Add AbstractFieldDetails.values_equal() to compare values (but not parameters)
+* Add is_favourite to Edsf.Persona
+* Add change_is_favourite to Edsf.Persona

Might be easier just to say that you implemented FavouriteDetails on Edsf.Persona.

::: backends/eds/lib/edsf-persona-store.vala
@@ +1237,3 @@
+        {
+          throw new PropertyError.NOT_WRITEABLE (
+              _("Is favourite is not writeable on this contact."));

That sounds a bit weird to me. How about “This contact can not be marked as a favourite.”?
Comment 11 Philip Withnall 2011-10-25 20:46:22 UTC
Review of attachment 199965 [details] [review]:

Looks good to me. (Well, as good as a test case can ever look.)
Comment 12 Travis Reitter 2011-10-25 21:08:36 UTC
Review of attachment 199964 [details] [review]:

I agree with Philip's points. Otherwise, it's fine by me.
Comment 13 Travis Reitter 2011-10-25 21:08:52 UTC
Review of attachment 199965 [details] [review]:

Also good by me.
Comment 14 Raul Gutierrez Segales 2011-10-25 21:22:13 UTC
Last comments addressed and then merged: 

commit 66a18236c6a0c94c12f86a649633d665aca3fdbc
Author: Raul Gutierrez Segales <rgs@collabora.co.uk>
Date:   Tue Oct 25 17:18:41 2011 +0100

    e-d-s: test setting is-favourite property
    
    Helps: https://bugzilla.gnome.org/show_bug.cgi?id=660908

 tests/eds/Makefile.am           |    6 ++
 tests/eds/set-is-favourite.vala |  160 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 166 insertions(+), 0 deletions(-)

commit d7ecae8905572e7cc7f9a3f90e3e411ec15552ac
Author: Raul Gutierrez Segales <rgs@collabora.co.uk>
Date:   Tue Oct 25 17:09:01 2011 +0100

    e-d-s: extend add-persona test to check for is-favourite
    
    Helps: https://bugzilla.gnome.org/show_bug.cgi?id=660908

 tests/eds/add-persona.vala |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

commit 35e425903dff408fc4da4fa6577cb6f8ae4caf17
Author: Raul Gutierrez Segales <rgs@collabora.co.uk>
Date:   Tue Oct 25 16:49:14 2011 +0100

    e-d-s: add favourites support to EDS backend
    
    Fixes: https://bugzilla.gnome.org/show_bug.cgi?id=660908

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