bluez: Reimplement vCard parsing when updating Personas

This completely re-implements parsing of the vCards returned from the
Bluetooth device, with the following aims:
 • Improve efficiency.
 • Correctly support multi-instance attributes (such as TEL and EMAIL).
 • Fix comparison of new and old values to reduce property notification
   spew.

Previously, the code was calling E.VCard.get_attribute() for each
attribute it expected, which iterates over the vCard’s entire attribute
list every time. Now, the code iterates over the attribute list once and
checks for the attributes it expects. This also means that unsupported
attributes can be detected. The performance improvement from this should
come from reduced iteration and better cache utilisation, although it
has not been measured. If further performance improvements are needed,
the attribute name strings could be interned and pointer comparisons
used instead of lots of strcmp()s.

The code was also previously using E.VCardAttribute.get_values() to get
the values of TEL, EMAIL and URL attributes, which is incorrect, as
those attributes are single-valued but may exist several times in the
vCard. Consequently, the code previously only parsed the first telephone
number, e-mail address and phone number in the vCard and ignored the
rest. This has now been fixed.

Finally, the code was doing pointer comparisons of elements in the
phone, e-mail and URI sets, and hence was always detecting them as
different (even if strcmp() comparison would have yielded equality).
This was causing excess property notification spew. This has been fixed
by correctly specifying the hash and equality functions to use for the
Sets.

Additionally, the code has been ported to use SmallSet instead of
HashSet. The memory and performance improvements of this change have not
been measured.

https://bugzilla.gnome.org/show_bug.cgi?id=712274

2 files changed, 183 insertions, 123 deletions

diff --git a/backends/bluez/Makefile.am b/backends/bluez/Makefile.am
index 9cae3823..f644687a 100644
--- a/backends/bluez/Makefile.am
+++ b/backends/bluez/Makefile.am
@@ -7,6 +7,7 @@ backend_LTLIBRARIES = bluez.la
 bluez_la_VALAFLAGS = \
 	$(backend_valaflags) \
 	--pkg libebook-1.2 \
+	--pkg folks-generics \
 	$(NULL)
 
 bluez_la_SOURCES = \
diff --git a/backends/bluez/bluez-persona.vala b/backends/bluez/bluez-persona.vala
index b93eb469..ac6e2d5c 100644
--- a/backends/bluez/bluez-persona.vala
+++ b/backends/bluez/bluez-persona.vala
@@ -41,17 +41,6 @@ public class Folks.Backends.BlueZ.Persona : Folks.Persona,
     PhoneDetails,
     UrlDetails
 {
-  private StructuredName? _structured_name = null;
-  private string _full_name = "";
-  private string _nickname = "";
-  private Set<UrlFieldDetails>? _urls = null;
-  private Set<UrlFieldDetails>? _urls_ro = null;
-  private LoadableIcon? _avatar = null;
-  private HashSet<PhoneFieldDetails> _phone_numbers;
-  private Set<PhoneFieldDetails> _phone_numbers_ro;
-  private HashSet<EmailFieldDetails> _email_addresses;
-  private Set<EmailFieldDetails> _email_addresses_ro;
-
   private const string[] _linkable_properties =
     {
       "phone-numbers",
@@ -69,6 +58,9 @@ public class Folks.Backends.BlueZ.Persona : Folks.Persona,
       get { return BlueZ.Persona._linkable_properties; }
     }
 
+  private SmallSet<UrlFieldDetails>? _urls = null;
+  private Set<UrlFieldDetails> _urls_ro;
+
   /**
    * {@inheritDoc}
    *
@@ -81,6 +73,8 @@ public class Folks.Backends.BlueZ.Persona : Folks.Persona,
       set { this.change_urls.begin (value); } /* not writeable */
     }
 
+  private LoadableIcon? _avatar = null;
+
   /**
   * {@inheritDoc}
   *
@@ -103,6 +97,9 @@ public class Folks.Backends.BlueZ.Persona : Folks.Persona,
       get { return BlueZ.Persona._writeable_properties; }
     }
 
+  private SmallSet<PhoneFieldDetails>? _phone_numbers = null;
+  private Set<PhoneFieldDetails> _phone_numbers_ro;
+
   /**
    * {@inheritDoc}
    *
@@ -115,6 +112,8 @@ public class Folks.Backends.BlueZ.Persona : Folks.Persona,
       set { this.change_phone_numbers.begin (value); } /* not writeable */
     }
 
+  private StructuredName? _structured_name = null;
+
   /**
    * {@inheritDoc}
    *
@@ -127,6 +126,8 @@ public class Folks.Backends.BlueZ.Persona : Folks.Persona,
       set { this.change_structured_name.begin (value); } /* not writeable */
     }
 
+  private string _full_name = "";
+
   /**
    * {@inheritDoc}
    *
@@ -139,6 +140,8 @@ public class Folks.Backends.BlueZ.Persona : Folks.Persona,
       set { this.change_full_name.begin (value); } /* not writeable */
     }
 
+  private string _nickname = "";
+
   /**
    * {@inheritDoc}
    *
@@ -151,6 +154,9 @@ public class Folks.Backends.BlueZ.Persona : Folks.Persona,
       set { this.change_nickname.begin (value); } /* not writeable */
     }
 
+  private SmallSet<EmailFieldDetails>? _email_addresses = null;
+  private Set<EmailFieldDetails> _email_addresses_ro;
+
   /**
    * {@inheritDoc}
    *
@@ -197,16 +203,35 @@ public class Folks.Backends.BlueZ.Persona : Folks.Persona,
     {
       debug ("Adding BlueZ Persona '%s' (IID '%s')", this.uid, this.iid);
 
-      this._phone_numbers = new HashSet<PhoneFieldDetails> ();
+      this._phone_numbers = new SmallSet<PhoneFieldDetails> (
+           AbstractFieldDetails<string>.hash_static,
+           AbstractFieldDetails<string>.equal_static);
       this._phone_numbers_ro = this._phone_numbers.read_only_view;
-
-      this._email_addresses = new HashSet<EmailFieldDetails> ();
+      this._email_addresses = new SmallSet<EmailFieldDetails> (
+           AbstractFieldDetails<string>.hash_static,
+           AbstractFieldDetails<string>.equal_static);
       this._email_addresses_ro = this._email_addresses.read_only_view;
-
-      this._urls = new HashSet<UrlFieldDetails> ();
+      this._urls = new SmallSet<UrlFieldDetails> (
+           AbstractFieldDetails<string>.hash_static,
+           AbstractFieldDetails<string>.equal_static);
       this._urls_ro = this._urls.read_only_view;
     }
 
+  private void _update_params (AbstractFieldDetails details,
+      E.VCardAttribute attr)
+    {
+      foreach (unowned E.VCardAttributeParam param in attr.get_params ())
+        {
+          /* EVCard handles parameter names and values entirely
+           * case-insensitively, so we’ll do the same. */
+          foreach (unowned string param_value in param.get_values ())
+            {
+              details.add_parameter (param.get_name ().down (),
+                  param_value.down ());
+            }
+        }
+    }
+
   /**
    * Update the Persona’s properties from a vCard.
    *
@@ -222,69 +247,136 @@ public class Folks.Backends.BlueZ.Persona : Folks.Persona,
     {
       var properties_changed = false;
 
-      this.freeze_notify ();
+      /* Somewhere to store the new property values. */
+      var new_phone_numbers = new SmallSet<PhoneFieldDetails> (
+          AbstractFieldDetails<string>.hash_static,
+          AbstractFieldDetails<string>.equal_static);
+      var new_uris = new SmallSet<UrlFieldDetails> (
+          AbstractFieldDetails<string>.hash_static,
+          AbstractFieldDetails<string>.equal_static);
+      var new_email_addresses = new SmallSet<EmailFieldDetails> (
+          AbstractFieldDetails<string>.hash_static,
+          AbstractFieldDetails<string>.equal_static);
+      BytesIcon? new_avatar = null;
+      var new_full_name = "";
+      var new_nickname = "";
+      StructuredName? new_structured_name = null;
 
-      /* Phone numbers. */
-      var attribute = card.get_attribute ("TEL");
-      var new_phone_numbers = new HashSet<PhoneFieldDetails> ();
+      /* Parse the attributes by iterating over the vCard’s attribute list once
+       * only. Convenience functions like E.VCard.get_attribute() cause multiple
+       * iterations over the list. */
+      unowned GLib.List<unowned E.VCardAttribute> attrs =
+          card.get_attributes ();
 
-      if (attribute != null)
+      foreach (var attr in attrs)
         {
-          unowned GLib.List<unowned StringBuilder> vals =
-              attribute.get_values_decoded ();
-          foreach (unowned StringBuilder v in vals)
-              new_phone_numbers.add (new PhoneFieldDetails (v.str));
-        }
+          unowned string attr_name = attr.get_name ();
 
-      if (!Folks.Internal.equal_sets<PhoneFieldDetails> (this._phone_numbers,
-              new_phone_numbers))
-        {
-          this._phone_numbers = new_phone_numbers;
-          this._phone_numbers_ro = new_phone_numbers.read_only_view;
-          this.notify_property ("phone-numbers");
-          properties_changed = true;
-        }
+          if (attr_name == "TEL")
+            {
+              var val = attr.get_value ();
+              if (val == null || (!) val == "")
+                  continue;
 
-      /* Full name. */
-      attribute = card.get_attribute ("FN");
-      var new_full_name = "";
+              var new_field_details = new PhoneFieldDetails ((!) val);
+              this._update_params (new_field_details, attr);
+              new_phone_numbers.add (new_field_details);
+            }
+          else if (attr_name == "URL")
+            {
+              var val = attr.get_value ();
+              if (val == null || (!) val == "")
+                  continue;
 
-      if (attribute != null)
-          new_full_name = attribute.get_value_decoded ().str;
+              var new_field_details = new UrlFieldDetails ((!) val);
+              this._update_params (new_field_details, attr);
+              new_uris.add (new_field_details);
+            }
+          else if (attr_name == "EMAIL")
+            {
+              var val = attr.get_value ();
+              if (val == null || (!) val == "")
+                  continue;
 
-      if (this._full_name != new_full_name)
-        {
-          this._full_name = new_full_name;
-          this.notify_property ("full-name");
-          properties_changed = true;
+              var new_field_details = new EmailFieldDetails ((!) val);
+              this._update_params (new_field_details, attr);
+              new_email_addresses.add (new_field_details);
+            }
+          else if (attr_name == "PHOTO")
+            {
+              var encoded_data = (string) attr.get_value ().data;
+              var bytes = new Bytes (Base64.decode (encoded_data));
+              new_avatar = new BytesIcon (bytes);
+            }
+          else if (attr_name == "FN")
+              new_full_name = attr.get_value ();
+          else if (attr_name == "NICKNAME")
+              new_nickname = attr.get_value ();
+          else if (attr_name == "N")
+            {
+              unowned GLib.List<unowned string> values = attr.get_values ();
+              unowned string? family_name = null, given_name = null,
+                  additional_names = null, prefixes = null, suffixes = null;
+
+              if (values != null)
+                {
+                  family_name = values.data;
+                  values = values.next;
+                }
+              if (values != null)
+                {
+                  given_name = values.data;
+                  values = values.next;
+                }
+              if (values != null)
+                {
+                  additional_names = values.data;
+                  values = values.next;
+                }
+              if (values != null)
+                {
+                  prefixes = values.data;
+                  values = values.next;
+                }
+              if (values != null)
+                {
+                  suffixes = values.data;
+                  values = values.next;
+                }
+
+              if (suffixes == null || values != null)
+                {
+                  debug ("Expected 5 components in N attribute of vCard, " +
+                      "but got %s.", (suffixes == null) ? "fewer" : "more");
+                }
+
+              new_structured_name =
+                  new StructuredName (family_name, given_name, additional_names,
+                      prefixes, suffixes);
+            }
+          else if (attr_name != "VERSION" && attr_name != "UID")
+            {
+              /* Unknown attribute. */
+              warning ("Unknown attribute ‘%s’ in vCard for persona %s.",
+                  attr_name, this.uid);
+            }
         }
 
-      /* Nickname. */
-      attribute = card.get_attribute ("NICKNAME");
-      var new_nickname = "";
-
-      if (attribute != null)
-          new_nickname = attribute.get_value_decoded ().str;
+      /* Now test the new property values to see if they’ve changed; if so, emit
+       * property change notifications. */
+      this.freeze_notify ();
 
-      if (this._nickname != new_nickname)
+      /* Phone numbers. */
+      if (!Folks.Internal.equal_sets<PhoneFieldDetails> (this._phone_numbers,
+              new_phone_numbers))
         {
-          this._nickname = new_nickname;
-          this.notify_property ("nickname");
+          this._phone_numbers = new_phone_numbers;
+          this._phone_numbers_ro = new_phone_numbers.read_only_view;
+          this.notify_property ("phone-numbers");
           properties_changed = true;
         }
 
       /* URIs. */
-      attribute = card.get_attribute ("URL");
-      var new_uris = new HashSet<UrlFieldDetails> ();
-
-      if (attribute != null)
-        {
-          unowned GLib.List<unowned StringBuilder> vals =
-              attribute.get_values_decoded ();
-          foreach (unowned StringBuilder v in vals)
-              new_uris.add (new UrlFieldDetails (v.str));
-        }
-
       if (!Folks.Internal.equal_sets<UrlFieldDetails> (this._urls, new_uris))
         {
           this._urls = new_uris;
@@ -293,56 +385,7 @@ public class Folks.Backends.BlueZ.Persona : Folks.Persona,
           properties_changed = true;
         }
 
-      /* Structured name. */
-      attribute = card.get_attribute ("N");
-      StructuredName? new_structured_name = null;
-
-      if (attribute != null)
-        {
-          string[] components = { "", "", "", "", "" };
-          unowned GLib.List<unowned StringBuilder> values =
-              attribute.get_values_decoded ();
-
-          uint i = 0;
-          foreach (unowned StringBuilder b in values)
-            {
-              if (i >= components.length)
-                  break;
-
-              components[i++] = b.str;
-            }
-
-          this._structured_name = new StructuredName (components[0],
-              components[1], components[2], components[3], components[4]);
-
-          if (i != 5)
-            {
-              debug ("Expected 5 components in N value of vCard, but got %u.",
-                  i);
-            }
-        }
-
-      if ((new_structured_name == null) != (this._structured_name == null) ||
-          (new_structured_name != null && this._structured_name != null &&
-           !new_structured_name.equal (this._structured_name)))
-        {
-          this._structured_name = new_structured_name;
-          this.notify_property ("structured-name");
-          properties_changed = true;
-        }
-
       /* E-mail addresses. */
-      attribute = card.get_attribute ("EMAIL");
-      var new_email_addresses = new HashSet<EmailFieldDetails> ();
-
-      if (attribute != null)
-        {
-          unowned GLib.List<unowned StringBuilder> vals =
-              attribute.get_values_decoded ();
-          foreach (unowned StringBuilder v in vals)
-              new_email_addresses.add (new EmailFieldDetails (v.str));
-        }
-
       if (!Folks.Internal.equal_sets<EmailFieldDetails> (this._email_addresses,
               new_email_addresses))
         {
@@ -353,16 +396,6 @@ public class Folks.Backends.BlueZ.Persona : Folks.Persona,
         }
 
       /* Photo. */
-      attribute = card.get_attribute ("PHOTO");
-      BytesIcon? new_avatar = null;
-
-      if (attribute != null)
-        {
-          var encoded_data = (string) attribute.get_value ().data;
-          var bytes = new Bytes (Base64.decode (encoded_data));
-          new_avatar = new BytesIcon (bytes);
-        }
-
       if ((new_avatar == null) != (this._avatar == null) ||
           (new_avatar != null && this._avatar != null &&
            !new_avatar.equal (this._avatar)))
@@ -372,6 +405,32 @@ public class Folks.Backends.BlueZ.Persona : Folks.Persona,
           properties_changed = true;
         }
 
+      /* Full name. */
+      if (this._full_name != new_full_name)
+        {
+          this._full_name = new_full_name;
+          this.notify_property ("full-name");
+          properties_changed = true;
+        }
+
+      /* Nickname. */
+      if (this._nickname != new_nickname)
+        {
+          this._nickname = new_nickname;
+          this.notify_property ("nickname");
+          properties_changed = true;
+        }
+
+      /* Structured name. */
+      if ((new_structured_name == null) != (this._structured_name == null) ||
+          (new_structured_name != null && this._structured_name != null &&
+           !new_structured_name.equal (this._structured_name)))
+        {
+          this._structured_name = new_structured_name;
+          this.notify_property ("structured-name");
+          properties_changed = true;
+        }
+
       this.thaw_notify ();
 
       return properties_changed;