GNOME Bugzilla – Bug 657141
Backend should ask eds for the default backend, not hardcode it
Last modified: 2011-09-16 20:16:56 UTC
If eds is set up such that the gmail addressbook is the default we should pick that up. Currently we just hardcode "system" which means we always use the local database which is often not what people want.
(In reply to comment #0) > If eds is set up such that the gmail addressbook is the default we should pick > that up. Currently we just hardcode "system" which means we always use the > local database which is often not what people want. Well, Folks uses GConf keys (or gsettings eventually) to define the primary store. I am guessing this makes sense while we still have the key-file backend around. Once that is gone and primary stores can only come from e-d-s, then querying e-d-s for the default address book would be a better way of defining the primary store. With this in mind, I had filed this for Gnome Contacts some time ago: https://bugzilla.gnome.org/show_bug.cgi?id=654908
(In reply to comment #1) > (In reply to comment #0) > > If eds is set up such that the gmail addressbook is the default we should pick > > that up. Currently we just hardcode "system" which means we always use the > > local database which is often not what people want. > > Well, Folks uses GConf keys (or gsettings eventually) to define the primary > store. I am guessing this makes sense while we still have the key-file backend > around. Once that is gone and primary stores can only come from e-d-s, then > querying e-d-s for the default address book would be a better way of defining > the primary store. > > With this in mind, I had filed this for Gnome Contacts some time ago: > > https://bugzilla.gnome.org/show_bug.cgi?id=654908 I don't see why we couldn't just request the default from e-d-s in the place we currently just hardcode "system".
Yeah, It might be interesting for some to have folks specific store selection, but by default we should use the desktop-wide default addressbook, which is the one set as default in e-d-s.
(In reply to comment #2) > (In reply to comment #1) > > (In reply to comment #0) > > > If eds is set up such that the gmail addressbook is the default we should pick > > > that up. Currently we just hardcode "system" which means we always use the > > > local database which is often not what people want. > > > > Well, Folks uses GConf keys (or gsettings eventually) to define the primary > > store. I am guessing this makes sense while we still have the key-file backend > > around. Once that is gone and primary stores can only come from e-d-s, then > > querying e-d-s for the default address book would be a better way of defining > > the primary store. > > > > With this in mind, I had filed this for Gnome Contacts some time ago: > > > > https://bugzilla.gnome.org/show_bug.cgi?id=654908 > > I don't see why we couldn't just request the default from e-d-s in the place we > currently just hardcode "system". Note that this (selecting the writeable PersonaStore) happens from the IndividualAggregator - where we can't have any explicit dependency with any given backend (in this case, the e-d-s one). So we'll need some way to set generic properties on PersonaStores so we can then query them from the IndividualAggregator without creating hard dependencies.
Initial take at this, following what was discussed on IRC with Philip: http://cgit.collabora.com/git/user/rgs/folks/log/?h=bgo-657141
Created attachment 194648 [details] [review] Add writeable store property to Folks backends
Review of attachment 194648 [details] [review]: I think we need to consider change notification for the property. Apart from that the patch looks good. ::: backends/eds/eds-backend.vala @@ +148,3 @@ unowned GLib.SList<weak E.SourceGroup> groups = this._ab_sources.peek_groups (); + unowned E.Source? default_source = list.peek_default_source (); Is it possible to get a change notification for this? For example, if the user changes their default address book in Evo while folks is being used elsewhere? @@ +229,3 @@ + if (this._default_source_uri == s.peek_relative_uri ()) + { + this._writeable_store_id = store.id; Need to call this.notify_property(). @@ +246,3 @@ + if (this._writeable_store_id == store.id) + { + this._writeable_store_id = null; Need to call this.notify_property(). ::: folks/backend.vala @@ +127,3 @@ + * This will be used to identify which {@link PersonaStore} from within + * this backend should be used by the {@link IndividualAggregator} as + * the primary (writeable) one. Mention that `null` means the backend has no preferred writeable store at the moment. You should probably also mention that the preferred writeable store could change, so clients (i.e. IndividualAggregator) should listen for changes to the property. ::: folks/individual-aggregator.vala @@ +485,3 @@ if (store.type_id == this._configured_writeable_store_type_id) { + if (backend.writeable_store_id == store.id) I think this needs some extra conditions to make sure that iff the user has set FOLKS_WRITEABLE_STORE or the GConf key, we don't change this._writeable_store to a store which isn't what they set. Also, the IndividualAggregator needs to listen to change notifications for backend.writeable_store_id and update this._writeable_store as appropriate.
Created attachment 194711 [details] [review] Patch refreshed after review comments
Created attachment 194712 [details] [review] Patch refreshed after review comments
Review of attachment 194712 [details] [review]: ::: backends/eds/eds-backend.vala @@ +219,3 @@ var store = new Edsf.PersonaStore (s); + store.notify["is-default-adddresso-book"].connect ( s/addresso/address/ @@ +255,3 @@ + this._writeable_store_id == store.id) + { + this._writeable_store_id = null; Is it possible to reset the writeable store ID back to an (appropriate) different store here? e.g. Could we iterate through the source list and check for the new default? ::: backends/eds/lib/edsf-persona-store.vala @@ +127,2 @@ /** + * Whether this PersonaStore represents a default address book s/a/the/? ::: folks/individual-aggregator.vala @@ +484,3 @@ + this._configured_writeable_store_id = store.id; + this._writeable_store.is_writeable = true; + this._writeable_store.trust_level = PersonaStoreTrust.FULL; Shouldn't we notify of the change to IndividualAggregator.primary_store here? @@ +518,3 @@ + store.is_writeable = true; + store.trust_level = PersonaStoreTrust.FULL; + this._writeable_store = store; We should also notify of the change to IndividualAggregator.primary_store here and in the rest of this function. This should probably be a separate commit.
(In reply to comment #10) > Review of attachment 194712 [details] [review]: > @@ +255,3 @@ > + this._writeable_store_id == store.id) > + { > + this._writeable_store_id = null; > > Is it possible to reset the writeable store ID back to an (appropriate) > different store here? e.g. Could we iterate through the source list and check > for the new default? We could.. not sure we want to (think convoluted setups, maaany address books). > @@ +518,3 @@ > + store.is_writeable = true; > + store.trust_level = PersonaStoreTrust.FULL; > + this._writeable_store = store; > > We should also notify of the change to IndividualAggregator.primary_store here > and in the rest of this function. This should probably be a separate commit. Why? This is all part of "setting the writeable store to whatever the chosen backend says it shold be". What different logical changes do you see?
Oh - I think you meant the missing notifications for primary store changes (if so, sorry for the extra question).
(In reply to comment #12) > Oh - I think you meant the missing notifications for primary store changes (if > so, sorry for the extra question). I did. Sorry if I didn't make that clear. I was suggesting it should be a separate commit because those notifications are missing at the moment and should be added anyway, regardless of the work to fix this bug.
Didn't make this release; punting to 0.6.2.
Latest iteration of this lives here: http://cgit.collabora.com/git/user/rgs/folks/log/?h=bgo-657141
Created attachment 195516 [details] [review] Updated patch Went over review comments and added a test case.
Review of attachment 195516 [details] [review]: + private void _check_if_default () _notify_if_default() would be a better name + store.notify["is-default-address-book"].connect ( + this._default_store_cb); + + if (store.is_default_address_book) + { + this._writeable_store_id = store.id; + this.notify_property ("writeable-store-id"); + } + store.removed.connect (this._store_removed_cb); this._persona_stores.set (store.id, store); @@ -217,10 +235,29 @@ public class Folks.Backends.Eds.Backend : Folks.Backend this.persona_store_added (store); } + private void _default_store_cb (Object store_obj, ParamSpec ps) + { + Edsf.PersonaStore store = (Edsf.PersonaStore) store_obj; + if (store.is_default_address_book) + { + this._writeable_store_id = store.id; + this.notify_property ("writeable-store-id"); + } + } Factor out some common code here + * - if link_personas () is called while we are swapping + * stores the Universe might collapse. Should we hold + * a lock then or set this._linking_enabled = false? Locking sounds appropriate here. link_personas() and the various entry points to changing or invalidating the writeable store should use the same lock. And because locking is so tricky, we should be extra careful to have thorough tests for this (ie, add some test cases to change-primary-store that involve many racy linkings with many store swaps).
Created attachment 195558 [details] [review] patch Addressed what was pointed out by Travis.
Review of attachment 195558 [details] [review]: ::: backends/eds/eds-backend.vala @@ +297,3 @@ + + private void _notify_if_writeable (Edsf.PersonaStore store, + StoreAction action) This function could do with a comment. ::: backends/eds/lib/edsf-persona-store.vala @@ +148,2 @@ /** + * Whether this PersonaStore represents the default address book Do we expect more than one store to ever have this set to true? Might also help to define what is meant by “the default address book” — i.e. the one the user's ticked as the default in Evolution; the one which Evolution uses by default for type-ahead e-mail address completion, etc. ::: folks/individual-aggregator.vala @@ +79,3 @@ private static const string _FOLKS_CONFIG_KEY = "/system/folks/backends/primary_store"; + private bool _user_set_writeable_store; This could do with a comment explaining what it means/is used for. @@ +534,3 @@ + * + * - all around the IndividualAggregator we have + * assertions about the primary store, it's trust-level s/it's/its/ @@ +535,3 @@ + * - all around the IndividualAggregator we have + * assertions about the primary store, it's trust-level + * and writeability and the state of this attributes s/this/these/ :-) @@ +541,3 @@ + * store. + * - we protect ourselves with a lock so link_personas () + * won't run while we are swapping the primary store I'm not entirely convinced this is true. IIRC, we established before that lock() in Vala translates to a GRecMutex — so re-entrant calls to this code from itself or from link_personas() (or vice-versa) won't be mutually exclusive. Put another way, IIRC lock() only provides mutual exclusion between strictly different threads. (Please, please correct me if I'm wrong here.) Therefore, there needs to be some reasoning in this comment as to why this code will never be called re-entrantly from link_personas() (and vice-versa). What would make this all simpler is if you could guarantee that this code is only ever called in the main thread, and then also guarantee that link_personas() never causes the main loop to be iterated. If that's the case, there's no need for any of the locking or any exclusion variables. @@ +563,3 @@ + { + previous_writeable_store.is_writeable = false; + previous_writeable_store.trust_level = PersonaStoreTrust.NONE; Should this not be PersonaStoreTrust.PARTIAL for some stores?
(In reply to comment #19) (...) > @@ +563,3 @@ > + { > + previous_writeable_store.is_writeable = false; > + previous_writeable_store.trust_level = PersonaStoreTrust.NONE; > > Should this not be PersonaStoreTrust.PARTIAL for some stores? It should be whatever the default trust of the Store was before it was promoted to trust-level = FULL. We should re-iterate over the defintions of primary-store, having a writeable store and the trust-level of PersonaStores... And also re-think where and when they should be modified.
(Punting bugs to 0.6.3)
Note the TODO in attachment 195603 [details] [review] on bug 657783; that may get merged before the fix for this bug, in which case the TODO needs to be addressed for this bug to be truly fixed.
Created attachment 196646 [details] [review] patch Here goes a simpler approach, based on top of the recent clean ups that have been done to the IndividualAggregator.
Review of attachment 196646 [details] [review]: ::: folks/individual-aggregator.vala @@ +101,3 @@ private uint _non_quiescent_backend_count = 0; private bool _is_quiescent = false; + private bool _user_configured_primary_store = false; It's probably worth adding a comment explaining that this variable is true iff the primary store has been set from environment variables or GConf. @@ +625,3 @@ + } + + this._primary_store.is_primary_store = true; Might be an idea to freeze notifications on both stores before changing their is_primary_store properties, then unfreeze notifications once both properties have been set. That way, client code handling notifications for those properties never sees a state where no persona store has is_primary_store == true. @@ +632,3 @@ + + private void _backend_persona_store_added_cb (Backend backend, + PersonaStore store) What about the case of if the store already has user_set_default == true, and _persona_store_user_set_default_changed_cb() never gets called on it? ::: folks/persona-store.vala @@ +649,3 @@ + /** + * Whether this {@link PersonaStore} has been marked as the default + * store by the user. I.e.: a PersonaStore for the e-d-s backend would “as the default store _in its backend_ by the user”? ::: tests/eds/change-primary-store.vala @@ +73,3 @@ + this._main_loop.run (); + + assert (this._new_primary_store_found); Whitespace problem. @@ +108,3 @@ + } + + private PersonaStore? _get_store (BackendStore store, string store_id) Might be better to call this “_get_persona_store” so that people don't get confused between the backend store and persona stores. ::: tests/lib/eds/backend.vala @@ +113,3 @@ } + public void set_default () Perhaps this should be called “set_as_default” instead? “set_default” implies to me that it should take a parameter which says what the new default should be.
Created attachment 196705 [details] [review] patch Updated version.
Review of attachment 196705 [details] [review]: Looks good to me, but I think it would be good to get a second opinion. Travis? ::: tests/eds/change-primary-store.vala @@ +122,3 @@ + private void _primary_store_cb (Object ia_obj, ParamSpec ps) + { + IndividualAggregator ia = (IndividualAggregator) ia_obj; Double space.
Review of attachment 196705 [details] [review]: Looks good to me.
Merged: commit 9ec53e830cf1ff12681b58f068a08cec6511dd8d Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Thu Sep 1 19:29:06 2011 +0100 e-d-s: test case for changing primary store commit b1f9c2df3c5d62184ac51a44195e44401db74fe8 Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Thu Sep 1 19:31:28 2011 +0100 e-d-s: allow setting default address books in tests commit 00ada566e82fef5ed87793b2de7c5cb6e2910ef2 Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Thu Sep 15 15:38:11 2011 +0100 e-d-s: notify when an address book is configured as default Fixes: https://bugzilla.gnome.org/show_bug.cgi?id=657141 commit d797e8e101252ddd674ffdffab61d9409219a48f Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Thu Sep 15 15:33:58 2011 +0100 core: add Folks.PersonaStore.user-set-default property With this property we can tell if a Store (i.e.: an e-d-s address book) has been marked by the user as his default store (either from Evo or from GOA, etc). Helps: https://bugzilla.gnome.org/show_bug.cgi?id=657141 commit 32c95ea8884b415d7835fd1fe147db195fc61f5f Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Thu Sep 15 14:43:36 2011 +0100 IndividualAggregator: improve naming for primary store related methods Renamed _set_primary_store () to _configure_primary_store () and refactor the code that actually sets the primary store into _set_primary_store (). Helps: https://bugzilla.gnome.org/show_bug.cgi?id=657141