Review of attachment 225367 [details] [review]:

Looks like a good start. Most of these are really picky comments (sorry!).

::: backends/ofono/Makefile.am
@@ +22,3 @@
+	--pkg gio-2.0 \
+	--pkg gobject-2.0 \
+	--pkg dbus-glib-1 \

dbus-glib is massively deprecated. Can you get away without using it?

::: backends/ofono/ofono-backend-factory.vala
@@ +29,3 @@
+using Folks.Backends.Ofono;
+
+private BackendFactory _backend_factory = null;

Should be marked as nullable.

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

All these documentation comments need a “@since UNRELEASED” tag.

::: backends/ofono/ofono-backend.vala
@@ +115,3 @@
+      catch (DBusError de)
+        {
+        }

See the comments in PersonaStore about throwing exceptions from constructors. Accordingly, I think this error handling stuff should be removed.

@@ +137,3 @@
+          Manager manager = Bus.get_proxy_sync (BusType.SYSTEM,
+                                                "org.ofono",
+                                                "/");

This should be async.

@@ +188,3 @@
+
+          this._is_prepared = false;
+          this.notify_property ("is-prepared");

There should be a [freeze|thaw]_notify() pair around these notify signal emissions.

@@ +200,3 @@
+      if (properties.contains ("Features"))
+      {
+        string[] features = properties.get ("Features").get_strv ();

Instead of doing paired calls to properties.contains() then properties.get(), it would be (marginally) more efficient to just call properties.get() and check whether the returned value is non-null.

@@ +209,3 @@
+        {
+          alias = properties.get ("Manufacturer").get_string();
+        }

Some comments about what’s going on (and why; e.g. why is Name preferred over Manufacturer) here would be helpful.

@@ +217,3 @@
+            this._add_modem (path, alias);
+          }
+        }

Indentation isn’t quite right in this method.

::: backends/ofono/ofono-persona-store.vala
@@ +30,3 @@
+/**
+ * A persona store which is associated with a single Ofono device. It will 
+ * create a {@link Persona} for each contact on the sim card phonebook.

s/sim/SIM/

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

Missing “@since UNRELEASED” and “@param” lines for the parameters.

@@ +126,3 @@
+              display_name: alias);
+
+      this.trust_level = PersonaStoreTrust.FULL;

Should we always trust all of the SIM cards we see? What SIM cards are generally exposed by ofono? Just the user’s personal one? If so, FULL trust is probably appropriate.

@@ +136,3 @@
+                                                             "org.ofono",
+                                                             path);
+      sim_manager.PropertyChanged.connect (this._property_changed);

The D-Bus stuff should be async and should be moved to prepare(). It’s generally quite bad practice to do async stuff or stuff which can throw exceptions from a constructor.

@@ +143,3 @@
+      if (property == "Present" && value.get_boolean () == false)
+        {
+          this.removed ();

Other folks backends generally call “this._emit_personas_changed (null, this._personas)” before emitting the ‘removed’ signal. It’s not something that we guarantee in the API anywhere (IIRC), but it’s probably safest to behave the same as the other backends.

@@ +145,3 @@
+          this.removed ();
+        }
+    }

Missing empty line after this method.

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

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

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

s/{ }/{}/

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

This could do with a comment documenting the expected input and output formats (or linking to some relevant ofono documentation).

@@ +164,3 @@
+        }
+
+        return vcards;

This is indented 2 spaces too far.

@@ +184,3 @@
+          this._prepare_pending = true;
+
+          string all_vcards = this._ofono_phonebook.Import ();

Is it possible for the ofono phonebook to change at runtime? If so, we should update the set of personas then.
(I don’t know anything about how ofono works.)

@@ +186,3 @@
+          string all_vcards = this._ofono_phonebook.Import ();
+
+          string[] vcards = split_all_vcards (all_vcards);

s/split_all_vcards/this._split_all_vcards/

@@ +192,3 @@
+          for (int i = 0; i < vcards.length; i++)
+            {
+              string vcard = vcards[i];

It should be possible to use “foreach (var vcard in vcards)” rather than manually maintaining a loop index.

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

These should probably have a warning() in them if nothing else.

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

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

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

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

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

“@since UNRELEASED” :-)

@@ +35,3 @@
+    NameDetails,
+    PhoneDetails,
+    EmailDetails

IIRC we use the convention that interfaces should be listed in ascending alphabetic order.

@@ +37,3 @@
+    EmailDetails
+{
+  private StructuredName _structured_name = null;

Should be nullable.

@@ +52,3 @@
+      "email-addresses"
+    };
+  private static string[] _writeable_properties = { };

s/{ }/{}/

@@ +110,3 @@
+    }
+
+  static HashMap<string, string> parse_vcard (string vcard)

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

@@ +127,3 @@
+
+          details[kv[0]] = kv[1].strip ();
+        }

This could really do with some documentation about the expected input and output, and what the method’s meant to achieve.

@@ +145,3 @@
+   * Create a new persona.
+   *
+   * Create a new persona for the given vcard contents.

s/vcard/vCard/

Missing “@since UNRELEASED” and “@param vcard”, etc.

@@ +156,3 @@
+              uid: uid,
+              store: store,
+              is_user: false);

Does ofono not provide any information about which vCard is the user’s?

@@ +170,3 @@
+      this._email_addresses = new HashSet<EmailFieldDetails> ();
+      this._email_addresses_ro = this._email_addresses.read_only_view;
+

Extraneous empty line.

@@ +176,3 @@
+    {
+      this._vcard = vcard;
+      HashMap<string, string> details = parse_vcard (this._vcard);

s/parse_vcard/this._parse_vcard/

@@ +179,3 @@
+      
+      foreach (var key in details.keys)
+      {

This is a fairly inefficient way of iterating over the map. HashMap.keys is O(n) IIRC, and has to allocate a new n-element array to hold the keys. It also means you then have to make several further lookups in the map to get the values corresponding to each key.

It would be a lot more efficient to use details.map_iterator(). e.g.:

var iter = details.map_iterator ();
while (iter.next () == true)
  {
    var key = iter.get_key ();
    var val = iter.get_value ();

    …
  }

@@ +182,3 @@
+        if (details[key] == null || details[key] == "")
+        {
+          /* Yes, this can actually happen */

Why not change parse_vcard() to make sure it can’t happen then?

@@ +188,3 @@
+        if (key.has_prefix ("TEL"))
+        {
+          /* XXX: Use type of number as well */

Reminder about the XXX here.

@@ +202,3 @@
+        {
+          string[] components = details[key].split (";");
+          assert (components.length == 5);

It’s a bad idea to use an assert() inside a parser, since malformed input could cause folks to abort. Probably better to emit a warning() and then continue instead.

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

Reminder about the XXX here.

@@ +214,3 @@
+           * this can be annotated somehow. */
+          this._email_addresses.add (new EmailFieldDetails (details[key]));
+        }

Indentation’s messed up in this method.

@@ +216,3 @@
+        }
+      }
+    }

Missing an empty line after this one.

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

Missing a copyright/licence header, and the whole file doesn’t follow the coding guidelines.

Also, we need to make sure that this isn’t exposed as part of folks’ public API. In fact, it would be wise if you checked over the generated .vapi and .h files for the entire backend (if there are any), just to make sure nothing inappropriate has snuck its way into them.

Created attachment 225538 [details] [review]
Add an ofono backend

Ok, I updated the patch following your review.  There was one point I didn't understand though, the backend_factory is not nullable in any other backend.  Should it be made nullable in all the backends?

Review of attachment 225538 [details] [review]:

A few points seem to have been forgotten, so I’ve commented on them again.

::: backends/ofono/ofono-backend-factory.vala
@@ +29,3 @@
+using Folks.Backends.Ofono;
+
+private BackendFactory _backend_factory = null;

The backend_factory should be nullable in every backend (assuming the backend legitimately sets it to null at some point). It’s a case of the other backends being wrong and needing to be updated.

@@ +34,3 @@
+ * The backend module entry point.
+ *
+ * @since UNRELEASED

Sorry, I just realised the “@param” lines are missing in this file too (and probably in other backends, I guess).

::: backends/ofono/ofono-backend.vala
@@ +143,3 @@
+
+          this._is_quiescent = true;
+          this.notify_property ("is-quiescent");

Still missing a [freeze|thaw]_notify() pair, as is unprepare(), below.

@@ +215,3 @@
+                  /* This modem has a sim card, so add a persona store for it */
+                  this._add_modem (path, alias);
+                }

What if ‘features’ contains ‘sim’ more than once? The modem will end up being added multiple times. It’s probably safer (and marginally more efficient) to bail out of the loop after calling _add_modem().

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

s/{ }/{}/

@@ +122,3 @@
+   * modem with the given address.
+   *
+   * @param path the object path of this modem

s/object path/D-Bus object path/ would be a bit more explicit.

@@ +132,3 @@
+              display_name: alias);
+
+      this.trust_level = PersonaStoreTrust.FULL;

Still unanswered:

Should we always trust all of the SIM cards we see? What SIM cards are
generally exposed by ofono? Just the user’s personal one? If so, FULL trust is
probably appropriate.

@@ +204,3 @@
+          sim_manager.PropertyChanged.connect (this._property_changed);
+
+          string all_vcards = this._ofono_phonebook.Import ();

Still unanswered:

Is it possible for the ofono phonebook to change at runtime? If so, we should
update the set of personas then.
(I don’t know anything about how ofono works.)

@@ +224,3 @@
+      catch (GLib.DBusError e)
+        {
+          warning ("DBus Error has occured when fetching ofono phonebook, %s", e.message);

What happens if this code is hit? The persona store will be created in a zombie state. It would probably be better to emit the ‘removed’ signal if either of these ’catch’ clauses are hit. See what EDS does as an example.

Regardless: s/occured/occurred/ (and again below).

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

Missing a “@param persona”.

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

Missing a “@param details”.

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

s/0.1.13/UNRELEASED/!

@@ +126,3 @@
+              continue;
+
+          details[kv[0]] = kv[1].strip ();

This could still do with some documentation. :-)

@@ +145,3 @@
+   * Create a new persona.
+   *
+   * Create a new persona for the given vcard contents.

Still: s/vcard/vCard/.

@@ +147,3 @@
+   * Create a new persona for the given vcard contents.
+   *
+   * @since UNRELEASED

Missing “@param vcard” and “@param store”.

@@ +159,3 @@
+              store: store,
+              is_user: false);
+      this._set_vcard (vcard);

Still unanswered: Does ofono not provide any information about which vCard is the user’s?

@@ +186,3 @@
+          if (val == null || val == "")
+            {
+              /* Yes, this can actually happen */

Still unanswered: Why not change parse_vcard() to make sure it can’t happen then?

@@ +192,3 @@
+          if (key.has_prefix ("TEL"))
+            {
+              /* XXX: Use type of number as well */

Reminder about the XXX here.

@@ +215,3 @@
+                  components[2],
+                  components[3],
+                  components[4]);

Just a thought, but doesn’t vCard implement escaping and other encodings which need to be undone before we can use the data? Why aren’t we using a vCard parsing library?

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

Reminder about the XXX here.

Created attachment 226095 [details] [review]
Add an ofono backend

That was weird I didn't get any e-mail about your last review.  Anyway, this update fixes some of the mentioned problems and I'll answer the rest here.

Ofono does support removal and creation of modems, but doesn't have any signals for when the phonebook on the sim card is modified.  It does have signals for when a modem is added or removed which I've connected to, as well as when the sim "Present" property becomes false.  I should probably also watch for when it becomes true in the backend.

It doesn't have any indication of which if any contact is the user.

Review of attachment 226095 [details] [review]:

Looks a lot better, but I’m still concerned that we’re re-inventing a vCard parser.

Also, there are still some ‘XXX’ comments left. I don’t know what your plans are for those.

::: backends/ofono/ofono-persona.vala
@@ +228,3 @@
+                  components[2],
+                  components[3],
+                  components[4]);

I’m still concerned about the lack of unescaping here (and in the rest of the vCard code).

Ok, I asked in #evolution and we could use the EVCard parser in libebook in the ofono backend (and also in the upcoming bluez backend) with a dependency on libebook http://developer.gnome.org/libebook/3.6/EVCard.html  That's probably a good idea.  I'll play with using that here in the ofono backend and see how that goes.  It also has unescaping code for the values from the vCard, which should solve that problem.

By the way, I looked at the EmailAddressDetails and PhoneDetails and don't see a way to specify which phone is HOME, which is WORK, etc. besides the phone_fields in edsf-persona.  Shouldn't we have a way to specify that in the base Details classes?

Created attachment 226143 [details] [review]
Add an ofono backend

Ok, now the ofono backend is using libebook to parse the vcard data.  It seems to work here, but the sim card I have doesn't have any N data for either of my contacts.  I'll test similar code in the bluez backend which does provide test cases with N data in the vCards next.

Review of attachment 226143 [details] [review]:

Nice!

> By the way, I looked at the EmailAddressDetails and PhoneDetails and don't see
> a way to specify which phone is HOME, which is WORK, etc. besides the
> phone_fields in edsf-persona.  Shouldn't we have a way to specify that in the
> base Details classes?

AbstractFieldDetails.parameters.

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

What’s libedataserver needed for? Surely we only need libebook?

::: backends/ofono/ofono-backend.vala
@@ +210,3 @@
+          else if (manufacturer_variant != null)
+            {
+              alias = manufacturer_variant.get_string();

Just noticed there’s a space missing here before ‘()’.

::: backends/ofono/ofono-persona.vala
@@ +157,3 @@
+  private void _set_vcard (string vcard)
+    {
+      this._vcard = vcard;

Is it necessary to store the vCard as this._vcard? It doesn’t seem to be used anywhere else, so this is just a waste of memory.

@@ +158,3 @@
+    {
+      this._vcard = vcard;
+      E.VCard card = new E.VCard.from_string(vcard);

Missing space before ‘(vcard)’.

::: configure.ac
@@ +111,3 @@
+else
+	AC_DEFINE(HAVE_OFONO, [0], [Define as 1 if you have the ofono backend])
+fi

Shouldn’t this check for the libebook pkg-config file and Vala headers?

Created attachment 226185 [details] [review]
Add an ofono backend

--pkg libedataserver-1.2 is needed otherwise the vapi for libebook has errors about E.Source and E.SourceRegistry not being found.  I have removed libedataserver from the linking flags though, since we don't need to link to it.

I'm also wondering if there's a way we could share the persona code between this and the bluez persona.  Both just take a personastore and a vcard and do the same things, maybe a VCardPersona in libfolks itself that both backends could use.  That would make folks itself depend on libebook though.

Review of attachment 226185 [details] [review]:

Pretty much there. Please commit to master (*not* folks-0-8) with a suitable NEWS entry once these two problems are fixed!

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

The libedataserver problem should be fixed by fixing libebook-1.2.deps (which accompanies the .vapi file) to correctly list libedataserver as a dependency. That might make folks depend on an updated EDS version, though, so in the meantime we could keep this --pkg line if it’s accompanied by a suitable FIXME.

::: configure.ac
@@ +228,3 @@
+AS_IF([test x$enable_ofono_backend = xyes], [
+        PKG_CHECK_MODULES([EBOOK], [libebook-1.2 >= $EBOOK_REQUIRED])
+])

The following is also required, to check for the .vapi file:

        AS_IF([test x$enable_ofono_backend = xyes], [
          VALA_CHECK_PACKAGES([libebook-1.2])
        ])

there should be a section (around line 300) with other such checks already.

Ok, thanks for the review. I added implementation of the new api to the ofono backend and fixed those two issues and merged to master.