[folks] Bug 667410 â A second aggregator instance only fetches a subset of contacts

[Philip Withnall <pwithnall src gnome org>]

commit d258c86814c0dd4405a8a2b7a45f3d2a036fa1eb
Author: Philip Withnall <philip tecnocode co uk>
Date:   Fri Jan 6 23:05:05 2012 +0000

    Bug 667410 â A second aggregator instance only fetches a subset of contacts
    
    This was happening because the initial BackendStore was hanging around across
    multiple IndividualAggregator instances, keeping all the Backends,
    PersonaStores and Personas alive.
    
    The IndividualAggregator didnât have code to deal with pre-prepared Backends
    and PersonaStores, meaning it never realised the Personas existed (because
    they werenât announced via personas-changed signals), and thus never created
    Individuals out of them.
    
    This commit fixes the problem by having IndividualAggregator check for
    existing Backends, PersonaStores and Personas when prepare() is called.
    
    It also adds a test case to the folks test suite, based on the one written
    by Guillaume in bgo#667410.
    
    Closes: https://bugzilla.gnome.org/show_bug.cgi?id=667410

 NEWS                             |    2 +
 folks/backend.vala               |   10 ++++
 folks/individual-aggregator.vala |   50 ++++++++++++++++++++-
 folks/persona-store.vala         |   10 ++++
 tests/folks/init.vala            |   91 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 161 insertions(+), 2 deletions(-)
---
diff --git a/NEWS b/NEWS
index bfb878b..ce61435 100644
--- a/NEWS
+++ b/NEWS
@@ -6,6 +6,8 @@ Bugs fixed:
 * Bug 666540 â Segfault on empty e-mail addresses with potential match
 * Bug 659610 â Support code coverage report generation
 * Bug 657063 â Allow to pass a command to folks-inspect
+* Bug 667410 â A second instance of the aggregator only fetch a small subset of
+  my contacts
 
 API changes:
 * Add PostalAddress.is_empty() and Role.is_empty()
diff --git a/folks/backend.vala b/folks/backend.vala
index 09fc846..a4b7c5b 100644
--- a/folks/backend.vala
+++ b/folks/backend.vala
@@ -35,6 +35,16 @@ using Gee;
  */
 public abstract class Folks.Backend : Object
 {
+  construct
+    {
+      debug ("Constructing Backend â%sâ (%p)", this.name, this);
+    }
+
+  ~Backend ()
+    {
+      debug ("Destroying Backend â%sâ (%p)", this.name, this);
+    }
+
   /**
    * Whether { link Backend.prepare} has successfully completed for this
    * backend.
diff --git a/folks/individual-aggregator.vala b/folks/individual-aggregator.vala
index 6c3cd51..e008695 100644
--- a/folks/individual-aggregator.vala
+++ b/folks/individual-aggregator.vala
@@ -351,12 +351,14 @@ public class Folks.IndividualAggregator : Object
           disable_linking == "no" || disable_linking == "0");
 
       this._backend_store = BackendStore.dup ();
-      this._backend_store.backend_available.connect (
-          this._backend_available_cb);
+
+      debug ("Constructing IndividualAggregator %p", this);
     }
 
   ~IndividualAggregator ()
     {
+      debug ("Destroying IndividualAggregator %p", this);
+
       this._backend_store.backend_available.disconnect (
           this._backend_available_cb);
 
@@ -505,10 +507,33 @@ public class Folks.IndividualAggregator : Object
             {
               this._prepare_pending = true;
 
+              this._backend_store.backend_available.connect (
+                  this._backend_available_cb);
+
+              /* Load any backends which already exist. This could happen if the
+               * BackendStore has stayed alive after being used by a previous
+               * IndividualAggregator instance. */
+              var backends = this._backend_store.enabled_backends.values;
+              foreach (var backend in backends)
+                {
+                  this._backend_available_cb (this._backend_store, backend);
+                }
+
+              /* Load any backends which haven't been loaded already. (Typically
+               * all of them.) */
               yield this._backend_store.load_backends ();
 
               this._is_prepared = true;
               this.notify_property ("is-prepared");
+
+              /* Mark the aggregator as having reached a quiescent state if
+               * appropriate. This will typically only happen here in cases
+               * where the stores were all prepared and quiescent before the
+               * aggregator was created. */
+              if (this._is_quiescent == false)
+                {
+                  this._notify_if_is_quiescent ();
+                }
             }
           finally
             {
@@ -699,6 +724,9 @@ public class Folks.IndividualAggregator : Object
   private void _backend_persona_store_added_cb (Backend backend,
       PersonaStore store)
     {
+      debug ("_backend_persona_store_added_cb(): backend: %s, store: %s (%p)",
+          backend.name, store.id, store);
+
       var store_id = this._get_store_full_id (store.type_id, store.id);
 
       this._maybe_configure_as_primary (store);
@@ -723,6 +751,24 @@ public class Folks.IndividualAggregator : Object
           this._non_quiescent_persona_store_count++;
         }
 
+      /* Handle any pre-existing personas in the store. This can happen if the
+       * store existed (and was prepared) before this IndividualAggregator was
+       * constructed. */
+      if (store.personas.size > 0)
+        {
+          var persona_set = new HashSet<Persona> ();
+          foreach (var p in store.personas.values)
+            {
+              persona_set.add (p);
+            }
+
+          this._personas_changed_cb (store, persona_set,
+              new HashSet<Persona> (), null, null,
+              GroupDetails.ChangeReason.NONE);
+        }
+
+      /* Prepare the store and receive a load of other personas-changed
+       * signals. */
       store.prepare.begin ((obj, result) =>
         {
           try
diff --git a/folks/persona-store.vala b/folks/persona-store.vala
index 118a7e7..c885a83 100644
--- a/folks/persona-store.vala
+++ b/folks/persona-store.vala
@@ -280,6 +280,16 @@ public enum Folks.PersonaDetail
  */
 public abstract class Folks.PersonaStore : Object
 {
+  construct
+    {
+      debug ("Constructing PersonaStore â%sâ (%p)", this.id, this);
+    }
+
+  ~PersonaStore ()
+    {
+      debug ("Destroying PersonaStore â%sâ (%p)", this.id, this);
+    }
+
   /**
    * The following list of properties are the basic keys
    * that each PersonaStore with write capabilities should
diff --git a/tests/folks/init.vala b/tests/folks/init.vala
index be0090c..7ab4b9e 100644
--- a/tests/folks/init.vala
+++ b/tests/folks/init.vala
@@ -15,6 +15,7 @@
  * along with this library.  If not, see <http://www.gnu.org/licenses/>.
  *
  * Authors: Guillaume Desmottes <guillaume desmottes collabora co uk>
+ *          Philip Withnall <philip tecnocode co uk>
  */
 
 using Gee;
@@ -35,6 +36,7 @@ public class InitTests : Folks.TestCase
 
       /* Set up the tests */
       this.add_test ("looped", this.test_looped);
+      this.add_test ("individual-count", this.test_individual_count);
     }
 
   public override void set_up ()
@@ -84,6 +86,95 @@ public class InitTests : Folks.TestCase
       this._tp_backend.remove_account (account1_handle);
       this._kf_backend.tear_down ();
     }
+
+  /* Prepare an aggregator and wait for quiescence, then count how many
+   * individuals it contains. Loop and do the same thing again, then compare
+   * the numbers of individuals and their IDs. Do this several times.
+   *
+   * This tests that the preparation code in IndividualAggregator can handle
+   * Backends and PersonaStores which have been prepared before the aggregator
+   * was created. To a lesser extent, it also tests that the aggregation code
+   * is deterministic. See: bgo#667410. */
+  public void test_individual_count ()
+    {
+      var main_loop = new GLib.MainLoop (null, false);
+
+      this._kf_backend.set_up (
+          "[0]\n" +
+          "msn=foo hotmail com\n" +
+          "[1]\n" +
+          "__alias=Bar McBadgerson\n" +
+          "jabber=bar jabber org\n");
+
+      void* account1_handle = this._tp_backend.add_account ("protocol",
+          "me example com", "cm", "account");
+      void* account2_handle = this._tp_backend.add_account ("protocol",
+          "me2 example com", "cm", "account2");
+
+      /* Run the test loop. */
+      Idle.add (() =>
+        {
+          this._test_individual_count_loop.begin ((obj, res) =>
+            {
+              this._test_individual_count_loop.end (res);
+              main_loop.quit ();
+            });
+
+          return false;
+        });
+
+      main_loop.run ();
+
+      /* Clean up for the next test */
+      this._tp_backend.remove_account (account2_handle);
+      this._tp_backend.remove_account (account1_handle);
+
+      this._kf_backend.tear_down ();
+    }
+
+  private async void _test_individual_count_loop ()
+    {
+      string[]? previous_individual_ids = null;
+
+      for (uint i = 0; i < 10; i++)
+        {
+          var aggregator = new IndividualAggregator ();
+
+          try
+            {
+              yield TestUtils.aggregator_prepare_and_wait_for_quiescence (
+                  aggregator);
+            }
+          catch (GLib.Error e1)
+            {
+              GLib.critical ("Error preparing aggregator: %s", e1.message);
+            }
+
+          if (previous_individual_ids == null)
+            {
+              /* First iteration; store the set of IDs. */
+              previous_individual_ids = aggregator.individuals.keys.to_array ();
+            }
+          else
+            {
+              /* Compare this set to the previous aggregator's set. */
+              debug ("%u vs %u individuals:", previous_individual_ids.length,
+                  aggregator.individuals.size);
+              assert (previous_individual_ids.length ==
+                  aggregator.individuals.size);
+              assert (aggregator.individuals.size > 0);
+
+              foreach (var id in previous_individual_ids)
+                {
+                  debug ("  %s", id);
+                  assert (aggregator.individuals.has_key (id) == true);
+                }
+            }
+
+          /* Destroy the aggregator and loop. */
+          aggregator = null;
+        }
+    }
 }
 
 public int main (string[] args)