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 657969 - Support RoleDetails in eds backend
Support RoleDetails in eds backend
Status: RESOLVED FIXED
Product: folks
Classification: Platform
Component: e-d-s backend
git master
Other Linux
: Normal normal
: folks-0.6.2
Assigned To: folks-maint
folks-maint
Depends on:
Blocks: 657970 657972
 
 
Reported: 2011-09-01 17:26 UTC by Alexander Larsson
Modified: 2011-09-05 22:50 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (8.32 KB, patch)
2011-09-03 18:39 UTC, Raul Gutierrez Segales
needs-work Details | Review
patch (21.20 KB, patch)
2011-09-05 08:51 UTC, Raul Gutierrez Segales
none Details | Review
updated patch (21.20 KB, patch)
2011-09-05 12:33 UTC, Raul Gutierrez Segales
reviewed Details | Review

Description Alexander Larsson 2011-09-01 17:26:10 UTC
I'd like this for Gnome Contacts
Comment 1 Raul Gutierrez Segales 2011-09-03 18:39:26 UTC
Created attachment 195595 [details] [review]
patch

TODO: add test case
Comment 2 Philip Withnall 2011-09-03 18:48:39 UTC
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?
Comment 3 Raul Gutierrez Segales 2011-09-05 08:51:03 UTC
Created attachment 195669 [details] [review]
patch

Updated according to feedback.
Comment 4 Alexander Larsson 2011-09-05 12:01:48 UTC
Tried the patch. It broke is_quiescent support as now used by gnome contacts.
Comment 5 Alexander Larsson 2011-09-05 12:03:12 UTC
Also, there are a few fields in evo that are not in Roles. What do we do about that?
Comment 6 Alexander Larsson 2011-09-05 12:04:42 UTC
For instance, department, which seems kinda useful.
Comment 7 Alexander Larsson 2011-09-05 12:08:36 UTC
Also, exposing editing multiple roles is a total non-starter in the UI. Not gonna happen.
Comment 8 Raul Gutierrez Segales 2011-09-05 12:33:31 UTC
Created attachment 195687 [details] [review]
updated patch

there was a copy-paste error in when retrieving the default role from e-d-s.
Comment 9 Raul Gutierrez Segales 2011-09-05 12:52:03 UTC
(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.
Comment 10 Raul Gutierrez Segales 2011-09-05 12:54:00 UTC
(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.
Comment 11 Raul Gutierrez Segales 2011-09-05 13:41:28 UTC
(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.
Comment 12 Alexander Larsson 2011-09-05 18:54:59 UTC
Yeah, not related, i still got quiescent problems without the patch.
Comment 13 Philip Withnall 2011-09-05 19:07:02 UTC
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?
Comment 14 Raul Gutierrez Segales 2011-09-05 22:50:18 UTC
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/