Review of attachment 226149 [details] [review]:

::: backends/bluez/Makefile.am
@@ +24,3 @@
+	--pkg gobject-2.0 \
+	--pkg libebook-1.2 \
+	--pkg libedataserver-1.2 \

As with ofono, libedataserver shouldn’t be needed here (or should have a FIXME).
Same for the CFLAGS and LIBS below.

::: backends/bluez/bluez-backend-factory.vala
@@ +36,3 @@
+ *
+ * @param backend_store the {@link BackendStore} the factory belongs to.
+ */

All these documentation comments are missing “@since UNRELEASED”.

::: backends/bluez/bluez-backend.vala
@@ +33,3 @@
+/**
+ * A backend which loads {@link Persona}s from paired Bluetooth
+ * devices using the Phonebook Access Protocol (PBAP) and presents them

s/Phonebook/Phone Book/

@@ +89,3 @@
+    }
+
+  private async void add_device (string dev)

All private methods’ names should be prefixed by an underscore.

@@ +98,3 @@
+          var properties = device.GetProperties ();
+
+          int obex = (int) properties["Class"].get_uint32 () & 0x100000;

This definitely needs some comments explaining it.

@@ +112,3 @@
+              PersonaStore store =
+                  new BlueZ.PersonaStore (properties["Address"].get_string (),
+                                          properties["Alias"].get_string ());

What if ‘properties’ doesn’t contain ‘Address’ or ‘Alias’ (or ‘Class’, above) keys?

@@ +141,3 @@
+        catch (GLib.DBusError e)
+          {
+          }

This error handling (all 4 catch blocks) looks somewhat dubious. Perhaps the errors should be thrown from _add_device(), and handled better in prepare()?

@@ +170,3 @@
+          foreach (var dev in adapter.ListDevices ())
+            {
+              yield add_device (dev);

s/add_device/this._add_device/

@@ +206,3 @@
+            {
+              this.persona_store_removed (persona_store);
+            }

This is going to result in lots of spurious property change notifications for persona-stores.

::: backends/bluez/bluez-persona-store.vala
@@ +44,3 @@
+  private bool _is_quiescent = false;
+
+  private static string[] _always_writeable_properties = { };

s/{ }/{}/

@@ +47,3 @@
+
+  private org.openobex.Client? _obex_client = null;
+  private dynamic ObjectPath _obex_session_path;

This doesn’t seem to be used anywhere except in _prepare(). Would it be better off as a local variable there? (I don’t know.)

@@ +123,3 @@
+   *
+   * Create a new persona store to expose the {@link Persona}s provided by the
+   * device with the given Bluetooth address.

Missing “@since UNRELEASED”, “@param addr …” and “@param alias …”.

@@ +125,3 @@
+   * device with the given Bluetooth address.
+   */
+  public PersonaStore (string addr, string alias) throws IOError, DBusError

Why does this throw IOError or DBusError?

@@ +135,3 @@
+    }
+
+  static string[] split_all_vcards (string all_vcards)

This needs to be private and its name should be prefixed with an underscore.

@@ +138,3 @@
+    {
+      string[] lines = all_vcards.split ("\n");
+      string[] vcards = { };

s/{ }/{}/

@@ +185,3 @@
+          this._obex_pbap = Bus.get_proxy_sync (BusType.SESSION,
+                                                "org.openobex.client",
+                                                this._obex_session_path);

This should be an async call.

@@ +188,3 @@
+
+          yield this._obex_pbap.Select ("int", "PB");
+          yield this._obex_pbap.SetFormat ("vcard30");

This should all be documented, really. More comments!

@@ +191,3 @@
+
+          org.openobex.PhonebookAccess.PhonebookEntry[] entries =
+              yield this._obex_pbap.List ();

Are there any signals we could connect to in order to be notified of changes to the list of entries?

@@ +194,3 @@
+
+          string all_vcards = yield this._obex_pbap.PullAll ();
+          string[] vcards = split_all_vcards (all_vcards);

s/split_all_vcards/this._split_all_vcards/

@@ +230,3 @@
+      catch (GLib.IOError e)
+        {
+        }

This error handling also looks dubious. There should always be _something_ inside a catch{} block — even if it’s just a comment explaining why error handling isn’t necessary.

@@ +238,3 @@
+          /* We've finished loading all the personas we know about */
+          this._is_quiescent = true;
+          this.notify_property ("is-quiescent");

This will set is-prepared and is-quiescent to true even if the above code throws an exception. That doesn’t seem right to me. Instead, the persona store should probably remove itself on error.

@@ +252,3 @@
+   *
+   * @throws Folks.PersonaStoreError.READ_ONLY every time since the
+   * BlueZ backend is read-only.

Missing “@since UNRELEASED” and “@param persona …”.

@@ +267,3 @@
+   *
+   * @throws Folks.PersonaStoreError.READ_ONLY every time since the
+   * BlueZ backend is read-only.

Missing “@since UNRELEASED” and “@param details …”.

::: backends/bluez/bluez-persona.vala
@@ +31,3 @@
+ * A persona subclass which represents a single persona from a simple key file.
+ *
+ * @since 0.1.13

s/0.1.13/UNRELEASED/

@@ +38,3 @@
+    PhoneDetails
+{
+  private StructuredName _structured_name = null;

The variable should be marked as nullable.

@@ +46,3 @@
+  private Set<EmailFieldDetails> _email_addresses_ro;
+
+  private string _vcard = "";

As with the ofono backend, there doesn’t seem to be any need to keep a copy of the vCard around.

@@ +126,3 @@
+   *
+   * Create a new persona for the {@link PersonaStore} ``store``, representing
+   * the Persona given by the given vCard string.

Missing “@since UNRELEASED”, “@param name …”, “@param vcard …” and “@param store …”.

::: backends/bluez/org-bluez.vala
@@ +25,3 @@
+  namespace bluez {
+    [DBus (name = "org.bluez.Manager")]
+    public interface Manager : Object {

folks’ coding style requires that braces be on a new line, indented 2 spaces deeper than the preceding line.

::: configure.ac
@@ +378,3 @@
+  BLUEZ_BACKEND_UNINST_PATH='$(BACKEND_BLUEZ)'
+  BACKEND_UNINST_PATH="$BACKEND_UNINST_PATH:$BLEUZ_BACKEND_UNINST_PATH"
+])

Missing pkg-config and VAPI checks for libebook if the BlueZ backend is enabled.

Created attachment 256123 [details] [review]
Add a BlueZ backend v2

Here is a new version of the patchset which attempts to add a bluez backend to libfolks. The main differences with the previous patchset sent by Jeremy Whiting is the port to the BlueZ 5 API and that the backend handle device additions/removals and connections/disconnections. I also tried to take into account all the remarks made in the previous review.

The current behaviour is the following:
  - when prepare() is called, the backend look at all the already paired phones and tries to connect to them. It also add a signal handler on those devices and watch for connection/disconnection event. It also add a signal handler on the device manager to watch for new phone additions or removals.
  - if a new phone is detected (not necessarily already paired), a signal handler is added to watch if the phone gets connected at some point (or disconnected).
  - if a phone is seen as connected, the backend tries to connect to it and fetch its contacts even if it has been already loaded (thus loading potentially new contacts): the contact merging is automatically done in the persona store.
  - if a phone is seen as disconnected, the persona store associated with it is kept as is.
  - if a phone is removed (unpaired) the signal handler watching the connected property is removed. The persona store associated with the device is not removed. Should it be ?

Created attachment 256177 [details] [review]
Add a BlueZ backend v2

Patchset updated:
  - factorize pbap pse detection,
  - minor cosmetics (coding style).

Created attachment 256193 [details] [review]
Add a BlueZ backend v2

Patchset updated:
  - simplify vcards reading code and do not store the whole vcards buffer in memory before processing it,
  - makes obex session methods async, so that obex timeout won't freeze the whole application,
  - make vcard reading async.

Review of attachment 256193 [details] [review]:

Looking OK. I feel it needs a lot more comments to explain the BlueZ magic.

::: backends/Makefile.am
@@ +25,3 @@
+if ENABLE_BLUEZ
+SUBDIRS += bluez
+endif

It also needs to be added (unconditionally) to DIST_SUBDIRS.

::: backends/bluez/Makefile.am
@@ +67,3 @@
+	$(bluez_la_SOURCES:.vala=.c) \
+	bluez_la_vala.stamp \
+	$(NULL)

GITIGNOREFILES is unnecessary since this is handled by git.mk now.

::: backends/bluez/bluez-backend-factory.vala
@@ +2,3 @@
+ * Copyright (C) 2009 Zeeshan Ali (Khattak) <zeeshanak@gnome.org>.
+ * Copyright (C) 2009 Nokia Corporation.
+ * Copyright (C) 2012 Collabora Ltd.

Copyright headers need updating.

@@ +33,3 @@
+/**
+ * The backend module entry point.
+ */

All the documentation comments in this file need ‘@since UNRELEASED’ lines.

::: backends/bluez/bluez-backend.vala
@@ +1,2 @@
+/*
+ * Copyright (C) 2012 Collabora Ltd.

Copyright header needs updating.

@@ +39,3 @@
+ * using one {@link PersonaStore} per device.
+ *
+ * @since 0.7.5

This should be ‘@since UNRELEASED’ (we replace all the ‘UNRELEASED’s before release). All the other comments in this file should be updated similarly.

@@ +136,3 @@
+              var store = this._persona_stores.get (address);
+              if (connected.get_boolean())
+                {

Instead of having three levels of nesting, it would be tidier to do:

if (connected == null || !connected.is_of_type (VariantType.BOOLEAN) || connected.get_boolean() == false)
  {
    return;
  }

There’s probably no need to put the warning in there.

@@ +137,3 @@
+              if (connected.get_boolean())
+                {
+                  GLib.message ("Device '%s (%s)' connected", alias, address);

This should be a GLib.debug().

@@ +157,3 @@
+                                        alias,
+                                        address,
+                                        de.message);

These should be GLib.warning()s.

@@ +162,3 @@
+                  else
+                    {
+                      yield this._add_persona_store(address, alias);

This doesn’t seem like the right place to add a persona store. See my comment in this._device_added_cb() below.

@@ +167,3 @@
+              else
+                {
+                  GLib.message ("Device '%s (%s)' disconnected", alias, address);

This should be a GLib.debug().

@@ +179,3 @@
+    }
+
+  private async void _add_persona_store(string address, string alias)

Need a space before the ‘(’. Same for the other functions in this file.

@@ +212,3 @@
+                        alias,
+                        address,
+                        ie.message);

This should be a GLib.warning(). Actually, it may be tidier to just throw the error from this function and handle it in the caller. Up to you.

@@ +219,3 @@
+                        alias,
+                        address,
+                        de.message);

And this.

@@ +265,3 @@
+      });
+
+      this._property_notifiers[path] = property_notifier;

Looks like this._add_persona_store() is never called here? Relying on it being called as part of this._device_properties_changed_cb() seems flaky.

@@ +275,3 @@
+        {
+          var property_notifier = this._property_notifiers.get (path);
+          var handler = this._property_notifiers_handlers.get (path);

To save a lookups in each of the HashMaps, you could do:

org.freedesktop.PropertyNotifier? property_notifier = null;
ulong? handler = null;
if (this._property_notifiers.unset (path, out property_notifier) == true &&
    this._property_notifiers_handlers.unset (path, out handler) == true)
  {
    property_notifier.disconnect (handler);
  }

@@ +280,3 @@
+        }
+
+      /* XXX: should we keep the persona store or remove it ? */

This needs resolving. What are the arguments for/against the two approaches?

@@ +300,3 @@
+
+          this._manager = yield Bus.get_proxy (BusType.SYSTEM,
+                                                 "org.bluez", "/");

Indentation problem here.

@@ +302,3 @@
+                                                 "org.bluez", "/");
+
+          var dict = this._manager.GetManagedObjects();

Need a space before the ‘(’ (and in several places below).

@@ +305,3 @@
+
+          this._manager.InterfacesAdded.connect((path, interfaces) => {
+            this._device_added_cb.begin(path, interfaces);

I think it’s better to explicitly include the .end() call for the async method, to make it really obvious that it may complete asynchronously. e.g.:

this._device_added_cb.begin (path, interfaces, (o, r) =>
  {
    this._device_added_cb.end (r);
  });

This also makes it easy to add in error handling for the .end() call.

@@ +310,3 @@
+          this._manager.InterfacesRemoved.connect((path, interfaces) => {
+            this._device_removed_cb.begin(path, interfaces);
+          });

These two signal handlers don’t seem to be disconnected when unpreparing the store.

@@ +338,3 @@
+              var handler_id = property_notifier.PropertiesChanged.connect((path, changed, invalidated) => {
+                this._device_properties_changed_cb.begin(address, alias, path, changed, invalidated);
+              });

I think there might be a possible race condition here, between the persona store being added and property notification signal being connected: it’s possible for a property to change on line 334 and no notification to be received because the notifier hasn’t been connected at that point. Could you connect the notifier first, and get it to ignore changes for addresses which don’t exist in the list of persona stores at the moment?

@@ +343,3 @@
+              this._property_notifiers_handlers[path] = handler_id;
+
+            }

This code to add the initial persona stores should be refactored with the code in _device_added_cb() to eliminate code duplication.

::: backends/bluez/bluez-persona-store.vala
@@ +1,2 @@
+/*
+ * Copyright (C) 2010 Collabora Ltd.

Copyright header needs updating.

@@ +37,3 @@
+ * each contact on the device.
+ *
+ * @since 0.7.5

All the @since lines in this file need adding or updating.

@@ +54,3 @@
+  private HashTable<string, Variant> _phonebook_filter;
+  private org.bluez.obex.Transfer _transfer;
+  private org.freedesktop.PropertyNotifier _transfer_notifier;

_transfer and _transfer_notifier need to have nullable types (e.g. ‘org.bluez.obex.Transfer?’) since you assign null to them in the construct{} block.

@@ +156,3 @@
+   *
+   * Create a new persona store to expose the {@link Persona}s provided by the
+   * device with the given Bluetooth address.

This needs @param lines to document each of its parameters. Also needs @throws lines for its exceptions.

Would it be possible to call ‘alias’ something else (like ‘display_name’)? Folks already uses the term ‘alias’ in the Telepathy sense. Unless it’s a technical piece of Bluetooth terminology?

@@ +181,3 @@
+    }
+
+  public async void _update_contacts_from_filename(string filename)

Missing space before the ‘(’. Probably elsewhere in the file too.

@@ +191,3 @@
+          var dis = new DataInputStream (file.read ());
+          uint i = 0;
+          string line = null;

This needs to have a nullable type.

@@ +202,3 @@
+
+                  Persona persona = new Persona (entry.vcard, entry.name, vcard.str,
+                    this, i == 0);

This block of code could do with some comments to explain what the various vCard variables are, what’s in the entry, and why you’ve got (i == 0).

@@ +214,3 @@
+            {
+              this._emit_personas_changed (added_personas, null);
+            }

Is it possible that _update_contacts_from_filename() will be called with this._personas non-empty to begin with? If so, personas-changed could be emitted unnecessarily.

@@ +262,3 @@
+                            this.id,
+                            this._transfer.Name);
+              this._cancel_obex_transfer();

You could move this._cancel_obex_transfer() to a finally{} block.

@@ +312,3 @@
+        }
+
+        this._obex_pbap = null;

Indentation problems.

@@ +315,3 @@
+    }
+
+  private void _cancel_obex_transfer(bool cancel = false)

To me, having a boolean ‘cancel’ parameter to a method called _cancel_obex_transfer() doesn’t make sense. It seems like the (cancel == false) case needs factoring out and substituting into the call sites for this method.

@@ +334,3 @@
+      if (this._transfer_notifier != null)
+        {
+          this._transfer_notifier.disconnect(this._transfer_handler);

You should reset this._transfer_handler to 0 after disconnecting it.

@@ +364,3 @@
+      HashTable<string, Variant> props;
+
+      this._obex_pbap.PullAll ("", this._phonebook_filter, out path, out props);

What does this do? This file is completely lacking in comments.

::: backends/bluez/bluez-persona.vala
@@ +1,2 @@
+/*
+ * Copyright (C) 2010 Collabora Ltd.

Copyright header is out of date.

@@ +33,3 @@
+ * A persona subclass which represents a single persona from a simple key file.
+ *
+ * @since 0.1.13

All the @since lines in this file need adding/updating to say ‘UNRELEASED’.

@@ +43,3 @@
+    NoteDetails,
+    UrlDetails
+    */

This XXX needs resolving. The interfaces should be listed in alphabetic order.

@@ +45,3 @@
+    */
+{
+  private StructuredName _structured_name = null;

This needs to have a nullable type if you assign null to it.

@@ +132,3 @@
+   *
+   * Create a new persona for the {@link PersonaStore} ``store``, representing
+   * the Persona given by the group ``uid`` in the key file ``key_file``.

This needs an @param line for each of its parameters.

@@ +146,3 @@
+              is_user: is_user);
+
+      this._set_vcard(vcard);

Need a space before the ‘(’.

@@ +211,3 @@
+        {
+          /* XXX: we can tell which the preferred email address is, perhaps
+           * this can be annotated somehow. */

This XXX needs to be resolved.

::: backends/bluez/org-bluez-obex-client.vala
@@ +1,1 @@
+using GLib;

This file needs a copyright header.

::: backends/bluez/org-bluez.vala
@@ +1,1 @@
+using GLib;

This file needs a copyright header.

@@ +25,3 @@
+          {
+            [DBus (name ="PropertyChanged")]
+            public signal void PropertyChanged(string name, Variant val);

Missing space before the ‘(’.

::: backends/bluez/org-freedesktop.vala
@@ +1,1 @@
+using GLib;

This file needs a copyright header.

::: configure.ac
@@ +133,3 @@
+        AC_DEFINE(HAVE_BLUEZ, [1], [Define as 1 if you have the bluez backend])
+], [
+        AC_DEFINE(HAVE_BLUEZ, [0], [Define as 1 if you have the bluez backend])

s/bluez/BlueZ/

@@ +280,3 @@
+AS_IF([test x$enable_bluez_backend = xyes], [
+        PKG_CHECK_MODULES([EBOOK], [libebook-1.2 >= $EBOOK_REQUIRED])
+])

You also need VALA_CHECK_PACKAGES([libebook-1.2]). See how the ofono backend does it.

Created attachment 256664 [details] [review]
Add a BlueZ backend v2

New patch attached with most remarks taken into account. Thanks for the review.

There is still some point that needs to be worked out I guess.
What behaviour should have the backend when a device is removed (unpaired).
Should it remove the persona store associated with the device (and thus removing the associated contacts) or keep it ? IMHO i would say it should remove it since the user decided to unpair the phone with the system.

In bluez-backend.vala, i don't think a race condition can occur when installing the signal handler after the creating of the persona store for a given device.
If the creation suceeds the persona will automatically call prepare at some point and try to fetch the contacts from the phone. Even if we miss a connection or disconnection event, the device will still try to connect to it and load the contacts from the phone.

Created attachment 256715 [details] [review]
Add a BlueZ backend v2

New patch attached with some typo fixed in documentation.

Created attachment 256717 [details] [review]
Add a BlueZ backend v2

Patch updated with some cosmetic fix.

Review of attachment 256664 [details] [review]:

This patch looks a lot better, thanks!

I think the persona store should be removed when the device is removed.

I haven’t had a chance to re-examine the race condition though. I’ll do that later.

::: backends/bluez/bluez-backend-factory.vala
@@ +34,3 @@
+ * The backend module entry point.
+ *
+ * @since UNRELEASED

This needs a @param line for its parameter (as do the other functions and methods in this file).

::: backends/bluez/bluez-backend.vala
@@ +50,3 @@
+  private HashMap<string, org.freedesktop.PropertyNotifier> _property_notifiers;
+  private HashMap<string, ulong> _property_notifiers_handlers;
+  private Manager _manager;

This should be nullable, since it’s null when the backend is unprepared.

@@ +120,3 @@
+  public override void set_persona_stores (Set<string>? storeids)
+    {
+    }

These three methods ([set|enable|disable]_persona_store()) should contain comments explaining that it’s not possible to add/remove BlueZ persona stores programmatically, as it depends on the user pairing/unpairing devices.

@@ +173,3 @@
+                {
+                  yield store.new_obex_session();
+                  yield store.update_contacts();

Need a space before the ‘(’, and in various other places in the file.

@@ +199,3 @@
+          GLib.debug ("Device '%s (%s)' disconnected", alias, address);
+          if (store != null)
+            yield store.remove_obex_session();

Should the persona store be removed completely here? What are the different states a Bluetooth device can be in?

@@ +271,3 @@
+    {
+      org.freedesktop.PropertyNotifier property_notifier =
+        yield Bus.get_proxy (BusType.SYSTEM, "org.bluez", path);

Vala warns about an unhandled IOError here.

@@ +354,3 @@
+          var handler = this._property_notifiers_handlers.get (path);
+          property_notifier.disconnect (handler);
+          this._property_notifiers.unset (path);

I think you also need to unset the path from this._property_notifiers_handlers.

@@ +357,3 @@
+        }
+
+      /* XXX: should we keep the persona store or remove it ? */

As you say, I think it makes more sense to remove the persona store when the device is explicitly unpaired.

@@ +453,3 @@
+              this._manager.disconnect (this._interface_added_handler);
+              this._manager.disconnect (this._interface_removed_handler);
+            }

You should probably also set this._manager to null.

@@ +458,3 @@
+            {
+              this.persona_store_removed (persona_store);
+            }

You also need to unwatch the device properties here, or this._property_notifiers[_handlers] will remain full of entries.

@@ +480,3 @@
+      this.persona_store_removed (store);
+      this._persona_stores.unset (store.id);
+      this.notify_property ("persona-stores");

You also need to unwatch the device properties here.

::: backends/bluez/bluez-persona-store.vala
@@ +59,3 @@
+  /**
+   * {@inheritDoc}
+   */

Needs a @since line.

@@ +242,3 @@
+                }
+            }
+          yield file.delete_async();

I think deleting the transferred file should be done in this._transfer_properties_changed_cb(). It makes more sense there, since that’s where the transaction is handled.

@@ +289,3 @@
+    {
+      var property = changed.get ("Status");
+      if (property != null)

You could remove one level of indenting by changing this to:
    if (property == null)
        return;

@@ +309,3 @@
+                            this._transfer.Name);
+              this._reset_obex_transfer ();
+            }

Might be a good idea to add:
    else
      {
        GLib.debug ("Unknown OBEX transfer status ‘%s’", status);
      }
just to aid debugging in future if problems are encountered in this code.

@@ +324,3 @@
+   * @since UNRELEASED
+   */
+  public async void new_obex_session () throws DBusError, IOError

I think this should be internal, rather than public.

@@ +327,3 @@
+    {
+      if (this._obex_session_path == null)
+        {

You could remove one level of indentation by using:
    if (this._obex_session_path != null)
        return;

@@ +330,3 @@
+          this._obex_client = yield Bus.get_proxy (BusType.SESSION,
+                                                   "org.bluez.obex",
+                                                   "/org/bluez/obex");

this._obex_client was already set in the class’ constructor. Does it need to be overwritten here?

@@ +405,3 @@
+   * @since UNRELEASED
+   */
+  private void _cancel_obex_transfer ()

Vala warns that this method is never used.

@@ +420,3 @@
+                            de.message);
+            }
+          this._transfer = null;

This line is unnecessary since it’s already done in this._reset_obex_transfer().

@@ +428,3 @@
+   * Watch an obex transfer identified by its D-Bus path
+   *
+   * @since UNRELEASED

Missing an @param line and some @throws lines.

@@ +454,3 @@
+   * transfer and cancel the current one.
+   *
+   * @since UNRELEASED

Missing @throws lines.

::: backends/bluez/bluez-persona.vala
@@ +43,3 @@
+    NoteDetails,
+    UrlDetails
+    */

This XXX still needs addressing. It would be fine to not support all the *Details interfaces in the first version. More support can be added later.

@@ +166,3 @@
+              is_user: is_user);
+
+      this._set_vcard(vcard);

Missing space before the ‘(’ (and elsewhere in this file).

@@ +231,3 @@
+        {
+          /* XXX: we can tell which the preferred email address is, perhaps
+           * this can be annotated somehow. */

folks has no concept of a preferred e-mail address.

Hmm, looks like my review is for a slightly out-of-date version of the patch. Most of it should still be valid though, if you’ve only made cosmetic changes.

Created attachment 256748 [details] [review]
Add a BlueZ backend v2

New patch attached with previous remarks taken into account:
  - personna store is removed when the device is unpaired,
  - if the backend fails to watch the device properties, the store associated with this device is not removed (might fix a race condition).
  - store are now properly removed (with their signal handler) in unprepare,
  - fixes and cosmetics ...

Review of attachment 256748 [details] [review]:

Cosmetically this is a lot better, but it looks like some of the functional remarks from comment #9 got forgotten. :-(  I guess that’s due to my review racing with your previous iteration of the patch, sorry.

As I mentioned before, it would be great if you could write some test cases for this, time permitting. Either follow the example of the libsocialweb tests in folks (which implements a mock LSW service in Vala; see folks.git/tests/lib/libsocialweb/backend.vala); or use python-dbusmock (https://launchpad.net/python-dbusmock) to mock up BlueZ separately. I don’t know which approach will be easier, since BlueZ runs on the system bus, which none of the other tests in libfolks use, so we have no infrastructure for mocking it at the moment.

::: backends/bluez/bluez-backend.vala
@@ +411,3 @@
+
+          this._manager = yield Bus.get_proxy (BusType.SYSTEM,
+                                               "org.bluez", "/");

Before starting to use this._manager, I think you should call DBusObject.get_interface("org.freedesktop.DBus.ObjectManager") on it to check whether it implements the ObjectManager interface. BlueZ 5 does, but BlueZ 4 doesn’t: if the user is running BlueZ 4 the code will currently print the following error:
    folks-WARNING **: backend-store.vala:409: Error preparing Backend 'bluez': GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod: Method "GetManagedObjects" with signature "" on interface "org.freedesktop.DBus.ObjectManager" doesn't exist

It would be nice if a more helpful message was printed instead (e.g. “BlueZ backend requires BlueZ 5, but only BlueZ 4 is available. Backend will remain inactive.”).

@@ +460,3 @@
+                                ie.message);
+                  this._persona_store_removed_cb (this._persona_stores.get (address));
+                }

All this code is still largely the same as this._device_added_cb(). Please refactor it so that this._device_added_cb() is called here, to reduce code duplication.

@@ +504,3 @@
+          foreach (var persona_store in this._persona_stores.values)
+            {
+              this._persona_store_removed_cb (persona_store);

Calling this._persona_store_removed_cb() here will emit a notify::persona-stores signal for each of the stores, which isn’t ideal. Take the first two lines of this._persona_store_removed_cb() and copy them here, then rely on the code below to clear this._persona_stores, then emit a single notify::persona-stores signal.

@@ +540,3 @@
+      this.persona_store_removed (store);
+      this._persona_stores.unset (store.id);
+      this.notify_property ("persona-stores");

Still need to unwatch the device properties here.

::: backends/bluez/bluez-persona-store.vala
@@ +244,3 @@
+                }
+            }
+          yield file.delete_async ();

I still think this should be done in this._transfer_properties_changed_cb().

@@ +312,3 @@
+                        this._transfer.Name);
+          this._reset_obex_transfer ();
+        }

I still think it would be a good idea to add an ‘else’ block here with a warning about an unknown OBEX status.

@@ +326,3 @@
+   * @since UNRELEASED
+   */
+  public async void new_obex_session () throws DBusError, IOError

I still think this should be internal, rather than public.

@@ +333,3 @@
+      this._obex_client = yield Bus.get_proxy (BusType.SESSION,
+                                               "org.bluez.obex",
+                                               "/org/bluez/obex");

this._obex_client was already set in the class’ constructor. Does it need to be
overwritten here?

@@ +389,3 @@
+  private void _reset_obex_transfer ()
+    {
+      this._transfer = null;

Would it make sense to delete the file (this._transfer.Filename) before resetting the transfer? Just in case the transfer completed but we didn’t then explicitly delete the file?

@@ +407,3 @@
+   * @since UNRELEASED
+   */
+  private void _cancel_obex_transfer ()

This method is still never used.

@@ +422,3 @@
+                            de.message);
+            }
+          this._transfer = null;

This line is unnecessary since it’s already done in
this._reset_obex_transfer().

::: backends/bluez/bluez-persona.vala
@@ +43,3 @@
+    NoteDetails,
+    UrlDetails
+    */

This XXX still needs addressing.

Created attachment 258035 [details] [review]
bluez: Add a Bluetooth Phonebook Access Profile backend using BlueZ 5

This pulls contacts out of a paired Bluetooth device and dumps them in
folks.

No test cases are included.

Some notes:
 • Test cases still need to be added. I suggest using python-dbusmock (https://code.launchpad.net/~pitti/python-dbusmock/trunk).
 • The patch needs more thorough testing, especially regarding error conditions and what happens when the device connects/disconnects and is paired/unpaired. What happens when bluetoothd or obexd disappear?
 • It also needs testing with a variety of phones.
 • The D-Bus traffic needs to be sanity-checked to ensure we’re not spamming the bus.
 • The following two fixes are required in Vala and GIO:
   - https://bugzilla.gnome.org/show_bug.cgi?id=710643
   - https://bugzilla.gnome.org/show_bug.cgi?id=710726

Review of attachment 258035 [details] [review]:

As per comment #14, folks’ dependencies on Vala and GIO need to be bumped to grab those two fixes.

Review of attachment 258035 [details] [review]:

::: backends/bluez/bluez-backend.vala
@@ +458,3 @@
+          this._properties_changed_handler =
+              this._manager.interface_proxy_properties_changed.connect (
+                  this._device_properties_changed_cb);

interface_proxy_properties_changed signal does not seem to catch the devices properties changes.
Here is an example of DBus message when the device is seen as connected:

signal sender=:1.4 -> dest=(null destination) serial=23788 path=/org/bluez/hci0/dev_10_68_3F_29_98_94; interface=org.freedesktop.DBus.Properties; member=PropertiesChanged
   string "org.bluez.Device1"
   array [
      dict entry(
         string "Connected"
         variant             boolean true
      )
   ]
   array [
   ]

::: backends/bluez/bluez-persona-store.vala
@@ +591,3 @@
+          /* If the device is connected at the moment, go ahead and block on
+           * downloading its contact list. */
+          yield this.set_connection_state (this._device.connected);

If the device is not connected at this point and is not in the future the store will never fetch contacts from it.
The phone only reaches a connected status if the user or a service tries to connect to it, so I think we should try to fetch the contacts here at first.

Note, if the phone is not connected and we try to fetch the contacts, it will reach the connected state. So we have to take care that the signal handlers watching the phone state are installed only after the initial phonebook download.

(In reply to comment #16)
> ::: backends/bluez/bluez-backend.vala
> @@ +458,3 @@
> +          this._properties_changed_handler =
> +              this._manager.interface_proxy_properties_changed.connect (
> +                  this._device_properties_changed_cb);
> 
> interface_proxy_properties_changed signal does not seem to catch the devices
> properties changes.
> Here is an example of DBus message when the device is seen as connected:

That looks like bug #710726; you need to update to GLib master.

> ::: backends/bluez/bluez-persona-store.vala
> @@ +591,3 @@
> +          /* If the device is connected at the moment, go ahead and block on
> +           * downloading its contact list. */
> +          yield this.set_connection_state (this._device.connected);
> 
> If the device is not connected at this point and is not in the future the store
> will never fetch contacts from it.
> The phone only reaches a connected status if the user or a service tries to
> connect to it, so I think we should try to fetch the contacts here at first.

OK, I’ll rework the code to do this. Sorry for breaking it. :-(

Created attachment 258141 [details] [review]
bluez: Add a Bluetooth Phonebook Access Profile backend using BlueZ 5

This pulls contacts out of a paired Bluetooth device and dumps them in
folks.

No test cases are included.

Created attachment 258142 [details] [review]
bluez: Add a Bluetooth Phonebook Access Profile backend using BlueZ 5

This pulls contacts out of a paired Bluetooth device and dumps them in
folks.

No test cases are included.

This bumps the following dependencies:
 • Vala: 0.22.0.28-9090
 • GLib: 2.39.0
which are required for the following fixes:
 • https://bugzilla.gnome.org/show_bug.cgi?id=710643
 • https://bugzilla.gnome.org/show_bug.cgi?id=710726

This includes a number of different changes, and I now consider the patch finished. Major things which are missing (but which can be added later) are:
 • Test cases.
 • Caching.
 • A comprehensive study of performance (specifically, whether it would be better to download all the contacts *without* their avatars in one sweep, then download the avatars in a second sweep).
 • It also needs testing with a variety of phones.
 • The D-Bus traffic needs to be sanity-checked to ensure we’re not spamming
the bus.

Looks good to me.

Comment on attachment 258142 [details] [review]
bluez: Add a Bluetooth Phonebook Access Profile backend using BlueZ 5

Committed with a few minor tweaks (such as a NEWS entry and changes to quiescence behavior on errors from prepare()).

Thanks everyone for working on and reviewing this.

commit 659a5c4539912bb3b31f7e55e48e6fd4215257f8
Author: Matthieu Bouron <matthieu.bouron@collabora.com>
Date:   Tue Aug 21 16:49:44 2012 +0530

    bluez: Add a Bluetooth Phonebook Access Profile backend using BlueZ 5
    
    This pulls contacts out of a paired Bluetooth device and dumps them in
    folks.
    
    No test cases are included.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=685848
    
    This bumps the Vala and GLib dependencies of folks, needed for the fol
    two fixes.
     • https://bugzilla.gnome.org/show_bug.cgi?id=710643
     • https://bugzilla.gnome.org/show_bug.cgi?id=710726
    
    https://bugzilla.gnome.org/show_bug.cgi?id=685848

 NEWS                                      |   5 +-
 backends/Makefile.am                      |   5 +
 backends/bluez/Makefile.am                |  41 ++
 backends/bluez/bluez-backend-factory.vala |  73 ++
 backends/bluez/bluez-backend.vala         | 636 ++++++++++++++++++
 backends/bluez/bluez-persona-store.vala   | 843 ++++++++++++++++++++++++
 backends/bluez/bluez-persona.vala         | 312 +++++++++
 backends/bluez/org-bluez-obex-client.vala |  99 +++
 backends/bluez/org-bluez.vala             | 116 ++++
 configure.ac                              |  28 +-
 folks/build-conf.vapi                     |   3 +
 po/POTFILES.in                            |   2 +
 po/POTFILES.skip                          |   2 +
 13 files changed, 2162 insertions(+), 3 deletions(-)