GNOME Bugzilla – Bug 628970
Folks API needs a way to determine whether a specific type of PersonaStore is available
Last modified: 2010-09-15 15:24:52 UTC
As it is now, there is no (good) way for the client to know if, eg, aggregator.add_persona_from_details(null, "telepathy", "/foo/bar", null) should fail because the store isn't available. I think the best option is to: * make IndividualAggregator.stores public (so they can be searched through) * add IndividualAggregator:store-changed to indicate when a store becomes available or unavailable This could pave the way for a cleaner alternative to IndividualAggregator.add_persona_from_details () -- eg, just deprecating that and having clients use the PersonaStore's add_persona_from_details().
(In reply to comment #0) > This could pave the way for a cleaner alternative to > IndividualAggregator.add_persona_from_details () -- eg, just deprecating that > and having clients use the PersonaStore's add_persona_from_details(). Both of the add_persona_from_details() functions should become void as well (though it might be worth the compatibility to just declare them as always returning NULL).
I think we should be careful about just adding more and more API to IndividualAggregator. Its function should be as an IndividualAggregator and not much more. I think we should consider tidying up and promoting BackendStore and friends a little more, and using them instead. Along those lines, I suggest that we might want to remove IndividualAggregator's private BackendStore, and have it take a BackendStore instance as a construction property. That would solve the problem that I've got at the moment in the folks command line client: I need to be able to list backends, but I can't get at IndividualAggregator's internal BackendStore, so I have to create my own (second) BackendStore instance. Ugly. I think it would also be beneficial in the case that we want to add API in future to blacklist/whitelist certain backends, or say that we only want the offline backends, or something similar. (In reply to comment #1) > (In reply to comment #0) > > This could pave the way for a cleaner alternative to > > IndividualAggregator.add_persona_from_details () -- eg, just deprecating that > > and having clients use the PersonaStore's add_persona_from_details(). > > Both of the add_persona_from_details() functions should become void as well > (though it might be worth the compatibility to just declare them as always > returning NULL). Agreed, but I'm not sure we should be caring about compatibility on that level at the moment. That function's used in one place in Empathy, and it would be trivial to stop using its return value.
Created attachment 169716 [details] [review] Notify when PersonaStores are available and prepared; expose IndividualAggregator.stores Squashed diff of branch: http://git.collabora.co.uk/?p=user/treitter/folks.git;a=shortlog;h=refs/heads/bgo628970-store-notification
As discussed on IRC, a better alternative would be to clean up BackendStore and make it a singleton. The client can then use the same (singleton) BackendStore instance as the IndividualAggregator, and can use methods on the BackendStore/Backends to query the PersonaStores.
Created attachment 169999 [details] [review] Expose the backends Completely redone patch that exposes the backends directly. Squashed diff of branch: http://git.collabora.co.uk/?p=user/treitter/folks.git;a=shortlog;h=refs/heads/bgo628970-expose-backends
Review of attachment 169999 [details] [review]: ::: folks/backend-store.vala @@ +41,3 @@ private GLib.List<ModuleFinalizeFunc> finalize_funcs = null; + private static weak BackendStore instance; + private static bool backends_loaded = false; This doesn't seem to be used at all. @@ +70,3 @@ + * may have emitted the signal before you access the @{link BackendStore}, so + * it's necessary to read this property in addition to connecting to the + * signal. I don't understand this. The client should have a BackendStore instance and be able to connect to the BackendStore.backend_available signal before calling BackendStore.load_backends(). @@ +72,3 @@ + * signal. + * + * @since 0.1.17 Since 0.1.18 now…or perhaps since 0.2.0? ::: folks/individual-aggregator.vala @@ +140,3 @@ + * function if you're using this from C), all the {@link Backend}s will have + * been prepared (though no {@link PersonaStore}s are guaranteed to be + * available yet). This last guarantee is new as of version 0.1.17. Version 0.1.18…or 0.2.0. Which Backends? The ones which haven't been blacklisted? I'm not sure we even want to mention this in the public docs here, since we're trying to separate Backend handling from the IndividualAggregator. @@ +153,3 @@ + /* ideally, we wouldn't add this backend until after it's been + * successfully prepared, but we need to prevent the backend from + * being prepared() multiple times (simultaneously) */ If we want to do this properly, the IndividualAggregator would need a second list of Backends: ones which are in the process of being prepared. Any access to the list of being-prepared or already-prepared Backends would be regulated by a lock block. Perhaps it would make more sense, however, to move the calls to Backend.prepare() into the BackendStore, so that IndividualAggregator (or any client code which uses BackendStore) doesn't have to worry about preparing the Backends it uses.
(Mass changing milestones; please search for this phrase to delete the bug spam.)
Created attachment 170293 [details] [review] Expose the backends (try 2) Squashed diff of branch: http://git.collabora.co.uk/?p=user/treitter/folks.git;a=shortlog;h=refs/heads/bgo628970-expose-backends2
(In reply to comment #8) > Created an attachment (id=170293) [details] [review] > Expose the backends (try 2) > > Squashed diff of branch: > > http://git.collabora.co.uk/?p=user/treitter/folks.git;a=shortlog;h=refs/heads/bgo628970-expose-backends2 Sorry, I realized I haven't addressed all the concerns above. No need to review this; I'll have a replacement branch.
Created attachment 170300 [details] [review] Expose the backends (try 3) Squashed diff of branch: http://git.collabora.co.uk/?p=user/treitter/folks.git;a=shortlog;h=refs/heads/bgo628970-expose-backends3
(In reply to comment #6) > Review of attachment 169999 [details] [review]: > > ::: folks/backend-store.vala > @@ +41,3 @@ > private GLib.List<ModuleFinalizeFunc> finalize_funcs = null; > + private static weak BackendStore instance; > + private static bool backends_loaded = false; > > This doesn't seem to be used at all. It was due to a mis-rebase, apparently. Cut in the new version. > @@ +70,3 @@ > + * may have emitted the signal before you access the @{link BackendStore}, > so > + * it's necessary to read this property in addition to connecting to the > + * signal. > > I don't understand this. The client should have a BackendStore instance and be > able to connect to the BackendStore.backend_available signal before calling > BackendStore.load_backends(). I was thinking in terms of the fact that the BackendStore is a singleton now, but I guess it would be odd for a client to use it in multiple places. So if they're going to do that, I think they'd understand that detail. No need to pollute the docs with it, so I've cut it. > @@ +72,3 @@ > + * signal. > + * > + * @since 0.1.17 > > Since 0.1.18 now…or perhaps since 0.2.0? Bumped to 0.2.0. > ::: folks/individual-aggregator.vala > @@ +140,3 @@ > + * function if you're using this from C), all the {@link Backend}s will have > + * been prepared (though no {@link PersonaStore}s are guaranteed to be > + * available yet). This last guarantee is new as of version 0.1.17. > > Version 0.1.18…or 0.2.0. Which Backends? The ones which haven't been > blacklisted? I'm not sure we even want to mention this in the public docs here, > since we're trying to separate Backend handling from the IndividualAggregator. Good point. I've made this a private comment. > @@ +153,3 @@ > + /* ideally, we wouldn't add this backend until after it's been > + * successfully prepared, but we need to prevent the backend from > + * being prepared() multiple times (simultaneously) */ > > If we want to do this properly, the IndividualAggregator would need a second > list of Backends: ones which are in the process of being prepared. Any access > to the list of being-prepared or already-prepared Backends would be regulated > by a lock block. > > Perhaps it would make more sense, however, to move the calls to > Backend.prepare() into the BackendStore, so that IndividualAggregator (or any > client code which uses BackendStore) doesn't have to worry about preparing the > Backends it uses. Yup - most of my changes to this branch are based around moving the Backend prep to the BackendStore. Now the BackendStore only ever exposes prepared backends.
Review of attachment 170300 [details] [review]: Looks good to me!
(In reply to comment #12) > Review of attachment 170300 [details] [review]: > > Looks good to me! Merged. Everything was buildable before rebasing (which is why I now realize I wasn't actually caught up with master after all), but of course master still isn't due to bug #629691. commit c6e5052b366db020357696f90176c814c82f70b7 Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Thu Sep 9 16:15:49 2010 -0700 Move Backend preparation to BackendStore. Fixes bgo#628970. An effect of this is that all backends will be prepared by the time IndividualAggregator.prepare() completes. folks/backend-store.vala | 38 +++++++++++++++++++++++++--------- folks/individual-aggregator.vala | 42 ++++++++++++++++++++++--------------- 2 files changed, 53 insertions(+), 27 deletions(-) commit 41c8d990629d1ef210a18e2d02ea56ba4a5cce17 Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Wed Sep 8 09:53:21 2010 -0700 Expose the BackendStore's Backends. Helps bgo#628970. folks/backend-store.vala | 32 ++++++++++++++++++++++++++++++++ 1 files changed, 32 insertions(+), 0 deletions(-) (existing commit f69f90d83b92371ea10bd536793da80a4655b662) (existing commit af444ebede64877359a28e91ad1991ac77856a28) commit a5acb1e9fd0a5635c4d8692c2f58f1ca0184bf91 Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Wed Sep 8 10:24:55 2010 -0700 Make BackendStore a singleton. Helps bgo#628970. NEWS | 11 +++++++++++ folks/backend-store.vala | 20 +++++++++++++++++++- folks/individual-aggregator.vala | 2 +- tools/import.vala | 2 +- 4 files changed, 32 insertions(+), 3 deletions(-)