GNOME Bugzilla – Bug 689146
disabling address books has no effect
Last modified: 2012-12-07 20:04:26 UTC
Steps to reproduce: - create three EDS address books in addition to the system address book (might also fail with less - this is what I am testing) - populate each with one different contact - folks_backend_set_persona_stores() with these three address books - wait for the aggregator to have three individuals - folks_backend_set_persona_stores([]) (= empty list, no address books active) Expected: - notification(s) that all three individuals were removed Actual: - no changes Here's the debug output for removing the three address books: [DEBUG syncevo-dbus-server 00:00:48] backends already loaded: setting EDS persona stores directly: [] Breakpoint 1, folks_backend_set_persona_stores (self=0x1da3c00, storeids=0x1d64000) at /work/gnome/checkout/folks/folks/backend.vala:135 135 public abstract void set_persona_stores (Set<string>? storeids); (gdb) c Continuing. (syncevo-dbus-server:3647): eds-DEBUG: eds-backend.vala:350: Removing address book 'pim-manager-test-dbus-a'. [Thread 0x7fffdced7700 (LWP 3687) exited] (syncevo-dbus-server:3647): eds-DEBUG: eds-backend.vala:350: Removing address book 'pim-manager-test-dbus-b'. (syncevo-dbus-server:3647): eds-DEBUG: eds-backend.vala:350: Removing address book 'pim-manager-test-dbus-c'. (syncevo-dbus-server:3647): folks-DEBUG: persona-store.vala:325: Destroying PersonaStore ?pim-manager-test-dbus-a? (0x1d65600) (syncevo-dbus-server:3647): folks-DEBUG: persona-store.vala:325: Destroying PersonaStore ?pim-manager-test-dbus-b? (0x1d654f0) (syncevo-dbus-server:3647): folks-DEBUG: persona-store.vala:325: Destroying PersonaStore ?pim-manager-test-dbus-c? (0x1d653e0) waiting for view with contacts for [] Here the test waits forever, because nothing further happens.
From private void _backend_persona_store_removed_cb (Backend backend, PersonaStore store) { ... /* no need to remove this store's personas from all the individuals, since * they'll do that themselves (and emit their own 'removed' signal if * necessary) */ How does this "they'll do that themselves" part work? This doesn't seem to work when the EDS backend removes an address book.
Created attachment 229996 [details] [review] add missing removal notifications I found the missing code... now it works for me as expected.
Review of attachment 229996 [details] [review]: Good catch. ::: backends/eds/eds-backend.vala @@ +70,3 @@ if (this._persona_stores.has_key (store.id)) { + Edsf.PersonaStore estore = (Edsf.PersonaStore)store; Need a space between the cast and ‘store’. @@ +144,3 @@ if (!storeids.contains (store.id)) { + stores_to_remove += (Edsf.PersonaStore)store; Need a space after the cast here as well. ::: backends/eds/lib/edsf-persona-store.vala @@ +2433,3 @@ } + public void emit_all_personas_removed () We can’t really expose this as a public symbol, but are forced to because libfolks-eds is a separate library. I suggest you rename this as “public void remove ()” instead, move the “this.removed ()” signal emission into it, and add a documentation comment which explains that it’s meant to be an internal symbol, and that people should use Backend.set_persona_stores() instead.
Created attachment 230644 [details] [review] revised solution I fixed the cast formatting. I tried implementing a remove() method as suggested. This interacts with the way how eds-backend.vala handles address book removal (directly or via callback) in a way which is not immediately obvious and apparently I didn't get that right, because the result no longer worked correctly. Therefore I suggest to keep the shared code as it is (emit personas removed, but nothing else) and only added a comment.
Review of attachment 230644 [details] [review]: I really don’t like the idea of adding this as a public symbol. Could you try a completely different approach please? Try modifying _backend_persona_store_removed_cb() (as mentioned in comment #1) to remove the store’s personas from their individuals. This should be safe for backends other than EDS, because by that point they won’t be listing any personas. For EDS, it should fix this bug without having to expose an internal method as public.
Created attachment 230766 [details] [review] fix the problem in _backend_persona_store_removed_cb > Could you try a completely different approach please? Try modifying > _backend_persona_store_removed_cb() (as mentioned in comment #1) to remove the > store’s personas from their individuals. Here's a patch using that approach. > This should be safe for backends other > than EDS, because by that point they won’t be listing any personas. For EDS, it > should fix this bug without having to expose an internal method as public. Note that I have not tried this with any backend other than EDS. It worked for me in the test which normally exposes the problem.
Review of attachment 230766 [details] [review]: Looks good to me. Can you test it against another backend (or against EDS with your original patch applied) then commit this patch with a suitable NEWS entry and the minor tweak below applied please? Thanks! ::: folks/individual-aggregator.vala @@ +910,3 @@ + * Therefore remove this store's personas from all the individuals. Should + * not have any effect if a store already triggered the 'removed' signals, + * because then we won't have anything here. Could you include a link to this bug report please?
I don't have other backends working at the moment and therefore cannot test that.
(In reply to comment #8) > I don't have other backends working at the moment and therefore cannot test > that. You should be able to test it against the EDS backend with your original patch (attachment #229996 [details]) applied to it to get it to emit the correct signals.
(In reply to comment #9) > (In reply to comment #8) > > I don't have other backends working at the moment and therefore cannot test > > that. > > You should be able to test it against the EDS backend with your original patch > (attachment #229996 [details]) applied to it to get it to emit the correct signals. That's not quite the same - such a modified backend would emit the signals *and* list them when asked to by this patch. It's still worthwhile to test, though, to see how resilient the _personas_changed_cb() code is against double removal (which would be the worst case scenario for other backends).
(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > I don't have other backends working at the moment and therefore cannot test > > > that. > > > > You should be able to test it against the EDS backend with your original patch > > (attachment #229996 [details] [details]) applied to it to get it to emit the correct signals. > > That's not quite the same - such a modified backend would emit the signals > *and* list them when asked to by this patch. > > It's still worthwhile to test, though, to see how resilient the > _personas_changed_cb() code is against double removal (which would be the worst > case scenario for other backends). I've tested this. _personas_changed_cb() does some redundant work in this case (this._disconnect_from_persona (persona)) but everything works as expected.