GNOME Bugzilla – Bug 658324
Deprecate PersonaStore.is-writeable
Last modified: 2011-09-14 20:45:12 UTC
From my e-mail of 2011-09-03: • PersonaStore.is-writeable should be deprecated; use Persona.writeable-properties and PersonaStore.can-add-personas (etc.) to see what can be done with a store and its personas. Ignoring the fact that we were conflating “writeable” with “primary”, and for a moment just considering writeability in terms of whether the store is read-only, the scenarios suggest that treating read-only-ness at such a coarse-grained level doesn't work. In the scenarios where we have a Telepathy store, we want to write to some properties of its personas (alias and group), but not others (im-addresses, for example). This seems to be better handled using Persona.writeable-properties.
Additionally, and relatedly, we should add PersonaStore.is-primary-store: • Add PersonaStore.is-primary-store which is set by the IA to match IndividualAggregator.primary-store (much as PersonaStore.is-writeable was, but it has a better name). This allows code which doesn't have access to an IA (such as the _update_*() methods in Individual) to see whether a persona belongs to the primary store.
Created attachment 196425 [details] [review] Patch Branch is here: http://cgit.collabora.com/git/user/rgs/folks/log/?h=bgo-658324
Review of attachment 196425 [details] [review]: A few small changes are needed, I think. ::: NEWS @@ +10,3 @@ to configure the primary store +* Add Folks.PersonaStore.is_primary_store to phase out PersonaStore.is_writeable +* Add Folks.IndividualAggregatorError.NO_PRIMARY_STORE Might be better to explicitly state that PersonaStore.is-writeable is deprecated. ::: folks/backend-store.vala @@ -220,3 @@ "ID", persona_store.id, "Prepared?", persona_store.is_prepared ? "yes" : "no", - "Writeable?", persona_store.is_writeable ? "yes" : "no", Can't hurt to change this to “Is primary store?” rather than deleting it entirely. ::: folks/individual-aggregator.vala @@ +62,3 @@ + * @since UNRELEASED + */ + NO_PRIMARY_STORE, IndividualAggregatorError.NO_WRITEABLE_STORE should be deprecated. @@ -641,3 @@ this._persona_store_is_quiescent_changed_cb); store.notify["trust-level"].disconnect (this._trust_level_changed_cb); - store.notify["is-writeable"].disconnect (this._is_writeable_changed_cb); What about disconnecting from the is-primary-store change notification? ::: folks/individual.vala @@ +1013,2 @@ /* TODO: We (incorrectly?) assume that writeable == primary here. * This will be fixed by bgo#657141. */ This TODO can be removed. ::: folks/persona-store.vala @@ +645,3 @@ + * @since UNRELEASED + */ + public bool is_primary_store { get; set; default = false; } Would it be possible to make the setter private or internal, so that it can only be called from the IndividualAggregator? That would make the API a bit tidier and would mean that clients couldn't accidentally set the property and wonder why their application explodes due to an assertion failure in IndividualAggregator. Note that the C code should be checked when changing the visibility of the setter, since Vala likes to silently promote internal and private setters to public under certain circumstances.
(In reply to comment #3) > Review of attachment 196425 [details] [review]: > ::: folks/backend-store.vala > @@ -220,3 @@ > "ID", persona_store.id, > "Prepared?", persona_store.is_prepared ? "yes" : "no", > - "Writeable?", persona_store.is_writeable ? "yes" : "no", > > Can't hurt to change this to “Is primary store?” rather than deleting it > entirely. I wholeheartedly agree with not deleting public interfaces, but for debugging or internal code as the above I think we should merciless remove deprecated (and very confusing) stuff. > ::: folks/individual-aggregator.vala > @@ +62,3 @@ > + * @since UNRELEASED > + */ > + NO_PRIMARY_STORE, > > IndividualAggregatorError.NO_WRITEABLE_STORE should be deprecated. I think we still want to use it in ensure_indeividual_property_writeable (). Otherwise we might end up coupling primary and writeable stores in the future. > @@ -641,3 @@ > this._persona_store_is_quiescent_changed_cb); > store.notify["trust-level"].disconnect (this._trust_level_changed_cb); > - store.notify["is-writeable"].disconnect (this._is_writeable_changed_cb); > > What about disconnecting from the is-primary-store change notification? Good catch. > ::: folks/individual.vala > @@ +1013,2 @@ > /* TODO: We (incorrectly?) assume that writeable == primary here. > * This will be fixed by bgo#657141. */ > > This TODO can be removed. Done. > ::: folks/persona-store.vala > @@ +645,3 @@ > + * @since UNRELEASED > + */ > + public bool is_primary_store { get; set; default = false; } > > Would it be possible to make the setter private or internal, so that it can > only be called from the IndividualAggregator? That would make the API a bit > tidier and would mean that clients couldn't accidentally set the property and > wonder why their application explodes due to an assertion failure in > IndividualAggregator. > > Note that the C code should be checked when changing the visibility of the > setter, since Vala likes to silently promote internal and private setters to > public under certain circumstances. Made it internal, since the IndividualAggregator needs to access this property.
Created attachment 196482 [details] [review] Updated patch.
(In reply to comment #4) > (In reply to comment #3) > > Review of attachment 196425 [details] [review] [details]: > > ::: folks/backend-store.vala > > @@ -220,3 @@ > > "ID", persona_store.id, > > "Prepared?", persona_store.is_prepared ? "yes" : "no", > > - "Writeable?", persona_store.is_writeable ? "yes" : "no", > > > > Can't hurt to change this to “Is primary store?” rather than deleting it > > entirely. > > I wholeheartedly agree with not deleting public interfaces, but for debugging > or internal code as the above I think we should merciless remove deprecated > (and very confusing) stuff. Of course, but I meant to replace the “Writeable?” line with: "Is primary store?", persona_store.is_primary_store ? "yes" : "no", > > ::: folks/individual-aggregator.vala > > @@ +62,3 @@ > > + * @since UNRELEASED > > + */ > > + NO_PRIMARY_STORE, > > > > IndividualAggregatorError.NO_WRITEABLE_STORE should be deprecated. > > I think we still want to use it in ensure_indeividual_property_writeable (). > Otherwise we might end up coupling primary and writeable stores in the future. The only use I can see of NO_WRITEABLE_STORE in ensure_individual_property_writeable() is after checking this._writeable_store. With this patch, there's no longer a concept of “writeable” stores: there is the primary store; and stores have writeable properties. ensure_individual_property_writeable() already uses IndividualAggregatorError.PROPERTY_NOT_WRITEABLE for what you're meaning, I think.
Review of attachment 196482 [details] [review]: Please commit after fixing the issues in comment #6. :-)
Merged: commit 96539acdb6c96af478c27c522b951677f0f25bc3 Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Tue Sep 13 17:48:17 2011 +0100 Folks.Individual: use is-primary-store instead of is-writeable Helps: https://bugzilla.gnome.org/show_bug.cgi?id=658324 commit adc4c5327afa80a9ad4255e85513edf609735fd6 Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Tue Sep 13 17:35:07 2011 +0100 IndividualAggregator: s/writeable_store/primary_store/g Renamed _writeable_store to _primary_store to augument clarity. Also, stop listening and setting is-writeable properties and work with is-primary-store properties. Helps: https://bugzilla.gnome.org/show_bug.cgi?id=658324 commit e857f5e159877aa47193f6dacb89367b7ac5aaf8 Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Tue Sep 13 17:33:44 2011 +0100 Folks.PersonaStore: add is-primary-store and deprecate is-writeable Helps: https://bugzilla.gnome.org/show_bug.cgi?id=658324 commit 25e75ed385d2733bd0b6f7390dbd288ec7c058dd Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Thu Sep 8 18:12:56 2011 +0100 core: use always-writeable instead of is-writeable Helps: https://bugzilla.gnome.org/show_bug.cgi?id=658324