GNOME Bugzilla – Bug 645441
Mechanism to specify primary backend
Last modified: 2011-04-01 20:55:35 UTC
Right now Folks has the key file backend hard-coded as its primary backend (that is, the one that should be used when linking Personas). There are also a bunch of assertions that state that there should be only one backend with capabilities to create a Persona. We should relax these settings in order to allow: - any other backend, which is fully trusted, to be used as the Primary Backend - have a configure option (i.e.: --default-primary-backend) that defaults to key-file - if we have --enable-tracker-backend=yes we should default to it as the primary backend - have a global config file which defines the primary backend Some other things that we might like to tackle: - atm, IndividualAggregator#add_persona_from_details takes a PersonaStore as one of its arguments. We should probably have this parameter have a default value set to the primary Persona Store. More discussion is needed to rule out (or in) the need for client apps to know to which PersonaStore they are adding a Persona.
Created attachment 184508 [details] [review] Add support to switch primary backend First go.
Review of attachment 184508 [details] [review]: Wouldn't we be better off using dconf for this instead? It should be low enough down in the stack that we can depend on it, and it'll give us all the nice asynchronicity, administrator overrideability, change notification, etc. that we should really have for this sort of configuration stuff. Apart from the config file stuff, the patch is taking a sensible approach and looks like a step in the right direction. I've made this bug depend on bug #645413 since there's little point in committing this until the Tracker backend has write support and we can test the changes necessary to fix this bug.
Since we'd like to support MeeGo right away and dconf won't be available there for a while [1], I suggest we defer using dconf for a bit. [1] https://bugs.meego.com/show_bug.cgi?id=12634
How about using GSettings/dconf, but having a configure switch which conditionally compiles GConf-based code instead? That way, GConf can be used on MeeGo, and when they switch to GSettings/dconf, folks configurations can be migrated. The migration path from GConf to GSettings is fairly smooth — much better than migrating from some custom /etc/folks.conf to GSettings. Putting conditional compilation in there isn't great, but the GConf and GSettings APIs are sufficiently similar that it shouldn't add too much extra complexity.
Created attachment 184836 [details] [review] Setup our own GConf instance for tests This was cherry-picked from the pending e-d-s branch, needed now that we'll be using GConf to read config options.
Created attachment 184837 [details] [review] Add support to switch primary backend GConf version of the previously posted patch.
Created attachment 184838 [details] [review] Make sure Trf.Persona knows how to deal with linkable properties Needed to link Trf.Personas.
Created attachment 184839 [details] [review] Test linking personas using Tracker as the primary backend Tests using Tracker to add a linking Persona.
Created attachment 184840 [details] [review] Test linking personas using Tracker as the primary backend Small update.
Review of attachment 184836 [details] [review]: I don't know much about GConf path magic, but I did spot this: ::: tests/data/Makefile.am @@ +6,3 @@ + rm -rf gconf.d + +clean-local: clean-gconf These two “clean-” rules should be added to .PHONY.
Review of attachment 184837 [details] [review]: I presume the necessary autoconf magic to switch between GSettings and GConf will be added once the GSettings/dconf support is added? I guess that's a sensible way of doing it. ::: folks/individual-aggregator.vala @@ +81,2 @@ /** + * Our configured primary (writeable) store. Might be useful to mention how it's set (i.e. from GConf/an environment variable). @@ +159,3 @@ + var store_type_id = Environment.get_variable ("FOLKS_WRITEABLE_STORE"); + if (store_type_id != null) + this._configured_writeable_store_type_id = store_type_id; Since the “else” block uses braces, the “if” block should too, for consistency.
Review of attachment 184838 [details] [review]: Looks good.
Review of attachment 184840 [details] [review]: I didn't have enough time to trace the code through properly, and since there weren't many comments I found it hard to work out what was going on. Could you add some more comments please? ::: tests/tracker/link-personas.vala @@ +65,3 @@ + this._persona_fullname_1 = "persona #1"; + this._persona_fullname_2 = "persona #2"; + this._tracker_backend.set_up (); Shouldn't this set_up() call be in the set_up() method? @@ +87,3 @@ + { + warning ("Couldn't set primary store: %s\n", e.message); + } Could do with a comment on this block of GConf code. Should it not also be in the set_up() method? @@ +95,3 @@ + this._main_loop.quit (); + assert_not_reached (); + }); A comment on this timeout would be useful too. @@ +102,3 @@ + assert (this._removed_individuals == 2); + + this._tracker_backend.tear_down (); Shouldn't this be in the tear_down() method? @@ +111,3 @@ + { + warning ("Couldn't unset primary store: %s\n", e.message); + } And this? @@ +142,3 @@ + } + + private async void _add_personas (PersonaStore pstore) A comment explaining at a reasonable level of detail what's going on/expected in this function would be useful. @@ +159,3 @@ + details1.insert ("full-name", (owned) v2); + + Double blank line. @@ +219,3 @@ + } + + private void _check_personas (Individual i) Comments!
Created attachment 184870 [details] [review] Setup our own GConf instance for tests Marked the check-* targets as .PHONY.
Created attachment 184871 [details] [review] Add support to switch primary backend Updated according to review.
Created attachment 184872 [details] [review] Test linking personas using Tracker as the primary backend Added some comments to clarify the approach taken by the test and made a few places of the code more robust. We were depending on an implementation detail: a linked individual _should_ contain the N linked personas + 1 linking persona. This might change in the near future so I got rid of that.
Review of attachment 184871 [details] [review]: folks/individual-aggregator.vala ================================ + private static const string _FOLKS_CONFIG_KEY = + "/apps/folks/backends/primary_store"; Replace "apps" with "system", since we aren't an application. Just make that fix and apply.
Review of attachment 184870 [details] [review]: Looks good - though since I wrote this patch, you should state here that you approve it before you apply it.
Review of attachment 184872 [details] [review]: This test is a little hard to follow, even with the comments (though I don't have any immediate suggestions, so I think it's good enough for now). + /* FIXME: we need a way to sync with Tracker + * delayed events. */ + Timeout.add_seconds (2, () => + { + this._aggregator.link_personas (this._personas); + return false; + }); I really wish we could fix this - please file a bug against Folks after you apply this patch, so we don't lose track of it.
(In reply to comment #17) > Review of attachment 184871 [details] [review]: > > folks/individual-aggregator.vala > ================================ > + private static const string _FOLKS_CONFIG_KEY = > + "/apps/folks/backends/primary_store"; > > Replace "apps" with "system", since we aren't an application. > > Just make that fix and apply. Done. (In reply to comment #18) > Review of attachment 184870 [details] [review]: > > Looks good - though since I wrote this patch, you should state here that you > approve it before you apply it. Reviewed & approved. (In reply to comment #19) > I really wish we could fix this - please file a bug against Folks after you > apply this patch, so we don't lose track of it. https://bugzilla.gnome.org/show_bug.cgi?id=646478 Merged all patches: commit 4379835b5144c45700c3fb2471910484cb338b14 Author: Raul Gutierrez Segales <raul.gutierrez.segales@collabora.co.uk> Date: Mon Mar 28 11:52:08 2011 +0100 Add test for linking personas using the Tracker backend tests/tracker/Makefile.am | 9 + tests/tracker/link-personas.vala | 320 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 329 insertions(+), 0 deletions(-) commit 359b1ad903341396d7a5532a6918377ffcc7af42 Author: Raul Gutierrez Segales <raul.gutierrez.segales@collabora.co.uk> Date: Mon Mar 28 11:51:43 2011 +0100 Implemented linkable_property_to_links in Trf.Persona backends/tracker/lib/trf-persona.vala | 27 ++++++++++++++++++++++++++- 1 files changed, 26 insertions(+), 1 deletions(-) commit 467672197a8225a4c9b91e541e80afe53fdbbfd4 Author: Raul Gutierrez Segales <raul.gutierrez.segales@collabora.co.uk> Date: Mon Mar 28 11:49:44 2011 +0100 Add support to switch primary backend either by GConf or an env var configure.ac | 2 + folks/Makefile.am | 3 ++ folks/individual-aggregator.vala | 58 ++++++++++++++++++++++++++++++++----- 3 files changed, 55 insertions(+), 8 deletions(-) commit bb6b6ff44a211c1df55b4a00a41564c49b9f89e8 Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Fri Jan 14 14:50:43 2011 -0800 Make tests use local GConf config and store. This avoids reading or writing the user's GConf store and lets us start from a consistent state when running tests (required for repeatability). configure.ac | 1 + tests/data/Makefile.am | 14 ++++++++++++ tests/data/gconf.path.in | 51 ++++++++++++++++++++++++++++++++++++++++++++++ tests/folks/Makefile.am | 1 + 4 files changed, 67 insertions(+), 0 deletions(-)