From d5a4faa0b786fb0248f05012cec8fdaabce7791d Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 15 Nov 2013 11:16:22 +0000 Subject: bluez: Reimplement vCard parsing when updating Personas MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- backends/bluez/Makefile.am | 1 + backends/bluez/bluez-persona.vala | 305 +++++++++++++++++++++++--------------- 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? _urls = null; - private Set? _urls_ro = null; - private LoadableIcon? _avatar = null; - private HashSet _phone_numbers; - private Set _phone_numbers_ro; - private HashSet _email_addresses; - private Set _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? _urls = null; + private Set _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? _phone_numbers = null; + private Set _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? _email_addresses = null; + private Set _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 (); + this._phone_numbers = new SmallSet ( + AbstractFieldDetails.hash_static, + AbstractFieldDetails.equal_static); this._phone_numbers_ro = this._phone_numbers.read_only_view; - - this._email_addresses = new HashSet (); + this._email_addresses = new SmallSet ( + AbstractFieldDetails.hash_static, + AbstractFieldDetails.equal_static); this._email_addresses_ro = this._email_addresses.read_only_view; - - this._urls = new HashSet (); + this._urls = new SmallSet ( + AbstractFieldDetails.hash_static, + AbstractFieldDetails.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 ( + AbstractFieldDetails.hash_static, + AbstractFieldDetails.equal_static); + var new_uris = new SmallSet ( + AbstractFieldDetails.hash_static, + AbstractFieldDetails.equal_static); + var new_email_addresses = new SmallSet ( + AbstractFieldDetails.hash_static, + AbstractFieldDetails.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 (); + /* 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 attrs = + card.get_attributes (); - if (attribute != null) + foreach (var attr in attrs) { - unowned GLib.List 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 (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 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 (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 (); - - if (attribute != null) - { - unowned GLib.List vals = - attribute.get_values_decoded (); - foreach (unowned StringBuilder v in vals) - new_uris.add (new UrlFieldDetails (v.str)); - } - if (!Folks.Internal.equal_sets (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 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 (); - - if (attribute != null) - { - unowned GLib.List vals = - attribute.get_values_decoded (); - foreach (unowned StringBuilder v in vals) - new_email_addresses.add (new EmailFieldDetails (v.str)); - } - if (!Folks.Internal.equal_sets (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; -- cgit v1.2.3