GNOME Bugzilla – Bug 657969
Support RoleDetails in eds backend
Last modified: 2011-09-05 22:50:18 UTC
I'd like this for Gnome Contacts
Created attachment 195595 [details] [review] patch TODO: add test case
Review of attachment 195595 [details] [review]: Looks like a good start, but we need support for multiple roles. As discussed on Jabber, this can be done using an X-ROLES attribute. ::: backends/eds/lib/edsf-persona-store.vala @@ +368,3 @@ + else if (k == Folks.PersonaStore.detail_key (PersonaDetail.ROLES)) + { + var roles = (Gee.HashSet<RoleFieldDetails>) v.get_object (); You can't assume that it's a *Hash*Set. This should be Set<RoleFieldDetails> instead. @@ +1178,3 @@ + if (cur_role.equal (new_role)) + return; + } Obviously, this is going to have to change when you sort out how to deal with multiple roles. @@ +1217,3 @@ + manager = manager_values.to_array ()[0]; + + var assistant_values = role_fd.get_parameter_values ("assistan"); s/assistan"/assistant"/ ::: backends/eds/lib/edsf-persona.vala @@ +894,3 @@ + + /* Should we compare parameters when checking if + 2 Roles are the same? */ RoleFieldDetails.equal() should (be changed to?) do that if necessary, right?
Created attachment 195669 [details] [review] patch Updated according to feedback.
Tried the patch. It broke is_quiescent support as now used by gnome contacts.
Also, there are a few fields in evo that are not in Roles. What do we do about that?
For instance, department, which seems kinda useful.
Also, exposing editing multiple roles is a total non-starter in the UI. Not gonna happen.
Created attachment 195687 [details] [review] updated patch there was a copy-paste error in when retrieving the default role from e-d-s.
(In reply to comment #7) > Also, exposing editing multiple roles is a total non-starter in the UI. Not > gonna happen. I guess its still good to have support for that.
(In reply to comment #6) > For instance, department, which seems kinda useful. As discussed on IRC, that would be org_unit (which isn't a VCard standard but is supported by e-d-s). Any other fields missing? Also, as mentioned on IRC, we might consider promoting some of these fields which are now stored as AbstracFieldDetails parameters into properties of Folks.Role.
(In reply to comment #4) > Tried the patch. It broke is_quiescent support as now used by gnome contacts. This patch is totally unrelated to that, I've tried Contacts without the RoleDetails supports in the e-d-s backend and is_quiescent is still broken (and checking the state of stores via folks-inspect I see a bunch of store with is-quiescent = FALSE). Looking at previous commits.
Yeah, not related, i still got quiescent problems without the patch.
Review of attachment 195687 [details] [review]: OK by me to commit once these issues have been fixed. ::: backends/eds/lib/edsf-persona-store.vala @@ +1361,3 @@ + foreach (var role_fd in roles) + { + if (count == 0) It would probably be a good idea to add a comment about how we treat the first one differently, and put subsequent roles into X-ROLES. @@ +1367,3 @@ + role = role_fd.value.role; + + /* FIXME: we are swallowing the extra parameter values */ I think this is probably fine for the moment. Nobody's going to set them. ::: backends/eds/lib/edsf-persona.vala @@ +581,3 @@ + */ + [CCode (notify = false)] + public Set<RoleFieldDetails> roles Needs a @since line. ::: folks/role-details.vala @@ +191,3 @@ + */ + public static bool equal_sets (Set<RoleFieldDetails> a, + Set<RoleFieldDetails> b) I don't feel comfortable with exposing this publicly, since we could eventually just make RoleFieldDetails implement a Comparable interface and then have a generic equal_sets(Set<Comparable>, Set<Comparable>) function. It's probably best to move this into the EDS backend and make it private, for the moment. ::: tests/eds/set-roles.vala @@ +34,3 @@ + public SetRolesTests () + { + base ("SetUrls"); Copy–paste error?
Merged: commit 21115d35329c10b5136880117edaf66d5c698a9a Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Sun Sep 4 15:21:44 2011 +0100 e-d-s: add test case for settings roles commit c0969ff08c7f67ea0118e24e7b9d5c70175e13e9 Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Sat Sep 3 20:30:12 2011 +0100 e-d-s: extend add persona test to handle Roles commit 6d5040f72929231894e02632827ba9135552ef5b Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Sat Sep 3 18:28:09 2011 +0100 e-d-s: implement support for RoleDetails Fixes: https://bugzilla.gnome.org/show_bug.cgi?id=657969 commit 50a9aebbf9694e04030c4528ce93751396728b4f Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Mon Sep 5 23:33:08 2011 +0100 build: tests don't use <folks/*.h> so add path to folks/