Created attachment 259887 [details] [review]
eds: Remove unused variable

Created attachment 259888 [details] [review]
key-file: Add missing licencing header

Created attachment 259889 [details] [review]
build: Factor common test build rules into test.mk

Simplify the build system a little.

Created attachment 259890 [details] [review]
build: Rename AM_VALAFLAGS to TARGET_VALAFLAGS in configure.ac

Remembering to always append to AM_VALAFLAGS, rather than define it from
scratch, is hard and error-prone. Simpler and more consistent to define a
new variable, TARGET_VALAFLAGS, which must be manually included in every
AM_VALAFLAGS or target-specific *_VALAFLAGS declaration.

Created attachment 259891 [details] [review]
build: Move backend.mk inclusions to the top of the files

Make it more obvious, and also make it non-optional.

Created attachment 259892 [details] [review]
tests: Add support for an isolated system bus

In addition to an isolated session bus, the tests are now run with an
isolated system bus as well. This is needed for the BlueZ backend, which
uses the org.bluez service on the system bus.

This depends on the GLib.TestDBus changes here:
https://bugzilla.gnome.org/show_bug.cgi?id=712148

Created attachment 259893 [details] [review]
tests: Add a function for asserting personas are in an aggregator

This will be useful for testing the behaviour of individual
PersonaStores.

Created attachment 259894 [details] [review]
tests: Add python-dbusmock support to the base TestCase class

This allows derived test cases to easily use python-dbusmock to mock up
D-Bus services which are used by the code under test. The derived code
simply needs to call:
    this.create_dbusmock_service (BusType.SESSION, "org.foo", "foo")
to allow instantiation of the ‘org.foo’ service using the ‘foo’
python-dbusmock template.

Created attachment 259895 [details] [review]
bluez: Add a test suite

This adds a test suite for the BlueZ backend, using a python-dbusmock
mock up of the BlueZ and OBEX D-Bus services.

This requires the latest version of python-dbusmock, plus up-to-date
versions of GLib and Vala for binding updates. The use of the second and
third arguments to AM_PROG_VALAC in configure.ac also necessitates use
of automake 1.12 or newer.

Only a few test cases have been added so far, covering vCard parsing and
general set up of PersonaStores. Using python-dbusmock it should be easy
to add more tests covering advanced Bluetooth device
appearance/disappearance situations in future.

Created attachment 259896 [details] [review]
bluez: Remove BluezBackendFactory class

It was utterly pointless.

Created attachment 259897 [details] [review]
core: Unprepare all backends when destroying the BackendStore

This helps free up some reference cycles (particularly in the BlueZ
backend) and hence ensure no memory is leaked.

Created attachment 259899 [details] [review]
bluez: Add some more debug output

Created attachment 259900 [details] [review]
bluez: Handle the optional Filename property of BlueZ

Previously the code assumed that Filename was always set, but it’s
actually an optional property. Gracefully handle it not existing.

Created attachment 259901 [details] [review]
bluez: Don’t warn about the store being offline

If the Bluetooth device is disconnected by the user between updates of
the contact metadata, the code previously printed a warning when the
next update attempt failed. Instead, just gracefully and silently fail
and schedule the next update.

Created attachment 259902 [details] [review]
bluez: Fix removal of a Map element while iterating over the Map

Created attachment 259903 [details] [review]
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.

Created attachment 259904 [details] [review]
bluez: Calculate Persona IID checksums without the vCard PHOTO attribute

Persona IIDs can come from two places:
 • the vCard UID attribute (preferred); or
 • a checksum of the vCard’s string representation.
Previously, the checksum was calculated over all fields in the vCard,
which interacted very badly with download_photos mode. When downloading
photos, only the PHOTO and UID attributes would be downloaded;
otherwise, all attributes except PHOTO would be downloaded. Obviously,
this meant that it was impossible to calculate the same checksum for a
Persona in download_photos and non-download_photos mode. This meant that
matching the photos up to the existing Personas was impossible. Sad
times.

Now, when in download_photos mode, download all vCard attributes
(including the PHOTO) but calculate the IID checksum over all the
attributes except the PHOTO. This should yield the same checksum as from
non-download_photos mode, unless one of the other vCard properties has
changed between downloads; a new checksum would be produced then, which
is the expected behaviour.

Created attachment 259905 [details] [review]
bluez: Don’t warn if removing an OBEX session fails due to cancellation

Or due to the connection being closed.

Created attachment 259906 [details] [review]
bluez: Correctly handle OBEX transfers which take zero time

If an OBEX transfer takes zero time (say, in an ideal world or, perhaps,
in a unit test) the transfer Status property will start out as
‘complete’ and never change. Previously, completed transfers were only
detected on receipt of a property change notification which, in this
situation, never came. Now, the initial value of the Status property is
also checked.

Created attachment 259907 [details] [review]
bluez: Allow persona store update poll frequency to be modified

For unit testing, it’s nice to not have to wait 5s between polled
updates of the BlueZ persona store. Add a new environment variable,
FOLKS_BLUEZ_TIMEOUT_DIVISOR, which allows the poll frequency to be
divided by the specified amount.

This should speed up the unit tests 100×.

Created attachment 259908 [details] [review]
eds: Remove redundant variable assignment

This depends on the following changes in python-dbusmock: https://code.launchpad.net/~drbob/python-dbusmock/python-dbusmock/+merge/195977

Review of attachment 259887 [details] [review]:

Looks good

Review of attachment 259887 [details] [review]:

Looks good

Review of attachment 259887 [details] [review]:

Looks good

Review of attachment 259888 [details] [review]:

Looks good

Review of attachment 259889 [details] [review]:

I haven't built it myself, but it looks good on reading

Review of attachment 259890 [details] [review]:

Looks good

Review of attachment 259891 [details] [review]:

Looks good

Review of attachment 259892 [details] [review]:

Looks good (but not tested). Merge once the dependent code is available in the stable releases of the main distros.

Review of attachment 259893 [details] [review]:

Looks good.

Review of attachment 259894 [details] [review]:

Don't we need to add python-dbusmock as a dependency?

It obviously shouldn't have to be a regular build dependency but maybe be need to break out the tests as a configure flag which can add that as a build dep.

Attachment #259887 [details] can’t actually be pushed yet, since it depends on a patch in bug #711827 (sorry, I’d forgotten about that dependency). I’ll push it as soon as that branch is reviewed and merged.

Thanks for the reviews! Pushed with all test cases passing.

Attachment 259888 [details] pushed as 5a38700 - key-file: Add missing licencing header
Attachment 259889 [details] pushed as 75943c1 - build: Factor common test build rules into test.mk
Attachment 259890 [details] pushed as 43d92e7 - build: Rename AM_VALAFLAGS to TARGET_VALAFLAGS in configure.ac
Attachment 259891 [details] pushed as 7fa70f1 - build: Move backend.mk inclusions to the top of the files
Attachment 259893 [details] pushed as 40d2744 - tests: Add a function for asserting personas are in an aggregator

Comment on attachment 259887 [details] [review]
eds: Remove unused variable

Attachment 259887 [details] pushed as 0bdce57 - eds: Remove unused variable

Review of attachment 259895 [details] [review]:

This generally looks good but we can't merge until we get the patches on bgo#712148 merged and new releases of GLib, Gobject Introspection (with updated .gir and .typelib files), and Vala (with updated VAPI files) which we can depend upon.

+        PKG_CHECK_MODULES([GLIB_2_39_2], [glib-2.0 >= 2.39.2],

+        AM_PROG_VALAC([0.22.0.45-383d-dirty],

Be sure to bump these to the first release which includes our required changes before merging.


+      /* FIXME: Have to get the display-name this way because
+       * Folks.PersonaStore.display_name is not declared as abstract. */

Is this a fixable API issue? If so, please file a bug denoted for our next API break.


+  /* Test that vCards with different numbers of values for their N (structured
+   * name) attribute are parsed correctly.. */

Extra .

Review of attachment 259896 [details] [review]:

Looks good.

Review of attachment 259897 [details] [review]:

Looks good

Review of attachment 259899 [details] [review]:

Looks good

Review of attachment 259900 [details] [review]:

Looks good

Review of attachment 259901 [details] [review]:

Looks good

Review of attachment 259902 [details] [review]:

Looks good

Review of attachment 259903 [details] [review]:

Nice work!

Review of attachment 259904 [details] [review]:

Looks good

Review of attachment 259905 [details] [review]:

Looks good

Review of attachment 259904 [details] [review]:

Looks good

Review of attachment 259906 [details] [review]:

Looks good

Review of attachment 259907 [details] [review]:

Glad to see this speedup!

Review of attachment 259908 [details] [review]:

Not this bug, but sure.

Philip, I think you have a few more patches on your branch that don't appear on this bug. (I haven't reviewed them).

For the record, I haven't had time to test the patches I just reviewed. They break the core tests so I didn't check the pass/failure status of the BlueZ tests.

(In reply to comment #35)
> Review of attachment 259895 [details] [review]:
> 
> This generally looks good but we can't merge until we get the patches on
> bgo#712148 merged and new releases of GLib, Gobject Introspection (with updated
> .gir and .typelib files), and Vala (with updated VAPI files) which we can
> depend upon.
> 
> +        PKG_CHECK_MODULES([GLIB_2_39_2], [glib-2.0 >= 2.39.2],
> 
> +        AM_PROG_VALAC([0.22.0.45-383d-dirty],
> 
> Be sure to bump these to the first release which includes our required changes
> before merging.

So, due to the rejection of the GTestDBus patches upstream (see the discussion in bug #712148, around comment #40), I decided the most efficient use of time to stop this patch set bit rotting was to copy GTestDBus into folks, which I have done. That means we don’t need to bump our dependencies or wait for bindings updates, which is nice. On the other hand, we now have some more C code in the tests/ directory, with all the accompanying build system faff. The next distcheck is going to be a bundle of laughs.

> +      /* FIXME: Have to get the display-name this way because
> +       * Folks.PersonaStore.display_name is not declared as abstract. */
> 
> Is this a fixable API issue? If so, please file a bug denoted for our next API
> break.

Filed bug #724502.

> +  /* Test that vCards with different numbers of values for their N (structured
> +   * name) attribute are parsed correctly.. */
> 
> Extra .

Whoops. Fixed.


Attachment 259892 [details] pushed as 21b7563 - tests: Add support for an isolated system bus
Attachment 259894 [details] pushed as 02bf00d - tests: Add python-dbusmock support to the base TestCase class
Attachment 259895 [details] pushed as 6e85b2b - bluez: Add a test suite
Attachment 259896 [details] pushed as 3dbd946 - bluez: Remove BluezBackendFactory class
Attachment 259897 [details] pushed as 9064888 - core: Unprepare all backends when destroying the BackendStore
Attachment 259899 [details] pushed as d1fe72a - bluez: Add some more debug output
Attachment 259900 [details] pushed as 8a5f289 - bluez: Handle the optional Filename property of BlueZ
Attachment 259902 [details] pushed as eb8b1ad - bluez: Fix removal of a Map element while iterating over the Map
Attachment 259903 [details] pushed as d5a4faa - bluez: Reimplement vCard parsing when updating Personas
Attachment 259904 [details] pushed as 34bb166 - bluez: Calculate Persona IID checksums without the vCard PHOTO attribute
Attachment 259906 [details] pushed as 0f398ab - bluez: Correctly handle OBEX transfers which take zero time
Attachment 259907 [details] pushed as f36c4b7 - bluez: Allow persona store update poll frequency to be modified
Attachment 259908 [details] pushed as ffea273 - eds: Remove redundant variable assignment