From d258c86814c0dd4405a8a2b7a45f3d2a036fa1eb Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 6 Jan 2012 23:05:05 +0000 Subject: [PATCH] =?utf8?q?Bug=20667410=20=E2=80=94=20A=20second=20aggregat?= =?utf8?q?or=20instance=20only=20fetches=20a=20subset=20of=20contacts?= MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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 (); + foreach (var p in store.personas.values) + { + persona_set.add (p); + } + + this._personas_changed_cb (store, persona_set, + new HashSet (), 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 . * * Authors: Guillaume Desmottes + * Philip Withnall */ 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) -- 2.7.4