GNOME Bugzilla – Bug 653777
Would be nice to have a helper function to create a writable persona
Last modified: 2011-08-31 08:36:26 UTC
Its often the case that when you edit an individual you have to start with creating a persona on the primary store as there might not a writable persona (e.g. for pure telepathy contacts). It would be nice to have a helper function like ..._ensure_primary_persona () that creates such a persona in the right place if necessary and then returns it.
The following branch: http://cgit.collabora.com/git/user/rgs/folks/log/?h=bgo-653777 makes sure that we can link personas even though there is a single Persona in the linking set and returns the created persona from link_persona(). With that, _ensure_primary_persona now looks like this (in Gnome Contacts): http://cgit.collabora.com/git/user/rgs/gnome-contacts/commit/?h=editing-readonly-personas&id=1f0d89e1cdc981ba0febea624ca10a05dc8a9922 As suggested by Alex, this might belong in the Folks.Individual (though you need an IndividualAggregator instance..).
I don't feel happy about overloading the semantics of linking like this. As it stands, linking magically produces a new Individual out of several others, and the client application doesn't have to know that it was achieved by creating a new Persona in a writeable store. I was imagining an API more like the following to fix this bug: class Individual { public async Persona ensure_property_writeable (string property_name) throws SomeKindOfError; } You'd call this basically whenever ensure_writable_persona() is called in gnome-contacts, with the addition that you pass in a property name. This allows us to control writeability at the property level, and prevents extraneous Personas from being created if the Individual already contains a Persona which is writeable for the given property. The Persona which is returned could be a new one or could be an existing one. Obviously, SomeKindOfError would have to deal with situations where it's just not possible to make the given property writeable (e.g. only the key-file backend is available as the user doesn't use eds, and they try to write a local ID, or some other property which the key-file backend doesn't support).
Well, but you still have to call link_personas() and you'd end up doing essentially the same thing.
(In reply to comment #3) > Well, but you still have to call link_personas() and you'd end up doing > essentially the same thing. ensure_property_writeable() could make sure the new Persona was linked to the Individual (if a new Persona was created). I think it's important to have the property name as an input, though you know the gnome-contacts code better than I do. What approach would be best to fit in with how gnome-contacts works?
(In reply to comment #4) > (In reply to comment #3) > > Well, but you still have to call link_personas() and you'd end up doing > > essentially the same thing. > > ensure_property_writeable() could make sure the new Persona was linked to the > Individual (if a new Persona was created). I think it's important to have the > property name as an input, Maybe we could revive this idea: https://bugzilla.gnome.org/show_bug.cgi?id=653543 (but with the needed changes, of course). > though you know the gnome-contacts code better than > I do. What approach would be best to fit in with how gnome-contacts works? I am not sure, need to play with it a little more and also I need some feedback from Alex. But yeah, I agree with the dangers of abusing link_personas(), though I want to maintain this as simple as possible.
(In reply to comment #4) > (In reply to comment #3) > > Well, but you still have to call link_personas() and you'd end up doing > > essentially the same thing. > > ensure_property_writeable() could make sure the new Persona was linked to the > Individual (if a new Persona was created). I think it's important to have the > property name as an input, though you know the gnome-contacts code better than > I do. What approach would be best to fit in with how gnome-contacts works? This approach sounds good to me.
Created attachment 193633 [details] [review] Add IndividualAggregator.ensure_individual_property_writeable() https://www.gitorious.org/folks/folks/commits/653777-ensure-property-writeable This is complete and works. I'd appreciate it if anybody reviewing it could think (in particular) about the API and if it could be made nicer. Putting the method in the IndividualAggregator seems a little iffy to me, but it has to have access to an IndividualAggregator instance so that it can link the personas. Putting it in the Individual class and having it take an IndividualAggregator parameter would be another option, but then we'd have to change the error handling (as IndividualAggregatorError.PROPERTY_NOT_WRITEABLE would no longer be appropriate).
Review of attachment 193633 [details] [review]: Generally looks good - just some minor issues: Please column-wrap the commit messages a little tighter, so they all fit nicely in a regular termal window. +++ b/backends/telepathy/lib/tpf-persona-store.vala @@ -61,6 +61,13 @@ public class Tpf.PersonaStore : Folks.PersonaStore ContactFeature.PRESENCE }; + private const string[] _always_writeable_properties = + { + "alias", + "is-favourite", + "groups" + }; I don't think "alias" and "groups" should be in here. It's possible that the server doesn't allow one or both (see Tpf.PersonaStore.can_alias_personas and .can_group_personas). +++ b/tests/folks/aggregation.vala ... + var individuals_changed_id = Trailing whitespace
Comment on attachment 193633 [details] [review] Add IndividualAggregator.ensure_individual_property_writeable() >--- a/NEWS >+++ b/NEWS >@@ -49,6 +49,9 @@ API changes: > * Add ObjectCache class > * Remove leaked internal PotentialMatch.result_to_string() method > * Add RoleDetails:role property >+* Add PersonaStore:always-writeable-properties property >+* Add IndividualAggregatorError.PROPERTY_NOT_WRITEABLE error >+* Add IndividualAggregator.ensure_individual_property_writeable() > It'll probably jump when you rebase, but this needs to move upwards to latest set of changes. >diff --git a/folks/individual-aggregator.vala b/folks/individual-aggregator.vala >index 39151dc..259815d 100644 >--- a/folks/individual-aggregator.vala >+++ b/folks/individual-aggregator.vala >+ /* Link the persona to the existing individual */ >+ var new_personas = new HashSet<Persona> (); >+ new_personas.add (new_persona); >+ >+ foreach (var p2 in individual.personas) >+ { >+ new_personas.add (p2); >+ } >+ >+ debug (" Linking personas to ensure %s property is writeable.", >+ property_name); >+ yield this.link_personas (new_personas); >+ >+ return new_persona; >+ } I'd name `new_personas` something like `linked_personas`; since not all the Personas in the Set are new and since you are grouping them to perform linking.
Comment on attachment 193633 [details] [review] Add IndividualAggregator.ensure_individual_property_writeable() Pushed with the above changes, though I just realised I forgot to re-wrap the commit messages. Sigh. commit a44e2df4d696b2ceb1740e83196fd1943d60664f Author: Philip Withnall <philip@tecnocode.co.uk> Date: Tue Aug 9 18:23:12 2011 +0200 Bug 653777 — Add a helper function to create a writable persona This adds IndividualAggregator.ensure_individual_property_writeable(), which returns a persona which has the specified property writeable in the specified individual. If no such persona exists already, a new one is created and linked to the individual. If that all fails, an error is returned. This allows for clients to write to properties of personas with confidence that the updates will be written out to the backend stores. (Achieved by calling IndividualAggregator.ensure_individual_property_writeable() first and only writing to the property if that call succeeds.) Closes: bgo#653777 NEWS | 4 + folks/individual-aggregator.vala | 130 ++++++++++++ tests/folks/aggregation.vala | 409 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 543 insertions(+), 0 deletions(-) commit 659d0c0b1dfb7123ee14c9cefe53696dd4dfb951 Author: Philip Withnall <philip@tecnocode.co.uk> Date: Thu Aug 11 13:57:25 2011 +0200 key-file: Allow empty Kf.Personas to be added to the persona store This is necessary for the tests for bgo#653777. It removes the error which would previously be thrown when trying to add a persona to Kf.PersonaStore with no properties. Helps: bgo#653777 backends/key-file/kf-persona-store.vala | 18 ++---------------- 1 files changed, 2 insertions(+), 16 deletions(-) commit 9ce804c7b3f712eb43121c1ef9cf17e0cf75bcc8 Author: Philip Withnall <philip@tecnocode.co.uk> Date: Thu Aug 11 13:55:10 2011 +0200 core: Add PersonaStore:always-writeable-properties This complements Persona:writeable-properties, listing the properties which are guaranteed to always be writeable on the personas in a given persona store. This is in contrast to Persona:writeable-properties, which may list extra properties which are only writeable on that particular persona. This could be the case with, for example, personas representing the user, which might have extra writeable properties to allow the user to change their alias or avatar. Helps: bgo#653777 NEWS | 3 ++ backends/eds/lib/edsf-persona-store.vala | 26 ++++++++++++++++++++ backends/key-file/kf-persona-store.vala | 17 +++++++++++++ backends/libsocialweb/lib/swf-persona-store.vala | 17 +++++++++++++ backends/telepathy/lib/tpf-persona-store.vala | 15 +++++++++++ backends/tracker/lib/trf-persona-store.vala | 28 ++++++++++++++++++++++ folks/persona-store.vala | 19 +++++++++++++++ 7 files changed, 125 insertions(+), 0 deletions(-)
While this now exists, it uses link_personas, so due to bug 653728 it will create two personas on the writable store if there wasn't one already.