GNOME Bugzilla – Bug 655745
Implement read/write support for gender property
Last modified: 2011-08-03 07:51:16 UTC
Although Edsf.Persona implements the GenderDetails interface, its not actually reading/writing back to e-d-s. This wasn't originally implemented because e-d-s doesn't use it. But as noted by Travis on irc, this is usually done via an X-GENDER attribute.
Created attachment 193047 [details] [review] Implement support for Gender property
Created attachment 193048 [details] [review] Test case for Gender property
Review of attachment 193047 [details] [review]: ::: backends/eds/lib/edsf-persona-store.vala @@ +921,3 @@ + catch (GLib.Error e) + { + GLib.warning ("Can't set gender: %s\n", e.message); Don't need a “\n” at the end of messages which use g_log(). @@ +945,3 @@ + attr.add_value (Edsf.Persona.gender_female); + } + contact.add_attribute (attr); I'd use a switch statement here, since you're operating over an enum. It means the compiler will warn us if we add new values to Gender in future without adding the corresponding case here. ::: backends/eds/lib/edsf-persona.vala @@ +60,3 @@ }; + public static const string gender_male = "MALE"; + public static const string gender_female = "FEMALE"; Are these values what's used by other software? They should be documented regardless. I'd also suggest exposing a (public static const string gender_attribute_name = "X-GENDER") (or something) so that client code doesn't have to hard-code the attribute name we're using if they fancy inspecting the vCard attributes themselves. (If that's not the reason for exposing MALE and FEMALE as public members of Edsf.Persona, please correct me.) @@ +293,1 @@ */ Why remove the @since tag?
Review of attachment 193048 [details] [review]: Looks fine to commit with the minor changes below, once the first patch has been committed. ::: tests/eds/set-gender.vala @@ +63,3 @@ + + v = Value (typeof (string)); + v.set_string ("bernie h. innocenti"); Need a new and more interesting name. Bernie's getting old. @@ +72,3 @@ + this._main_loop.quit (); + assert_not_reached (); + }); Indentation's a bit wacky on this block. @@ +95,3 @@ + catch (GLib.Error e) + { + GLib.warning ("Error when calling prepare: %s\n", e.message); Spurious “\n”.
Created attachment 193110 [details] [review] Implement read/write support for Gender Comments approached.
Created attachment 193111 [details] [review] Test case for Gender property Approached comments.
Review of attachment 193110 [details] [review]: Looks fine once a valadoc comment's added for each of the three new Edsf.Persona members. ::: backends/eds/lib/edsf-persona.vala @@ +63,3 @@ + * http://tools.ietf.org/html/draft-ietf-vcarddav-vcardrev-06 */ + public static const string gender_male = "M"; + public static const string gender_female = "F"; Sorry, by “documented”, I meant with a valadoc comment, since they're public API.
Review of attachment 193111 [details] [review]: Looks good to me.
Created attachment 193113 [details] [review] Implement read/write support for Gender Added valadoc type comments for the new public const members.
Merged: commit dd1a2268d2a49e6bdb4640c4e353fa0df4fb67b4 Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Tue Aug 2 11:47:37 2011 +0100 e-d-s: add test for Gender property Helps: https://bugzilla.gnome.org/show_bug.cgi?id=655745 tests/eds/Makefile.am | 6 ++ tests/eds/set-gender.vala | 154 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 160 insertions(+), 0 deletions(-) commit 21abb433ae11eb38305627ee1778e703c281a6b5 Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Tue Aug 2 11:45:34 2011 +0100 e-d-s: Implement read/write support for gender property Fixes: https://bugzilla.gnome.org/show_bug.cgi?id=655745 NEWS | 1 + backends/eds/lib/edsf-persona-store.vala | 49 ++++++++++++++++++ backends/eds/lib/edsf-persona.vala | 80 ++++++++++++++++++++++++++++-- 3 files changed, 126 insertions(+), 4 deletions(-)