(In reply to comment #0)
> Now that Connection.Interface.ContactList has been undrafted, folks should use
> it instead of the old API when available.
> http://telepathy.freedesktop.org/spec/Connection_Interface_Contact_List.html
> 
> You probably want to wait for high level client API in tp-glib though :
> https://bugs.freedesktop.org/show_bug.cgi?id=26205

I'll support this once that bug is closed and the version of tp-glib is released.

(Setting milestone 'Future' since it's not clear when the dependent bug will be fixed)

Empathy 3.4 is going to require ContactList so Folks should feel free to do the same and start using it as well.

Actually, it should really use it for 3.4 to avoid fetching and preparing the contact list twice (once by empathy and once in folks).

Created attachment 211128 [details] [review]
Port TpfPersonaStore to high-level tp-glib APIs

It now uses Connection.ContactList iface instead of deprecated
ContactList channels.

Note that this introduce an important behaviour change: folks will
no longer pull all TpContact features, but rely on the user to define
features needed on the default AM's factory.

All unit tests finally pass \o/

git branch: http://cgit.collabora.com/git/user/xclaesse/folks.git/log/?h=contact-list

It works fine in empathy with small patch: http://cgit.collabora.com/git/user/xclaesse/empathy.git/log/?h=new-folks

Diff is hard to review since it's almost a rewrite of tpf-persona-store.vala, so I suggest reviewing the new file entirely.

Created attachment 211129 [details] [review]
Port TpfPersonaStore to high-level tp-glib APIs

It now uses Connection.ContactList iface instead of deprecated
ContactList channels.

Note that this introduce an important behaviour change: folks will
no longer pull all TpContact features, but rely on the user to define
features needed on the default AM's factory.

Review of attachment 211129 [details] [review]:

This is looking good. There are a few issues with async function calls which ‘yield’s have been omitted for; I expect there’ll be some error handling warnings to clean up once the ‘yield’s have been added. Braces are missing around a load of single-line if blocks (I think they’re always good to add so that when lines are added to the if blocks in future, the diffs don’t get messy due to having to add braces and re-indent the existing line). Other than that, there are some places where debug() would be helpful, and a few places which could be optimised.

I haven’t run the code yet; I’ll do that once these comments have been addressed. Also, it would be good if you could compile your code with valac’s --enable-experimental-non-null mode and quickly check if there are any actual issues with null dereferences. Most of the output of --enable-experimental-non-null is useless, but it’s caught a few genuine bugs in the past. There’s no need to make the backend completely --enable-experimental-non-null–safe though, since the rest of folks doesn’t compile with it yet.

Thanks for all your work on this!

::: backends/telepathy/lib/tpf-persona-store.vala
@@ +32,3 @@
  * A persona store which is associated with a single Telepathy account. It will
+ * create {@link Persona}s for each of the contacts in the account's
+ * contact list.

Might be useful to mention here the need to add features to the TpSimpleClientFactory.

@@ +52,3 @@
+  private HashMap<unowned Contact, Persona> _contact_persona_map;
+
+  private HashSet<string> _favourite_ids;

Should probably mention that these are TpContact IDs.

@@ +65,3 @@
   private bool _is_quiescent = false;
+  private bool _got_initial_members = false;
+  private bool _got_self_contact = false;

Would be useful to add a comment noting that this means “we’ve seen a self contact at least once, and that this._self_persona may be null even if this._got_self_contact is true” (e.g. if the self Persona subsequently disappears after the store has reached quiescence).

Or you could just rename it to _got_initial_self_contact. That would probably be better.

@@ +329,3 @@
       debug.unindent ();
 
+      debug.print_line (domain, level, "%u contact–Persona mappings:",

s/contact/TpContact/ would be clearer.

@@ +342,3 @@
       debug.unindent ();
 
+      debug.print_line (domain, level, "%u favourite ids:",

s/ids/TpContact IDs/

@@ +458,3 @@
                   this._logger.favourite_contacts_changed.connect (
                       this._favourite_contacts_changed_cb);
+                  this._initialise_favourite_contacts.begin ();

I think you could move the ’yield this._logger.prepare ()’ and associated code from above into _initialise_favourite_contacts(). This might speed up prepare() slightly, at the expense of change_is_favourite() throwing an error if called before the logger has been prepared.

@@ +538,3 @@
     {
+      /* This is not efficient, but probably better than doing DBus roundtrip
+       * to get a TpContact. Maybe we should add a id->persona map? */

You’re probably right. This is fine for now, and given that it’s only called when favourite contacts are changed (i.e. on startup and once in a blue moon), it might not be worth the memory to maintain an ID → Persona map.

@@ +543,3 @@
         {
+          if (iter.get_key().get_identifier() == id)
+            return iter.get_value();

There are some spaces missing before ‘()’s in the lines above. Also braces missing from around the body of the if statement.

@@ +553,3 @@
         {
+          this._favourite_ids.add (id);
+          Persona ?p = this._lookup_persona_by_id (id);

You can use ‘var’ here instead of ‘Persona?’. Same below.

@@ +555,3 @@
+          Persona ?p = this._lookup_persona_by_id (id);
+          if (p != null)
+            p.is_favourite = true;

This should call a (new) internal _set_is_favourite() method on the Tpf.Persona, instead of setting the user-visible property directly. This is an approach taken by the newer folks backends which nicely breaks the loop between Tpf.Persona.is_favourite and _favourite_contacts_changed_cb().

This change should probably be done as a separate commit.

@@ +645,3 @@
+        {
+          warning ("Connection does not implement ContactList iface,
+              legacy CM are not supported anymore. Stay offline");

This message should probably be translated and a bit more helpful. Perhaps suggest to upgrade Telepathy (though we can’t say anything for specific CMs).

@@ +649,1 @@
+          this._force_quiescent ();

The PersonaStore should probably remove itself here rather than reaching quiescence. See what Edsf.PersonaStore does when it emits its ‘removed’ signal.

@@ +663,3 @@
+        this._can_group_personas = MaybeBool.TRUE;
+      else
+        this._can_group_personas = MaybeBool.FALSE;

Missing braces.

@@ +677,3 @@
+        }
+      this.notify_property ("can-add-personas");
+      this.notify_property ("can-remove-personas");

Probably best to freeze/thaw notifications around these.

@@ +683,3 @@
+      this._self_contact_changed_cb (this._conn, null);
+
+      /* TpConnection still does not have high-level API for this */

Adding a FIXME and a bug reference might ensure we move to any new high-level API for it in future.

@@ +713,1 @@
+      this._conn.notify["contact-list-state"].connect (this._contact_list_state_changed_cb);

Need to disconnect from this in _reset().

@@ +844,3 @@
         {
+          this._personas.set (p.iid, p);
+          this._persona_set.add (p);

Instead of doing this._persona_set.contains() and this._persona_set.add() separately, it’s more efficient to do “if (this._persona_set.add (p) == true) { … }”. This optimisation works for most of libgee’s operations on Sets and Maps.

@@ +856,3 @@
         {
+          this._personas.unset (p.iid);
+          this._persona_set.remove (p);

Same optimisation as above can be made using the return value of this._persona_set.remove().

@@ +866,3 @@
     {
+      if (this._contact_persona_map == null)
+        return;

Braces (and below).

@@ +870,2 @@
+      var contact = obj as Contact;
+      var persona = this._contact_persona_map[contact];

This can be optimised by instead doing:
  Persona? persona = null;
  this._contact_persona_map.unset (contact, out persona);
  if (persona == null)
    …
and removing the unset() call further down.

@@ +881,3 @@
+          var personas = new HashSet<Persona> ();
+          personas.add (persona);
+          this._emit_personas_changed (null, personas);

What if persona is this._self_contact? There should either be code to set this._self_contact = null in _remove_persona() or here; I’m not sure where is best. Probably _remove_persona(). For the sake of symmetry, it might be good to put corresponding code in _add_persona() too.

@@ +887,3 @@
     }
 
+  internal Tpf.Persona _ensure_persona_for_contact (Contact contact)

This method could do with a debug() statement added at the beginning, just like the old _ensure_persona_from_contact() method had. Other key methods should probably have debug() statements added too (though I haven’t commented about it elsewhere). This makes debugging problems from the (scant) logs which users can provide a lot easier. As a general note, I’ve found it’s good to include both pointers and IDs in logs, so that instances of objects can be correlated.

@@ +899,1 @@
+      persona.is_favourite = this._favourite_ids.contains (contact.get_identifier ());

As above, this should call an internal persona._set_is_favourite() method, rather than setting the user-visible property directly.

@@ +901,1 @@
+      return persona;

Might be useful to add a comment somewhere in this method explaining that the persona is purposefully not announced using _emit_personas_changed(), and that this is a behaviour change from the old implementation.

@@ +937,3 @@
     {
+      if (this._conn.contact_list_state != ContactListState.SUCCESS)
+        return;

Does Telepathy guarantee that the contact-list-state never decreases?

@@ +941,1 @@
+      this._conn.contact_list_changed.connect (this._contact_list_changed_cb);

This needs to be disconnected in _reset().

@@ +948,3 @@
 
+  private void _contact_list_changed_cb (GLib.GenericArray<TelepathyGLib.Contact> added,
+      GLib.GenericArray<TelepathyGLib.Contact> removed)

This method could definitely do with some debug() added to it, since it’s the core of the persona store.

@@ +977,1 @@
+          if (persona == this._self_persona)

This probably deserves a comment explaining it.

@@ +997,2 @@
       else
+        tp_persona.contact.remove_from_group_async (group);

Shouldn’t you be ‘yield’ing on add_to_group_async() and remove_from_group_async()?

The method should also specify that it “throws GLib.Error”, propagated from these calls, although it would be better to convert it to a Folks.PropertyError and throw that instead. The change_groups() and change_group() methods in Tpf.Persona can then be hooked up to be fully async and propagate errors properly (which they always should’ve been, but we could never manage it because of the use of queues in the backend).

@@ +1023,3 @@
         }
 
+      tp_persona.contact.remove_async ();

Shouldn’t you ‘yield’ on this too?

@@ +1030,3 @@
     {
+      var contact_ids = new string[1];
+      contact_ids[0] = contact_id;

Couldn’t you do “var contact_ids = { contact_id };”?

@@ +1075,3 @@
+          var persona = yield this._ensure_persona_for_id (contact_id);
+          var tp_persona = (Tpf.Persona) persona;
+          tp_persona.contact.request_subscription_async (add_message);

Yield?

@@ +1079,1 @@
+          return persona;

This breaks the case where the contact already existed — in this case, the method should return null.

::: backends/telepathy/lib/tpf-persona.vala
@@ +445,3 @@
+
+      foreach (var group in removed)
+        this._change_group (group, false);

Please put braces around the bodies of these foreach blocks (even though they’re single lines).

@@ +720,3 @@
+          this._contact_groups_changed (added, removed);
+        });
+      this._contact_groups_changed (this.contact.get_contact_groups (), new string[0]);

You could just use ‘{}’ instead of ‘new string[0]’, I think.

Created attachment 211643 [details] [review]
Fix review comments

(In reply to comment #8)
> Review of attachment 211129 [details] [review]:
> I haven’t run the code yet; I’ll do that once these comments have been
> addressed. Also, it would be good if you could compile your code with valac’s
> --enable-experimental-non-null mode and quickly check if there are any actual
> issues with null dereferences. Most of the output of
> --enable-experimental-non-null is useless, but it’s caught a few genuine bugs
> in the past. There’s no need to make the backend completely
> --enable-experimental-non-null–safe though, since the rest of folks doesn’t
> compile with it yet.

How do I set that valac flag? Why is that flag not set by default if code must pass it?
 
> @@ +645,3 @@
> +        {
> +          warning ("Connection does not implement ContactList iface,
> +              legacy CM are not supported anymore. Stay offline");
> 
> This message should probably be translated and a bit more helpful. Perhaps
> suggest to upgrade Telepathy (though we can’t say anything for specific CMs).

No, translating warning messages is a very bad idea when they are not displayed in GUI. It just makes impossible to debug if someone posts folks debug messages in another language.

I think it is empathy's job to display a proper message to user.

> @@ +677,3 @@
> +        }
> +      this.notify_property ("can-add-personas");
> +      this.notify_property ("can-remove-personas");
> 
> Probably best to freeze/thaw notifications around these.

What would it change?

> @@ +881,3 @@
> +          var personas = new HashSet<Persona> ();
> +          personas.add (persona);
> +          this._emit_personas_changed (null, personas);
> 
> What if persona is this._self_contact? There should either be code to set
> this._self_contact = null in _remove_persona() or here; I’m not sure where is
> best. Probably _remove_persona(). For the sake of symmetry, it might be good to
> put corresponding code in _add_persona() too.

I added code in _remove_persona(), but what do you want to add in _add_persona() ?

> @@ +937,3 @@
>      {
> +      if (this._conn.contact_list_state != ContactListState.SUCCESS)
> +        return;
> 
> Does Telepathy guarantee that the contact-list-state never decreases?

Yes, once state reach SUCCESS, it won't change anymore.

> @@ +1079,1 @@
> +          return persona;
> 
> This breaks the case where the contact already existed — in this case, the
> method should return null.

That behaviour seems broken, tbh. I consider this as a bug fix.



Fixed all the rest. I have a question about async methods though: I see some that warning() and the return. Shouldn't them throw an error rather?

Checked the memory usage of folks-inspector as shown in gnome-system-monitor:

master: 12.7Mb
contact-list branch: 10.5Mb

That's probably not scientific measurement, but still :)

Review of attachment 211643 [details] [review]:

::: backends/telepathy/lib/tpf-persona-store.vala
@@ +340,3 @@
       debug.unindent ();
 
+      debug.print_line (domain, level, "%u Tpcontact–Persona mappings:",

s/Tpcontact/TpContact/

@@ +353,3 @@
       debug.unindent ();
 
+      debug.print_line (domain, level, "%u favourite TpContact ids:",

s/ids/IDs/

@@ +468,3 @@
+              this._logger.favourite_contacts_changed.connect (
+                  this._favourite_contacts_changed_cb);
+              this._initialise_favourite_contacts.begin ();

Since this._initialise_favourite_contacts() now throws an error, you need to connect the GAsyncReadyCallback for this async method call, or the error will get lost. i.e.:
    this._initialise_favourite_contacts.begin ((o, r) =>
      {
        try
          {
            this._initialise_favourite_contacts.end (r);
          }
        catch (GLib.Error e1)
          {
            warning (…);
          }
      });

@@ +563,3 @@
           if (p != null)
+            {
+              p._set_is_favourite (false);

Please make sure this Tpf.Persona._set_is_favourite() change is done as a separate commit from the ContactList stuff.

@@ +691,3 @@
+      /* FIXME: TpConnection still does not have high-level API for this.
+       * See fd.o#14540 */
+      var flags = yield FolksTpLowlevel.connection_get_alias_flags_async (this._conn);

This could remain using .begin()/.end() as it was before rather than ‘yield’, actually, since it can safely happen in parallel with other things. Might reduce startup time a bit.

@@ +835,2 @@
         {
+          debug ("Add persona %p", p);

Would be useful to also put the persona’s UID in this debug message.

@@ +847,2 @@
         {
+          debug ("Remove persona %p", p);

Would be useful to add the UID here too.

@@ +905,3 @@
+
+      debug ("Persona %p created for TpContact %s, favourite: %s",
+          persona, contact.get_identifier (), is_favourite ? "yes" : "no");

Would be helpful to include the persona’s UID here.

@@ +1028,3 @@
+          /* Translators: the parameter is an error message. */
+          throw new PropertyError.UNKNOWN_ERROR (
+              _("Failed to change group membershit: %s"), e.message);

s/membershit/membership/!

(In reply to comment #10)
> (In reply to comment #8)
> > Review of attachment 211129 [details] [review] [details]:
> > I haven’t run the code yet; I’ll do that once these comments have been
> > addressed. Also, it would be good if you could compile your code with valac’s
> > --enable-experimental-non-null mode and quickly check if there are any actual
> > issues with null dereferences. Most of the output of
> > --enable-experimental-non-null is useless, but it’s caught a few genuine bugs
> > in the past. There’s no need to make the backend completely
> > --enable-experimental-non-null–safe though, since the rest of folks doesn’t
> > compile with it yet.
> 
> How do I set that valac flag? Why is that flag not set by default if code must
> pass it?

You should be able to set the VALAFLAGS="--enable-experimental-non-null" environment variable (just like CFLAGS). If that fails, just temporarily add it to VALAFLAGS on configure.ac:259.

folks doesn’t currently pass with this flag enabled, so there’s no “need” for code to pass it. I just thought it would be helpful to:
 • potentially find null-dereferencing problems, and
 • get a bit more of folks to compile with the flag enabled.

If you don’t have time to play around with the flag, feel free to ignore this. :-)

> > @@ +645,3 @@
> > +        {
> > +          warning ("Connection does not implement ContactList iface,
> > +              legacy CM are not supported anymore. Stay offline");
> > 
> > This message should probably be translated and a bit more helpful. Perhaps
> > suggest to upgrade Telepathy (though we can’t say anything for specific CMs).
> 
> No, translating warning messages is a very bad idea when they are not displayed
> in GUI. It just makes impossible to debug if someone posts folks debug messages
> in another language.
> 
> I think it is empathy's job to display a proper message to user.

Fair enough; it’s OK to not translate the message as long as Empathy will show a suitable message.

> > @@ +677,3 @@
> > +        }
> > +      this.notify_property ("can-add-personas");
> > +      this.notify_property ("can-remove-personas");
> > 
> > Probably best to freeze/thaw notifications around these.
> 
> What would it change?

I can imagine clients connecting to all of those notify::can-*-personas signals and running the same code whenever any of them are emitted. Adding freeze/thaw would improve that slightly, but as you say it doesn’t change much. Please add them anyway.

> > @@ +881,3 @@
> > +          var personas = new HashSet<Persona> ();
> > +          personas.add (persona);
> > +          this._emit_personas_changed (null, personas);
> > 
> > What if persona is this._self_contact? There should either be code to set
> > this._self_contact = null in _remove_persona() or here; I’m not sure where is
> > best. Probably _remove_persona(). For the sake of symmetry, it might be good to
> > put corresponding code in _add_persona() too.
> 
> I added code in _remove_persona(), but what do you want to add in
> _add_persona() ?

I was thinking about something like:
    if (p.is_user)
      {
        this._self_persona = p;
      }
but if you don’t think it’s necessary, then don’t add it.

> > @@ +937,3 @@
> >      {
> > +      if (this._conn.contact_list_state != ContactListState.SUCCESS)
> > +        return;
> > 
> > Does Telepathy guarantee that the contact-list-state never decreases?
> 
> Yes, once state reach SUCCESS, it won't change anymore.

Might be worth adding a comment saying this.

> > @@ +1079,1 @@
> > +          return persona;
> > 
> > This breaks the case where the contact already existed — in this case, the
> > method should return null.
> 
> That behaviour seems broken, tbh. I consider this as a bug fix.

You’re probably right, but unfortunately that’s the behaviour specified in the documentation for Folks.PersonaStore.add_persona_from_details(), and it would be a pointless behaviour break to change it.

> Fixed all the rest. I have a question about async methods though: I see some
> that warning() and the return. Shouldn't them throw an error rather?

Yes, they should all throw an error. Most of the ones which use warning() are left over from the early days of folks when we were still working out what works best (and also battling introspection problems).

(In reply to comment #11)
> Checked the memory usage of folks-inspector as shown in gnome-system-monitor:
> 
> master: 12.7Mb
> contact-list branch: 10.5Mb
> 
> That's probably not scientific measurement, but still :)

\o/

Created attachment 211748 [details] [review]
Port TpfPersonaStore to high-level tp-glib APIs

It now uses Connection.ContactList iface instead of deprecated
ContactList channels.

Note that this introduce an important behaviour change: folks will
no longer pull all TpContact features, but rely on the user to define
features needed on the default AM's factory.

Created attachment 211749 [details] [review]
Port TpfPersonaStore to high-level tp-glib APIs

It now uses Connection.ContactList iface instead of deprecated
ContactList channels.

Note that this introduce an important behaviour change: folks will
no longer pull all TpContact features, but rely on the user to define
features needed on the default AM's factory.

Created attachment 211751 [details] [review]
Add internal TpfPersona._set_is_favourite

Created attachment 211752 [details] [review]
Various coding style fixes, add comments and debug()

Created attachment 211753 [details] [review]
TpfPersonaStore: disconnect signals in _reset

Created attachment 211754 [details] [review]
TpfPersonaStore: warn on error initialising tp logger

Created attachment 211755 [details] [review]
TpfPersonaStore: remove the store if connection does not implement ContactList

Created attachment 211756 [details] [review]
TpfPersonaStore: freeze/thaw notify signals when changing connection

Created attachment 211757 [details] [review]
TpfPersona: throw PropertyError when an error occure changing group membership

Created attachment 211758 [details] [review]
TpfPersonaStore: throw PersonaStoreError if an error occure while removing the contact

Created attachment 211759 [details] [review]
TpfPersonaStore: return null in add_persona_from_details() if persona is already in contact list

Created attachment 211762 [details] [review]
Add back TpContact features

This is for backward compatibility

The patches all look fine. I had a chance to play around with your branch using Empathy this afternoon, and almost everything seems to work fine. One problem is with favourites:
 1. Edit a non-favourite contact and mark them as a favourite. They’ll be added to the favourites group in the UI correctly.
 2. Go offline and online again.
 3. The contact has disappeared from the favourites group, and is no longer marked as a favourite.

I think the branch will be ready to merge once this problem’s fixed. :-)

(In reply to comment #13)
> (In reply to comment #10)
> > (In reply to comment #8)
> > > Review of attachment 211129 [details] [review] [details] [details]:
> > > I haven’t run the code yet; I’ll do that once these comments have been
> > > addressed. Also, it would be good if you could compile your code with valac’s
> > > --enable-experimental-non-null mode and quickly check if there are any actual
> > > issues with null dereferences. Most of the output of
> > > --enable-experimental-non-null is useless, but it’s caught a few genuine bugs
> > > in the past. There’s no need to make the backend completely
> > > --enable-experimental-non-null–safe though, since the rest of folks doesn’t
> > > compile with it yet.
> > 
> > How do I set that valac flag? Why is that flag not set by default if code must
> > pass it?
> 
> You should be able to set the VALAFLAGS="--enable-experimental-non-null"
> environment variable (just like CFLAGS). If that fails, just temporarily add it
> to VALAFLAGS on configure.ac:259.
> 
> folks doesn’t currently pass with this flag enabled, so there’s no “need” for
> code to pass it. I just thought it would be helpful to:
>  • potentially find null-dereferencing problems, and
>  • get a bit more of folks to compile with the flag enabled.
> 
> If you don’t have time to play around with the flag, feel free to ignore this.
> :-)

Did you get a chance to play around with this?

Created attachment 211874 [details] [review]
TpfPersonaStore: do not reset _favourite_ids at each reconnect

Voilà bug fixed.

about nullable flag, I tried but it generate tones of warnings everywhere and not only on my code.

(In reply to comment #28)
> Voilà bug fixed.

Yes, but now there’s this bug:
 1. Mark a contact as a favourite (or select an existing favourite).
 2. Edit them and mark them as *not* a favourite. They disappear from the favourites group as expected.
 3. Go offline and online again. They reappear in the favourites group.


Once that’s fixed, please squash the commits as you see fit, and merge everything to master. :-) I don’t see any point in delaying this branch any further.

Thanks again for your work on this.


> about nullable flag, I tried but it generate tones of warnings everywhere and
> not only on my code.

OK, no problem. I’ll take a look at it when I next have time to play around with --enable-experimental-non-null.

It seems the logger does not emit change signal so the id stay in the _favourite_ids set. I don't see how this can be the fault of the persona store.

I merged the code in master already, will dig that issue later.

Reported bug #674058 for that.