GNOME Bugzilla – Bug 682941
API to configure input of aggregation
Last modified: 2012-10-10 20:18:12 UTC
It would be good to have some API for choosing the data sources that a FolksIndividualAggregator instance aggregates. This includes choosing backends as well as individual databases that might be accessed via these backends. It should be possible to select the data sources before the initial folks_individual_aggregator_prepare() and later at runtime. In the case of the EDS backend it would be good to use the UUID of the source as identifier and not the source name, which is not guaranteed to be unique. Ideally, changes at runtime only cause minimal changes in the set of individuals, instead of rebuilding everything from scratch.
(In reply to comment #0) > It would be good to have some API for choosing the data sources that a > FolksIndividualAggregator instance aggregates. This includes choosing backends > as well as individual databases that might be accessed via these backends. Backends can already be enabled/disabled at runtime using BackendStore.[enable|disable]_backend(). I guess the logical API to add for PersonaStores would be: • Backend.enable_persona_store (string id); • Backend.disable_persona_store (string id); There would then need to be an IndividualAggregator constructor which took in an existing BackendStore: • IndividualAggregator.new_with_backend_store (BackendStore store); You could then instantiate your own BackendStore, enable/disable backends as you please, call BackendStore.prepare(), then BackendStore.load_backends(), then grab the Backend instances and call Backend.[enable|disable]_persona_store() on them as you please. IIRC, BackendStore might need some work to tidy up how it handles enabling/disabling backends. Disabling backends at runtime might not currently work. I’m not sure whether folks itself should store the enabled/disabled status of each PersonaStore. It feels like something which perhaps should be stored by the applications themselves. > It should be possible to select the data sources before the initial > folks_individual_aggregator_prepare() and later at runtime. That would be the case above. Once you’d finished calling Backend.[enable|disable]_persona_store() on the Backends, you could create your IndividualAggregator instance. > In the case of the EDS backend it would be good to use the UUID of the source > as identifier and not the source name, which is not guaranteed to be unique. That’s already the case, isn’t it? See line 237 of edsf-persona-store.vala. > Ideally, changes at runtime only cause minimal changes in the set of > individuals, instead of rebuilding everything from scratch. If a PersonaStore is disabled, it should emit the PersonaStore:personas-changed signal, marking all its Personas as removed. The IndividualAggregator will then re-link all Personas in the Individuals which contained the removed Personas. That’s the minimal set of Personas which have to be re-linked. I don’t think folks can do better here (apart from optimisations to the linking algorithm, which is currently quite slow). Note that, as discussed by e-mail, the EDS Backend should honour the ESource::enabled property.
(In reply to comment #1) > (In reply to comment #0) > I guess the logical API to add for PersonaStores would be: > • Backend.enable_persona_store (string id); > • Backend.disable_persona_store (string id); > > There would then need to be an IndividualAggregator constructor which took in > an existing BackendStore: > • IndividualAggregator.new_with_backend_store (BackendStore store); A list for this parameter would be better. For example, then one can combine one particular local store with the Telepathy store. > IIRC, BackendStore might need some work to tidy up how it handles > enabling/disabling backends. Disabling backends at runtime might not currently > work. s/BackendStore/IndividualAggregator/ here, right? The set of BackendStores would be fixed in the use cases that I have in mind, so the lack of support for disabling them is not an issue. What needs to be handled as well as possible are Backend.enable/disable_persona_store() calls at runtime. > I’m not sure whether folks itself should store the enabled/disabled status of > each PersonaStore. It feels like something which perhaps should be stored by > the applications themselves. Agreed. > > In the case of the EDS backend it would be good to use the UUID of the source > > as identifier and not the source name, which is not guaranteed to be unique. > > That’s already the case, isn’t it? See line 237 of edsf-persona-store.vala. The code which evaluates FOLKS_BACKEND_EDS_USE_ADDRESS_BOOKS expects names to be listed in that env variable: if (use_addressbooks.length > 0 && !(s.peek_name () in use_addressbooks)) { continue; } > Note that, as discussed by e-mail, the EDS Backend should honour the > ESource::enabled property. Right. We also discusses/agreed that an explicit enable_persona_store() would override an enabled=false property.
While thinking about the proposed API I came across one possible drawback. Consider that I have two local database A and B enabled. Now I want replace B with C. For that, I would call disable_persona_store(B) and enabled_persona_store(C). Would that trigger two recalculations of the unified address book? Probably. It would be much better to switch from "A + B" to "A + C" directly. If it cannot be implemented like that right away, then at least the API should allow such a implementation later on. Therefore it might be more natural to replace enable/disable_persona_store() with a single set_persona_store() which takes a GeeSet. NULL for the set is allowed and means "use default stores", whatever that means for the implementation. For EDS it would mean everything in FOLKS_BACKEND_EDS_USE_ADDRESS_BOOKS or (if that is unset) everything which has the "enabled" property set to true.
One BackendStore can have many backends, so a list of backendstores doesn't make much sense to me. However adding new_with_backend_store shouldn't be too tricky I'll take a look at that today. Adding set_persona_store which takes a GeeSet of persona stores shouldn't be too tricky either, and make sense for the reasons you stated.
(In reply to comment #4) > One BackendStore can have many backends, so a list of backendstores doesn't > make much sense to me. However adding new_with_backend_store shouldn't be too > tricky I'll take a look at that today. Right, I misunderstood the role of BackendStore. Please ignore my comment about making that a list.
Created attachment 224362 [details] [review] Add with_backend_store method to create IndividualAggregator With this patch most of what you want is doable. I checked the code in BackendStore and you should be able to do as Phillip said create a BackendStore, disable|enable _backend (backendname) to set up which backends you want, then call BackendStore.prepare() load_backends() and then pass the BackendStore into IndividualAggregator.with_backend_store (). load_backends will unload and unprepare any backends that you have disabled by calling disable_backend so once load_backends () is done the BackendStore should be the way you want it. I'll look into Backend.enable|disable_persona_store and set_persona_stores next.
I need the second half of the patch before I can try this out in practice.
Created attachment 224857 [details] [review] Add disable_persona_store and enable_persona_store to eds backend Untested, but it builds. This should remove a persona store that has already been loaded by calling disable_persona_store and should load a persona store that was previously disabled by calling enable_persona_store. set_persona_stores should be possible also, but I didn't get that far with this patch.
Review of attachment 224362 [details] [review]: ::: folks/individual-aggregator.vala @@ +289,3 @@ + { + this._backend_store = store; + } Shouldn’t you chain up to “Object()” here, and also move the “this._backend_store = BackendStore.dup ();” from the ‘construct’ block into the normal constructor? Also, this new constructor needs a documentation comment, not forgetting to add a “@since UNRELEASED” to it (and also list it in NEWS).
Review of attachment 224857 [details] [review]: It would be good if a test case could be added for this. That’ll help us to pin down the most appropriate behaviour for this API, if nothing else. Also, the new API needs to be mentioned in NEWS. ::: backends/eds/eds-backend.vala @@ +69,3 @@ + if (this._persona_stores.has_key (store.id)) + { + _remove_address_book (store); Missing ‘this.’. @@ +85,3 @@ + this.notify_property ("persona-stores"); + + this.persona_store_added (store); It would be good if this could share the code with ‘_add_address_book()’ rather than copy-pasting (e.g. add a new ‘_add_address_book_from_source()’ and move the PersonaStore construction call to that). ::: folks/backend.vala @@ +103,3 @@ + * @since UNRELEASED + */ + public abstract void enable_persona_store (PersonaStore store); The documentation for these should be expanded to specify things like: what happens to all the Personas in the added/removed stores; what signals are emitted regarding the Personas (and the PersonaStores) and when; etc.
(In reply to comment #8) > set_persona_stores should be possible also, but I didn't get that far with this > patch. I'd prefer to wait with testing until that's also available, because its the API that I need in SyncEvolution.
Created attachment 224965 [details] [review] Add with_backend_store method to create IndividualAggregator I think I got all your feedback into this patch. I'll add notes to NEWS as a separate commit when merging these into master.
Created attachment 224966 [details] [review] Add disable_persona_store and enable_persona_store to eds backend I think I got your feedback into this patch also. I'll add a unit test for this as a separate commit next.
Created attachment 224968 [details] [review] Add set_persona_stores to backend api Patrick, This set_persona_stores is implemented in terms of calling enable_persona_store and disable_persona_store on the stores passed in and the stores already loaded. Is that going to cause any issues with signals coming one at a time in that way? If so I could implement it to do the notifications all at once like we do in _ab_source_list_changed_cb otherwise this is very readable the way it is imo. Also, is it a problem to pass a Gee.Set of PersonaStore objects? It's very handy this way, but if it's tricky and a List would be simpler we could create a Set internal to the method instead.
Ok, I have a unit test for this, but it's currently failing, will fix it monday morning and get it up here for review.
(In reply to comment #14) > This set_persona_stores is implemented in terms of calling enable_persona_store > and disable_persona_store on the stores passed in and the stores already > loaded. Is that going to cause any issues with signals coming one at a time in > that way? The goal is to minimize churn in the individuals and thus change notifications. Does using enable/disable_persona_store achieve that, or will it work on one change at a time? > If so I could implement it to do the notifications all at once like > we do in _ab_source_list_changed_cb Without knowing much about the code it sounds like this is the better way to do it. > Also, is it a problem to pass a Gee.Set of PersonaStore objects? It's very > handy this way, but if it's tricky and a List would be simpler we could create > a Set internal to the method instead. Hmm, why PersonaStore? The original proposal was to use string IDs for the databases: • Backend.enable_persona_store (string id); • Backend.disable_persona_store (string id); That makes more sense than having to instantiate some kind of PersonaStore just to pass in the id. If a GeeSet is more natural for libfolks, then let's use that.
Review of attachment 224965 [details] [review]: ::: folks/individual-aggregator.vala @@ +288,3 @@ + + /** + * Create a new IndividualAggregator with a custom {@link BackendStore} This needs a full stop at the end. @@ +301,3 @@ + * agg.individuals_changed_detailed.connect (individuals_changed_cb); + * agg.prepare (); + * }}} I think it would be more sensible to hyperlink to the existing constructor’s documentation rather than copy-and-paste it.
Review of attachment 224966 [details] [review]: ::: backends/eds/eds-backend.vala @@ +83,3 @@ + * If the given persona store is not already in this backend, + * it will be added to the backend and persona-stores property notification + * will be sent, along with persona_store_added. s/sent/emitted/ persona-stores and persona-store-added should be hyperlinked. (e.g. “{@link Backend.persona_stores}” and “{@link Backend.persona_store_added}”.) However, I think it would make more sense for the documentation about signals to be on the method declaration in backend.vala, rather than in eds-backend.vala, since it should be applicable to all derived implementations of the method.
Review of attachment 224968 [details] [review]: ::: backends/eds/eds-backend.vala @@ +104,3 @@ + * + * If any persona stores given are not already part of + * this backend, they will be added and signals will be sent accordingly. “signals will be sent accordingly” isn’t very helpful! Which signals? Which order? As mentioned below, this should be documented in the method declaration in backend.vala, rather than in this derived version. @@ +114,3 @@ + foreach (PersonaStore store in stores) + { + this.enable_persona_store (store); Calling this multiple times will result in multiple calls to “this.notify_property ("persona-stores")”. You should either factor out that call so it’s only made once from this function (and once from enable_persona_store(), etc.); or surround the code in this function with a “this.freeze_notify()”/“this.thaw_notify()” pair. That should work, but it would be a bit uglier. @@ +123,3 @@ + this.disable_persona_store (store); + } + } I’m fairly sure this won’t work, because it modifies this._persona_stores while iterating over it. That will cause the Gee.Iterator to error out. The generally-used pattern for this kind of thing is to pre-process ‘stores’ and ‘this._persona_stores’ to produce two new sets: ‘enabled’ and ‘disabled’. The code should then iterate over those and actually call ‘this.[enable|disable]_persona_store()’. ::: folks/backend.vala @@ +110,3 @@ + * @since UNRELEASED + */ + public abstract void set_persona_stores (Set<PersonaStore> stores); As Patrick says, it probably makes more sense to use PersonaStore IDs rather than actual instances, since it then allows the API to be used without instantiating all the PersonaStores. There’s no need to also change [enable|disable]_persona_store(), though; I think they’re fine to continue using PersonaStore instances. Again, this documentation could be improved by listing the signals it will cause to be emitted and the order they’ll be emitted in. e.g. “This will cause {@link Backend.persona_store_removed} signals to be emitted for all removed stores, followed by {@link Backend.persona_store_added} signals for all added stores. As these signals are emitted, the sets of individuals in any associated {@link IndividualAggregator}s will be updated, and {@link IndividualAggregator.individuals_changed} may be emitted multiple times as appropriate. A property change notification for {@link Backend.persona_stores} will be emitted last.”. (Note that I don’t know how correct that sequence of signal emissions is, but it sounds about right to me.)
(In reply to comment #16) > The goal is to minimize churn in the individuals and thus change notifications. > Does using enable/disable_persona_store achieve that, or will it work on one > change at a time? Currently it’s not possible to minimise churn in the Individuals further than one emission of IndividualAggregator.individuals-changed per emission of Backend.persona_store_[enabled|disabled], since the IndividualAggregator handles those signals separately. If churn were to be minimised further, the IndividualAggregator would have to be changed to handle the property change notifications for Backend.persona_stores instead, then IndividualAggregator._backend_persona_store_[added|removed]_cb() could be merged and do some magic to merge calls to IndividualAggregator._personas_changed_cb() (which is the big method which handles aggregation of Personas to form Individuals). I hope that isn’t too unclear. > Hmm, why PersonaStore? The original proposal was to use string IDs for the > databases: > > • Backend.enable_persona_store (string id); > • Backend.disable_persona_store (string id); > > That makes more sense than having to instantiate some kind of PersonaStore just > to pass in the id. Agreed. > If a GeeSet is more natural for libfolks, then let's use that. A Set is the correct data structure here because mathematically we’re considering a set of things (i.e. there’s no order defined over them). folks is pedantic like that.
Created attachment 225100 [details] [review] Add with_backend_store method to create IndividualAggregator Updated to link to original ctor documetation and fixed the typo.
Created attachment 225102 [details] [review] Add disable_persona_store and enable_persona_store to eds backend Updated as per your feedback. Patrick, as Philip pointed out it should be fine to use PersonaStores to pass into enable_persona_store and disable_persona_store. If needed, you can get them from the Backend itself.
Created attachment 225103 [details] [review] Add set_persona_stores to backend api Changed set_persona_stores to use a Set of persona store id strings rather than stores themselves. Also fixed the eds implementation as per feedback given.
Created attachment 225104 [details] [review] Added a unit test for eds disable_persona_store Here's a new unit test for eds backend which tests disable_persona_store. I tried making it test enabling and disabling (disabling first, then enabling) but didn't get the notification somehow of the persona store getting added. I'll look into that next. Also, for some reason if I tested disabling the primary persona store ("test") in addition to the "other" one the test fails outright, with no assertion failure or anything. Either we don't handle removal of the primary eds persona store well, or something else is wrong.
Created attachment 225110 [details] [review] Added a unit test for eds disable_persona_store and enable_persona_store. With this patch enable and disable of the "other" persona store are working fine (though the test required a small tweak, removing the persona store id from our hashmap before emitting persona_store_removed signal. Removal and adding the primary persona store are still problematic though.
Review of attachment 225100 [details] [review]: Ready to commit with the change below. ::: folks/individual-aggregator.vala @@ +290,3 @@ + * Create a new IndividualAggregator with a custom {@link BackendStore}. + * + * {@link IndivdiualAggregator} Er, perhaps with some context like: “This behaves the same as the default constructor for {@link IndividualAggregator}, but uses the given {@link BackendStore} rather than the default one.”
Review of attachment 225102 [details] [review]: Looks good to go with the change below. ::: folks/backend.vala @@ +105,3 @@ + * + * If the given persona store is not already in this backend {@link Backend.persona_stores}, + * it will be added to the backend and persona-stores property notification Link the persona-stores property (“{@link Backend.persona_stores}”).
Review of attachment 225103 [details] [review]: ::: backends/eds/eds-backend.vala @@ +99,3 @@ + if (!this._persona_stores.has_key (id)) + { + E.Source s = this._ab_sources.ref_source (id); Is it possible for ref_source() to fail and return null? If so, that should be handled. @@ +104,3 @@ + store.removed.connect (this._store_removed_cb); + this._persona_stores.set (store.id, store); + this.persona_store_added (store); This lot could be factored out. @@ +110,3 @@ + } + + var stores_to_remove = new LinkedList<PersonaStore> (); I’d actually use a Vala array here for efficiency, since it would use O(log_2(n)) allocations (for n elements) rather than O(n). Not that it particularly matters. @@ +124,3 @@ + { + this._remove_address_book (store, false); + } It would be useful to put a comment somewhere about why the removals are performed in a separate loop (in order to avoid invalidating the loop iterator). ::: folks/backend.vala @@ +119,3 @@ + * signals will be emitted. + * If any persona stores already part of this backend are not + * given, they will be disabled and removed. What about the documentation I provided?
Review of attachment 225110 [details] [review]: Looks good to me apart from the minor issues below. Once the issue with the primary persona store has been debugged and fixed, this test case can be expanded to cover the primary store as well. ::: tests/eds/enable-disable-stores.vala @@ +1,2 @@ +/* + * Copyright (C) 2011 Collabora Ltd. Copyright’s out of date. @@ +118,3 @@ + catch (GLib.Error e) + { + GLib.warning ("Error when calling prepare: %s\n", e.message); Extraneous ‘\n’. @@ +162,3 @@ + // this._store_added[store] = true; + // this._main_loop.quit (); + // } Commented-out code can be removed.
Created attachment 225114 [details] [review] Add with_backend_store method to create IndividualAggregator
Created attachment 225115 [details] [review] Add disable_persona_store and enable_persona_store to eds backend The next question is how these methods should be implemented in the key-file and telepathy backends...
Created attachment 225116 [details] [review] Add set_persona_stores to backend api
Created attachment 225118 [details] [review] Added a unit test for eds disable_persona_store and enable_persona_store. Ok, I believe I've added all changes mentioned in previous comments, before this can hit master the methods need to be implemented in the other backends though. (And I need to add it to the NEWS file also).
(In reply to comment #33) > before > this can hit master the methods need to be implemented in the other backends > though. Wouldn't it be sufficient to document the new functions as "only implemented in some backends" and return proper errors in the backends which don't implement them? Actually, I think we forgot error reporting in the new API, did we? > (And I need to add it to the NEWS file also). Right.
Review of attachment 225114 [details] [review]: Looks good to me.
Review of attachment 225115 [details] [review]: Looks good to me.
Review of attachment 225116 [details] [review]: ::: backends/eds/eds-backend.vala @@ +112,3 @@ + if (s == null) + { + warning ("Unable to reference EDS source with id %s", id); s/id/ID/ @@ +126,3 @@ + /* Keep persona stores to remove in a separate array so we don't + * invalidate the list we are iterating over. */ + var stores_to_remove = new GLib.List<PersonaStore> (); GLib.List is actually effectively the same as Gee.LinkedList, except it’s less type safe and can easily cause refcounting bugs. I was suggesting using (e.g.) “PersonaStore[] stores_to_remove”. ::: folks/backend.vala @@ +124,3 @@ + * {@link Backend.persona_stores} will be emitted last. + * + * @since UNRELEASED The documentation also needs a “@param storeids” line to document the parameter.
Review of attachment 225118 [details] [review]: Looks good apart from the two minor comments below. ::: backends/eds/eds-backend.vala @@ +333,3 @@ debug ("Removing address book '%s'.", store.id); + this._persona_stores.unset (store.id); Is this the fix for primary persona stores? If so, it needs to be in a separate commit. ::: tests/eds/enable-disable-stores.vala @@ +120,3 @@ + catch (GLib.Error e) + { + GLib.warning ("Error when calling prepare: %s\n", e.message); Still an extraneous ‘\n’.
(In reply to comment #34) > (In reply to comment #33) > > before > > this can hit master the methods need to be implemented in the other backends > > though. > > Wouldn't it be sufficient to document the new functions as "only implemented in > some backends" and return proper errors in the backends which don't implement > them? Nope, since the new methods are ‘abstract’ rather than ‘virtual’. They need to be implemented everywhere. > Actually, I think we forgot error reporting in the new API, did we? I can’t think of a use for error reporting. Can you? Jeremy, one thing I’ve just realised I forgot from all the reviews of the new APIs is to mention that the documentation comments need “@param” lines for each of their parameters, and “@return” lines if they return a non-void value. Sorry for not mentioning this sooner.
(In reply to comment #39) > (In reply to comment #34) > > Actually, I think we forgot error reporting in the new API, did we? > > I can’t think of a use for error reporting. Can you? For example, "not implemented" :-) Another example might be "cannot disable primary store", if that cannot be allowed. I know that these are not very compelling reasons to add error report now. But if a good reasons comes up later, it'll be too late.
(In reply to comment #40) > (In reply to comment #39) > > (In reply to comment #34) > > > Actually, I think we forgot error reporting in the new API, did we? > > > > I can’t think of a use for error reporting. Can you? > > For example, "not implemented" :-) But it will be implemented for all backends! :-) > Another example might be "cannot disable primary store", if that cannot be > allowed. I can’t see any reason why that should not be allowed. Even if the underlying library folks is using doesn’t support it, folks can ignore all signals from the library and _pretend_ that the persona store is disabled. > I know that these are not very compelling reasons to add error report now. But > if a good reasons comes up later, it'll be too late. I understand, but I don’t think these APIs will ever need error reporting, and it complicates the (C) API to add it in unnecessarily.
Created attachment 225306 [details] [review] Add set_persona_stores to backend api Updated as per your feedback.
Created attachment 225307 [details] [review] Implement new api in keyfile backend This is an implementation of enable/disable_persona_store and set_persona_stores in keyfile backend.
Created attachment 225308 [details] [review] Add disable_persona_store and enable_persona_store to telepathy backend And finally, implementing enable/disable_persona_store in the telepathy backend. set_persona_stores will be next. If this looks good I'll do that, add to the NEWS file and then merge at last.
Review of attachment 225306 [details] [review]: Go for it.
Review of attachment 225307 [details] [review]: Looking good. Just a few small comments. ::: backends/key-file/kf-backend.vala @@ +85,3 @@ + if (this._persona_stores.has_key (store.id) == false) + { + this._add_store ((Kf.PersonaStore)store); Missing a space after the cast (“(Kf.PersonaStore) store”). @@ +120,3 @@ + string filename = id + ".ini"; + File file = File.new_for_path (Environment.get_user_data_dir ()); + file = file.get_child ("folks"); The code to get this directory could be factored out into a separate function, and then cached outside the loop so it’s not being called for every element of storeids. @@ +125,3 @@ + PersonaStore store = new Kf.PersonaStore (file); + this._add_store (store, false); + this.persona_store_added (store); Would be tidier if _add_store() unconditionally emitted persona_store_added (but only conditionally emitted the notify). @@ +141,3 @@ + { + this._remove_store ((Kf.PersonaStore)removed_stores[i], false); + this.persona_store_removed (removed_stores[i]); Same here. It would be tidier if persona_store_removed was unconditionally emitted in _remove_store(). @@ +294,3 @@ private void _store_removed_cb (Folks.PersonaStore store) { + this._remove_store ((Kf.PersonaStore)store); Missing a space after the cast (“(Kf.PersonaStore) store”).
Review of attachment 225308 [details] [review]: Looks good. ::: backends/telepathy/tp-backend.vala @@ +82,3 @@ + /** + * {@inheritDoc} + */ Something’s gone wrong with the whitespace here. @@ +211,3 @@ + { + this.notify_property ("persona-stores"); + this.persona_store_added (store); As with the key-file backend, it’s probably cleaner to emit persona_store_[added|removed] unconditionally in _[add|remove]_store().
Philip, Ok, I've pushed my branch 682941 here: http://cgit.collabora.com/git/user/jwhiting/folks.git/log/?h=682941 I believe I've fixed all the items you have pointed out previously including adding to the NEWS file. I'll merge this monday with you or travis ok.
I partly updated SyncEvolution to use the new API. Right now it does a folks_backend_set_persona_stores() with an empty set, because the code for choosing that set is missing. I was expecting my tests to fail, because with no data stores enabled, the unified address book should be empty. But the tests passed as before, indicating that disabling them failed. Should it be possible to disable all stores? Does it matter when the function is called? I called it after BackendStore.loadBackends() completed, and before IndividualAggregator.prepare().
The problem is that the effect folks_backend_set_persona_stores() is not permanent. My expectation was that it means "use this set of stores and nothing else, until told otherwise". In other words, I want full control and in particular, prevent pulling data from additional stores (regardless whether they were already created or will be created). But the way it is implemented only disables stores which are already known. If some store gets created later, that will be noticed and it'll be enabled automatically, even if not in the set (_ab_source_list_changed_cb and _add_address_book in eds-backend.vala). In my case, the EDS databases are static, but somehow _ab_source_list_changed_cb is triggered twice, once before folks_backend_set_persona_stores() and once after it. The second instance is the one which adds back and enables stores which I expected to be disabled for the tests. Can we make the effect of folks_backend_set_persona_stores()? I guess the EDS backend would have to remember the set and (if set) only add new sources if they are in the set. I proposed using a GeeSet (instead of a GList) in comment #3 because then a NULL GeeSet pointer in folks_backend_set_persona_stores() can be used to indicate "restore default selection", which cannot be done with GList because NULL is the empty GList. I'm fine with not implementing this special meaning of NULL for now - I don't need it and only mentioned it for the sake of completeness. What I do need is the "use this set, and I really mean it" functionality ;-}
(In reply to comment #50) > Can we make the effect of folks_backend_set_persona_stores()? ... permanent?
(In reply to comment #50) > Can we make the effect of folks_backend_set_persona_stores()? I guess the EDS > backend would have to remember the set and (if set) only add new sources if > they are in the set. Whoops, what an oversight. Yes, this needs doing before the branch can be merged.
Ok, force pushed to my branch with a change to the eds backend to store the HashSet of strings passed into set_persona_stores it is then used in the same way as the environment variable to not enable eds addressbooks that aren't in the set. Passing an empty set clears it and all addressbooks should work after that (though any loaded ones will get disabled, we should probably check for an empty set in the set_persona_stores method and handle that as a special case).
(In reply to comment #53) > Ok, force pushed to my branch with a change to the eds backend Where is that branch? I'd like to test this change. > to store the > HashSet of strings passed into set_persona_stores it is then used in the same > way as the environment variable to not enable eds addressbooks that aren't in > the set. That sounds right. > Passing an empty set clears it and all addressbooks should work after > that (though any loaded ones will get disabled, we should probably check for an > empty set in the set_persona_stores method and handle that as a special case). "will get disabled" is a bit unexpected. So either don't allow that special case or implement the necessary check to restore the default set of stores.
A few comments previous, the branch is here: http://cgit.collabora.com/git/user/jwhiting/folks.git/log/?h=682941 I'll add that special case to enable everything if an empty set is passed in.
Patrick, I've added a commit on the end of that branch that should fix the issue (makes it load any missing persona stores if the set given is empty). Let me know if it works for you and I'll merge this branch to master.
commit 016a3ec9ac9073a555147288348c0ee57a2ba5d2 Author: Jeremy Whiting <jpwhiting@kde.org> Date: Tue Oct 2 15:37:54 2012 -0600 eds: if set given to set_persona_stores is empty, load any missing stores. diff --git a/backends/eds/eds-backend.vala b/backends/eds/eds-backend.vala index 9d40e14..48926b4 100644 --- a/backends/eds/eds-backend.vala +++ b/backends/eds/eds-backend.vala @@ -103,6 +103,14 @@ public class Folks.Backends.Eds.Backend : Folks.Backend public override void set_persona_stores (Set<string> storeids) { this._storeids = storeids; + + /* If the set is empty, load all unloaded stores then return. */ + if (storeids.size == 0) + { + this._ab_source_list_changed_cb (); + return; + } + bool stores_changed = false; /* First handle adding any missing persona stores. */ foreach (string id in storeids) You test for "set is empty" instead of "no set was given". That's not what I proposed. I tried set_persona_stores() with an empty set, expecting it to not read from any store. Instead it read from all of them. The same problem exists here: + if (this._storeids.size > 0 && + !(uid in this._storeids)) + { + continue; + } Here's a fix that shows what I mean: diff --git a/backends/eds/eds-backend.vala b/backends/eds/eds-backend.vala index 48926b4..179a081 100644 --- a/backends/eds/eds-backend.vala +++ b/backends/eds/eds-backend.vala @@ -104,8 +104,8 @@ public class Folks.Backends.Eds.Backend : Folks.Backend { this._storeids = storeids; - /* If the set is empty, load all unloaded stores then return. */ - if (storeids.size == 0) + /* If the set is NULL, load all unloaded stores then return. */ + if (storeids == null) { this._ab_source_list_changed_cb (); return; @@ -170,7 +170,7 @@ public class Folks.Backends.Eds.Backend : Folks.Backend { this._persona_stores = new HashMap<string, PersonaStore> (); this._persona_stores_ro = this._persona_stores.read_only_view; - this._storeids = new HashSet<string> (); + this._storeids = null; } /** @@ -308,7 +308,7 @@ public class Folks.Backends.Eds.Backend : Folks.Backend continue; } - if (this._storeids.size > 0 && + if (this._storeids != null && !(uid in this._storeids)) { continue;
I've added a test which enables two address books and disables the system address book. Into each address book I import exactly the same contact: BEGIN:VCARD VERSION:3.0 FN:John Doe N:Doe;John END:VCARD My expectation is that this will be merged into one FolksIndividual. Is that expectation reasonable? What happens instead is that I get two individuals. I see a scary looking warning in the output: folks: Failed to find primary PersonaStore with type ID 'e ds' and ID 'system-address-book'. Individuals will not be linked properly and creating new links between Personas will not work. The configured primary PersonaStore's backend may not be installed. If you are unsure, check with your distribution. I understand that you cannot write linking information permanently without an enabled primary store. My expectation was that the automatic linking would happen nevertheless, in memory.
I kept the system-address-book enabled and still don't get the two "John Does" merged into one.
Ok, I force pushed to my branch changes so passing a null set makes it use any persona store, passing an empty set makes it use no persona stores. I'll add an implementation to the tp-backend and the libsocialweb backend then it should be ready to merge (Right after Philip finishes the 0.8.0 release).
(In reply to comment #59) > I kept the system-address-book enabled and still don't get the two "John Does" > merged into one. Because we can’t implicitly link on personas’ names, because many people share the same name. This is precisely the reason why folks requires the user to explicitly link those personas.
Merged the branch to master.