GNOME Bugzilla – Bug 660908
Add favourites support to EDS backend
Last modified: 2011-10-25 21:22:13 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.
Created attachment 199944 [details] [review] patch Implementation of FavouriteDetails for Edsf.Persona. I'll follow up with a test case.
Created attachment 199947 [details] [review] extend add persona Extend the add-persona test case so it checks for is-favourite too.
Created attachment 199948 [details] [review] test setting is-favourite Test setting is-favourite on an existing persona.
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
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?
Review of attachment 199947 [details] [review]: Seems fine (but please wait to apply along with the final other patches)
(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.
Created attachment 199964 [details] [review] patch Updated according to review.
Created attachment 199965 [details] [review] test setting is-favourite Updated version of the test case.
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.”?
Review of attachment 199965 [details] [review]: Looks good to me. (Well, as good as a test case can ever look.)
Review of attachment 199964 [details] [review]: I agree with Philip's points. Otherwise, it's fine by me.
Review of attachment 199965 [details] [review]: Also good by me.
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(-)