GNOME Bugzilla – Bug 654907
The writable store shouldn't be set by type_id
Last modified: 2011-08-02 18:29:24 UTC
Back in the key-file and tracker days this made perfect sense: var store_type_id = Environment.get_variable ("FOLKS_WRITEABLE_STORE"); if (store_type_id != null) { this._configured_writeable_store_type_id = store_type_id; } else { this._configured_writeable_store_type_id = "key-file"; .... With eds its plain wrong. How should we do this? Maybe something like type_id:store_id (which for the case of the default address book would translate to eds:system)? Maybe we want variables like the_default_addressbook and hide away eds details from our users?
I forgot to mention this and although its obvious I'll just state it for posterity: its wrong to set the writable store via type_id because for eds, in contrast to the Tracker and key-file backends, its pretty common to have more than one store (many local address books, Google backends, etc).
The latest designs for the 'online accounts' panel have a 'default' checkbox: https://live.gnome.org/Design/SystemSettings/OnlineAccounts Not sure its related, but it might.
(In reply to comment #0) > How should we do this? Maybe something like type_id:store_id (which for the > case of the default address book would translate to eds:system)? Agreed. We're selecting a single persona store to be the main writeable one, so we need to be able to select by a combination of PersonaStore.type_id and PersonaStore.id. I suggest we change FOLKS_WRITEABLE_STORE to use type_id:id as you suggest, and add explicit checks for “key-file” and “tracker” to provide backwards compatibility.
Created attachment 192346 [details] [review] Configure the writeable store by type_id and store_id
Review of attachment 192346 [details] [review]: ::: folks/individual-aggregator.vala @@ +230,3 @@ + if (store_config_ids.str (":") != null) + { + var ids = store_config_ids.split (":"); It would be safer to limit the number of parts to 2 using the second parameter of split(). This also restricts us to never using colons in PersonaStore type IDs, which should be documented. @@ +459,3 @@ + if (store.type_id == this._configured_writeable_store_type_id && + (this._configured_writeable_store_id == "" || + this._configured_writeable_store_id == store.id)) If I set FOLKS_WRITEABLE_STORE=eds, this will result in all eds PersonaStores being writeable (I think). Do we want that?
(In reply to comment #5) > Review of attachment 192346 [details] [review]: > > ::: folks/individual-aggregator.vala > @@ +230,3 @@ > + if (store_config_ids.str (":") != null) > + { > + var ids = store_config_ids.split (":"); > > It would be safer to limit the number of parts to 2 using the second parameter > of split(). > > This also restricts us to never using colons in PersonaStore type IDs, which > should be documented. > > @@ +459,3 @@ > + if (store.type_id == this._configured_writeable_store_type_id && > + (this._configured_writeable_store_id == "" || > + this._configured_writeable_store_id == store.id)) > > If I set FOLKS_WRITEABLE_STORE=eds, this will result in all eds PersonaStores > being writeable (I think). Do we want that? More than that, the line just after the end of the patch sets this._writeable_store (ie, the primary store). So setting FOLKS_WRITEABLE_STORE=eds would result in one of the EDS addressbooks being selected in a non-clear (and potentially non-deterministic) way. I think we should print some very obvious warning in this case (ie, the PersonaStore ID is unspecified in FOLKS_WRITEABLE_STORE when it really needs to be) and maybe flat-out quit (since writing to an arbitrary PersonaStore could be very bad and only developers will be setting this environment variable anyhow).
Created attachment 193041 [details] [review] Configure the writeable store by type_id and store_id Addressed the following comment: - set the max number of tokens when using split() - require the store_id to be set when type_id = 'eds' - document that PersonaStore#type_id must not contain colons
Review of attachment 193041 [details] [review]: Looks good to me apart from this minor comment. Please commit once you've fixed it. ::: folks/persona-store.vala @@ +374,3 @@ + * Note: the type_id must not contain colons because the primary writeable + * store is configured, either via GConf or the FOLKS_WRITEABLE_STORE + * env variable, with a string of the from 'type_id:store_id'. I wouldn't document this publicly, since then it becomes part of the API, and people might start relying on it. I think type_ids should remain opaque. Just move this paragraph into a non-valadoc comment below. Also, s/from/form/.
commit 5c062183b8475bc46d7acb7c225e749e992fd3fa Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Thu Jul 21 01:33:50 2011 +0100 Configure the writeable store by type_id and store id Fixes: https://bugzilla.gnome.org/show_bug.cgi?id=654907 NEWS | 1 + folks/individual-aggregator.vala | 44 +++++++++++++++++++++++++++++++------- folks/persona-store.vala | 8 ++++++- 3 files changed, 44 insertions(+), 9 deletions(-)