Change GroupDetails.groups to be a Set<string>

Helps: bgo#640092

6 files changed, 43 insertions, 49 deletions

diff --git a/NEWS b/NEWS
index 04cb364..dde2f31 100644
--- a/NEWS
+++ b/NEWS
@@ -17,6 +17,7 @@ API changes:
   MultiMap<string, string>
 * Removed LinkedHashSet in favour of Gee.HashSet
 * Backend.persona_stores is now of type Map<string, PersonaStore>
+* GroupDetails.groups is now of type Set<string>
 
 Overview of changes from libfolks 0.4.0 to libfolks 0.5.0
 =========================================================
diff --git a/backends/telepathy/lib/tpf-persona.vala b/backends/telepathy/lib/tpf-persona.vala
index fbc259c..0e2a09e 100644
--- a/backends/telepathy/lib/tpf-persona.vala
+++ b/backends/telepathy/lib/tpf-persona.vala
@@ -35,7 +35,7 @@ public class Tpf.Persona : Folks.Persona,
     ImDetails,
     PresenceDetails
 {
-  private HashTable<string, bool> _groups;
+  private HashSet<string> _groups;
   private bool _is_favourite;
   private string _alias;
   private HashMultiMap<string, string> _im_addresses;
@@ -146,25 +146,23 @@ public class Tpf.Persona : Folks.Persona,
    *
    * See {@link Folks.GroupDetails.groups}.
    */
-  public HashTable<string, bool> groups
+  public Set<string> groups
     {
       get { return this._groups; }
 
       set
         {
-          value.foreach ((k, v) =>
+          foreach (var group in value)
             {
-              unowned string group = (string) k;
-              if (this._groups.lookup (group) == false)
+              if (this._groups.contains (group) == false)
                 this._change_group (group, true);
-            });
+            }
 
-          this._groups.foreach ((k, v) =>
+          foreach (var group in this._groups)
             {
-              unowned string group = (string) k;
-              if (value.lookup (group) == false)
+              if (value.contains (group) == false)
                 this._change_group (group, true);
-            });
+            }
 
           /* Since we're only changing the members of this._groups, rather than
            * replacing it with a new instance, we have to manually emit the
@@ -193,9 +191,9 @@ public class Tpf.Persona : Folks.Persona,
 
       if (is_member)
         {
-          if (this._groups.lookup (group) != true)
+          if (!this._groups.contains (group))
             {
-              this._groups.insert (group, true);
+              this._groups.add (group);
               changed = true;
             }
         }
@@ -269,7 +267,7 @@ public class Tpf.Persona : Folks.Persona,
         }
 
       /* Groups */
-      this._groups = new HashTable<string, bool> (str_hash, str_equal);
+      this._groups = new HashSet<string> ();
 
       contact.notify["avatar-file"].connect ((s, p) =>
         {
diff --git a/folks/group-details.vala b/folks/group-details.vala
index 8244dc4..7fc167c 100644
--- a/folks/group-details.vala
+++ b/folks/group-details.vala
@@ -19,6 +19,7 @@
  */
 
 using GLib;
+using Gee;
 
 /**
  * Interface for {@link Persona}s or {@link Individual}s which can be grouped
@@ -117,8 +118,10 @@ public interface Folks.GroupDetails : Object
    *
    * Freeform group IDs are mapped to a boolean which is `true` if the
    * contact is a member of the group, and `false` otherwise.
+   *
+   * @since UNRELEASED
    */
-  public abstract HashTable<string, bool> groups { get; set; }
+  public abstract Set<string> groups { get; set; }
 
   /**
    * Add or remove the contact from the specified group.
diff --git a/folks/individual.vala b/folks/individual.vala
index 2281a54..5e67770 100644
--- a/folks/individual.vala
+++ b/folks/individual.vala
@@ -83,7 +83,7 @@ public class Folks.Individual : Object,
 {
   private bool _is_favourite;
   private string _alias;
-  private HashTable<string, bool> _groups;
+  private HashSet<string> _groups;
   /* These two data structures should store exactly the same set of Personas:
    * the Personas contained in this Individual. The HashSet is used for fast
    * lookups, whereas the List is used for iteration.
@@ -402,18 +402,18 @@ public class Folks.Individual : Object,
   /**
    * {@inheritDoc}
    */
-  public HashTable<string, bool> groups
+  public Set<string> groups
     {
       get { return this._groups; }
 
       set
         {
-          this._groups = value;
           this._persona_list.foreach ((p) =>
             {
               if (p is GroupDetails && ((Persona) p).store.is_writeable == true)
                 ((GroupDetails) p).groups = value;
             });
+          this._update_groups ();
         }
     }
 
@@ -691,11 +691,11 @@ public class Folks.Individual : Object,
 
   private void _update_groups ()
     {
-      var new_groups = new HashTable<string, bool> (str_hash, str_equal);
+      var new_groups = new HashSet<string> ();
 
       /* this._groups is null during initial construction */
       if (this._groups == null)
-        this._groups = new HashTable<string, bool> (str_hash, str_equal);
+        this._groups = new HashSet<string> ();
 
       /* FIXME: this should partition the personas by store (maybe we should
        * keep that mapping in general in this class), and execute
@@ -708,37 +708,34 @@ public class Folks.Individual : Object,
             {
               var persona = (GroupDetails) p;
 
-              persona.groups.foreach ((k, v) =>
+              foreach (var group in persona.groups)
                 {
-                  new_groups.insert ((string) k, true);
-                });
+                  new_groups.add (group);
+                }
             }
         });
 
-      new_groups.foreach ((k, v) =>
+      foreach (var group in new_groups)
         {
-          unowned string group = (string) k;
-          if (this._groups.lookup (group) != true)
+          if (!this._groups.contains (group))
             {
-              this._groups.insert (group, true);
-              this._groups.foreach ((k, v) =>
+              this._groups.add (group);
+              foreach (var g in this._groups)
                 {
-                  unowned string g = (string) k;
                   debug ("   %s", g);
-                });
+                }
 
               this.group_changed (group, true);
             }
-        });
+        }
 
       /* buffer the removals, so we don't remove while iterating */
       var removes = new GLib.List<string> ();
-      this._groups.foreach ((k, v) =>
+      foreach (var group in this._groups)
         {
-          unowned string group = (string) k;
-          if (new_groups.lookup (group) != true)
+          if (!new_groups.contains (group))
             removes.prepend (group);
-        });
+        }
 
       removes.foreach ((l) =>
         {
diff --git a/tests/telepathy/individual-properties.vala b/tests/telepathy/individual-properties.vala
index 20a4c77..d671716 100644
--- a/tests/telepathy/individual-properties.vala
+++ b/tests/telepathy/individual-properties.vala
@@ -66,9 +66,9 @@ public class IndividualPropertiesTests : Folks.TestCase
               assert (((PresenceDetails) i).is_online () == true);
 
               /* Check groups */
-              assert (i.groups.size () == 2);
-              assert (i.groups.lookup ("Montreal") == true);
-              assert (i.groups.lookup ("Francophones") == true);
+              assert (i.groups.size == 2);
+              assert (i.groups.contains ("Montreal") == true);
+              assert (i.groups.contains ("Francophones") == true);
             }
 
           assert (removed == null);
diff --git a/tools/inspect/utils.vala b/tools/inspect/utils.vala
index a544f71..37351e4 100644
--- a/tools/inspect/utils.vala
+++ b/tools/inspect/utils.vala
@@ -236,22 +236,17 @@ private class Folks.Inspect.Utils
         }
       else if (prop_name == "groups")
         {
-          HashTable<string, bool> groups =
-              (HashTable<string, bool>) prop_value.get_boxed ();
+          Set<string> groups = (Set<string>) prop_value.get_object ();
           output_string = "{ ";
           bool first = true;
 
-          /* FIXME: This is rather inefficient */
-          groups.foreach ((k, v) =>
+          foreach (var group in groups)
             {
-              if ((bool) v == true)
-                {
-                  if (first == false)
-                    output_string += ", ";
-                  output_string += "'%s'".printf ((string) k);
-                  first = false;
-                }
-            });
+              if (first == false)
+                output_string += ", ";
+              output_string += "'%s'".printf (group);
+              first = false;
+            }
 
           output_string += " }";
           return output_string;