GNOME Bugzilla – Bug 653728
linking APIs need work
Last modified: 2011-09-07 22:42:24 UTC
It seems like the way to link personas together is with IndividualAggregator.link_personas (Set<Persona> personas) This kinda works if the individual object is totally new. However, it doesn't really make sense if you already have an individual and you want to add a persona to it. What link_personas() does is create a completely new persona on the writable store with all the linking information. This kinda breaks if the individual already exists and has a persona on the writable store. It should really instead try to find an existing one and update it. So, if any of the personas in the passed in set is on the writable store that should be used (of course, then its problematic if two personas are on the writable store, which to use?). Additionally, it would probably closer to what the user sees if the way linking works is on the individual level. I.e. The user doesn't see separate personas in the ui, only individuals, and it is the individuals that gets selected for linking. unlink_individual seems kinda broken too, as it just finds the persona on the writable store and outright deletes it. Thats not right as that persona could contain information other than the linking data. Instead it should remove only the data that is needed for the linking, and only remove the persona if it ends up empty. And similarly to linking what really happens in the UI is that an existing individual gets a single persona unlinked, rather than a complete disconnection of all the personas in the individual, so the API isn't quite what you want. Additionally often you want to set a field on an individual (say a note on a telepathy-only individual), but it turns out there is no persona in the individual that is on a writable store. You then have to create such a persona and link it. It would be nice if this was easy to do in the API with some ensure_primary_store_persona() call. Of course, that implies some guarantees on what the primary store can support, but the current default (keyfile) doesn't e.g. let you store a note...
For reference, here is the commit where i'd like to use the ensure_primary_store_persona call: http://git.gnome.org/browse/gnome-contacts/commit/?id=de222ab40a367117e4399f0bcfb11396f4183caa I'd also note that the amount of code required just to edit notes is somewhat daunting.
(In reply to comment #0) > It seems like the way to link personas together is with > IndividualAggregator.link_personas (Set<Persona> personas) > > This kinda works if the individual object is totally new. However, it doesn't > really make sense if you already have an individual and you want to add a > persona to it. If you have an existing individual, my_individual, and want to link another persona, my_persona, to it, just do: aggregator.link_personas (set of (my_individual.personas + my_persona)) > What link_personas() does is create a completely new persona on the writable > store with all the linking information. This kinda breaks if the individual > already exists and has a persona on the writable store. It should really > instead try to find an existing one and update it. So, if any of the personas > in the passed in set is on the writable store that should be used (of course, > then its problematic if two personas are on the writable store, which to use?). That's getting dangerously close to syncing*, and Syncing Is Bad. The current behaviour is absolutely fine if all clients which are viewing the data are folks-powered, since they'll all aggregate the new persona together with the existing ones to form a single individual. It only falls down in the case that the data can also be viewed by a non-folks-powered client, such as the Google Contacts web interface. * Take the following scenario: we start with one persona from libsocialweb and one persona from Google Contacts which are initially not aggregated together. The user then manually links the personas. If we modified the writeable persona (i.e. the Google Contacts one) various bits of data from the libsocialweb persona would be copied in the Google Contacts one. If the person represented by this persona then changed their details on libsocialweb, we've ended up with an outdated copy of the details stored in Google Contacts. Bad. :-( So I guess if we want to solve the problem in the case of having a non-folks-powered viewer of the data (cf. Google Contacts' web interface), we need to ensure that we only copy immutable data to the writeable backend, such as IM addresses and web service addresses. If we were to do this on the basis of the linkable_properties, this requires that we tighten up what's allowed to be a linkable property to only things which are immutable identifiers for personas. > Additionally, it would probably closer to what the user sees if the way linking > works is on the individual level. I.e. The user doesn't see separate personas > in the ui, only individuals, and it is the individuals that gets selected for > linking. I don't see how that matters. What the programmer sees is often different to what the user sees. > unlink_individual seems kinda broken too, as it just finds the persona on the > writable store and outright deletes it. Thats not right as that persona could > contain information other than the linking data. Instead it should remove only > the data that is needed for the linking, and only remove the persona if it ends > up empty. Agreed. This design dates from the time when we could absolutely rely on the key-file backend being the only writeable one. We need to make sure that removing the linking data from the writeable persona doesn't result in data loss of any kind. i.e. We need to ensure that the data at least still exists on one of the personas after unlinking. One alternative which sidesteps this is to instead implement unlinking by adding an anti-link between the relevant personas (cf. bug #629537). That ensures that we'll never cause data loss. Unfortunately, it does mean that the unlinking only happens on the local machine. The individual will remain linked on other computers you have which use folks from the same PersonaStores. For that reason, if the first solution is possible I think it's preferable. > And similarly to linking what really happens in the UI is that an existing > individual gets a single persona unlinked, rather than a complete disconnection > of all the personas in the individual, so the API isn't quite what you want. > > Additionally often you want to set a field on an individual (say a note on a > telepathy-only individual), but it turns out there is no persona in the > individual that is on a writable store. You then have to create such a persona > and link it. It would be nice if this was easy to do in the API with some > ensure_primary_store_persona() call. Agreed, but this is a separate problem. Could you file a separate bug about it please? > Of course, that implies some guarantees on what the primary store can support, > but the current default (keyfile) doesn't e.g. let you store a note... I'm not sure we can do anything about that. We can be reasonably confident that e-d-s will be available (at least on GNOME), and I think it would be foolish to end up turning the key-file backend into a poor man's vCard.
> If you have an existing individual, my_individual, and want to link another > persona, my_persona, to it, just do: > > aggregator.link_personas (set of (my_individual.personas + my_persona)) While that technically works, for folks-only use it has several problems: 1) It causes weird entries for anything accessing the writable store without folks, like google contacts web ui, or your phone. 2) Even with a folks using UI this will be somewhat visible in the UI. Gnome-contacts editing UI will by necessity be showing all the sources when editing, and thus will show these internal personas to the user. 3) If you link together multiple personas via the UI this will generally happen one new persona at a time, so you'll end up with multiple link-only personas that contain increasingly more link info. This makes 1) worse and makes it even harder to unlink things as you may now have multiple linking personas. They can also get stale if you update one of them. Additionally, the way e.g. im addresses for linking is a general problem for the UI when editing. For instance, if the primary store persona has some im addresses set just because they're needed to match the im address of a linked telepathy persona, then when editing the primary store persona you'll see e.g. weird facebook ids that if you edit you'll break linking and stuff. IMHO, all this could be made much easier by not using the normal fields for linking personas together. If instead we had a field like im_address_for_linking in the primary store that was *only* used to match im_address in other personas then we'd get rid of much of these problems. For instance, its easy to never show this duplicated code in the UI, and its easy to update the right fields when linking/unlinking personas without running into any sync-like risks. Not sure how this would be best implemented. Can all eds backend store attributes outside of the standard set? Maybe we could use a custom TYPE or some other vcard extension attribute to mark the im_addresses used for linking only. > * Take the following scenario: we start with one persona from libsocialweb and > one persona from Google Contacts which are initially not aggregated together. > The user then manually links the personas. If we modified the writeable persona > (i.e. the Google Contacts one) various bits of data from the libsocialweb > persona would be copied in the Google Contacts one. If the person represented > by this persona then changed their details on libsocialweb, we've ended up with > an outdated copy of the details stored in Google Contacts. Bad. :-( I don't think this is a correct scenario though. The only data we copy from the socialweb persona should be immutable identifiers. Anything else is bad to use for linking. So, unless the persona on the social service is plain deleted we don't risk having outdated data copied. >> Additionally, it would probably closer to what the user sees if the way linking >> works is on the individual level. I.e. The user doesn't see separate personas >> in the ui, only individuals, and it is the individuals that gets selected for >> linking. > >I don't see how that matters. What the programmer sees is often different to >what the user sees. It certainly doesn't make the it impossible to implement what the user sees, but it makes it harder for each application that wants to implement this. I don't think anyone will ever want to link together a subset of the personas of some individual with another subset from another individual, so an api that let you specify a set of individuals to link together is easier to use, and as expressive.
> Agreed, but this is a separate problem. Could you file a separate bug about it > please? See bug 653777 for this
(In reply to comment #2) > (In reply to comment #0) > > It seems like the way to link personas together is with > > IndividualAggregator.link_personas (Set<Persona> personas) > > > > This kinda works if the individual object is totally new. However, it doesn't > > really make sense if you already have an individual and you want to add a > > persona to it. > > If you have an existing individual, my_individual, and want to link another > persona, my_persona, to it, just do: > > aggregator.link_personas (set of (my_individual.personas + my_persona)) > > > What link_personas() does is create a completely new persona on the writable > > store with all the linking information. This kinda breaks if the individual > > already exists and has a persona on the writable store. It should really > > instead try to find an existing one and update it. So, if any of the personas > > in the passed in set is on the writable store that should be used (of course, > > then its problematic if two personas are on the writable store, which to use?). > > That's getting dangerously close to syncing*, and Syncing Is Bad. The current > behaviour is absolutely fine if all clients which are viewing the data are > folks-powered, since they'll all aggregate the new persona together with the > existing ones to form a single individual. It only falls down in the case that > the data can also be viewed by a non-folks-powered client, such as the Google > Contacts web interface. I don't see a problem with this is the only data copied into the writable Persona is the linkable properties. That's the way our linking scheme is intended to work. All other things being equal, it would be nice to replace an existing Individual if possible. But we've already got the "replacement_individual" argument to the signal Individual.removed to handle this (and UIs already have to pay attention to that). So I think replacing an existing Individual would just be a minor optimization; nothing critical. And since we've got as-deterministic-as-possible Individual IDs now, we shouldn't have trouble with stored Individual IDs changing upon Individual replacement (unless it would have changed anyhow). > So I guess if we want to solve the problem in the case of having a > non-folks-powered viewer of the data (cf. Google Contacts' web interface), we > need to ensure that we only copy immutable data to the writeable backend, such > as IM addresses and web service addresses. If we were to do this on the basis > of the linkable_properties, this requires that we tighten up what's allowed to > be a linkable property to only things which are immutable identifiers for > personas. Is there any reason we wouldn't just copy the linkable properties, as discussed above? > > Additionally, it would probably closer to what the user sees if the way linking > > works is on the individual level. I.e. The user doesn't see separate personas > > in the ui, only individuals, and it is the individuals that gets selected for > > linking. > > I don't see how that matters. What the programmer sees is often different to > what the user sees. > > > unlink_individual seems kinda broken too, as it just finds the persona on the > > writable store and outright deletes it. Thats not right as that persona could > > contain information other than the linking data. Instead it should remove only > > the data that is needed for the linking, and only remove the persona if it ends > > up empty. > > Agreed. This design dates from the time when we could absolutely rely on the > key-file backend being the only writeable one. > > We need to make sure that removing the linking data from the writeable persona > doesn't result in data loss of any kind. i.e. We need to ensure that the data > at least still exists on one of the personas after unlinking. > One alternative which sidesteps this is to instead implement unlinking by > adding an anti-link between the relevant personas (cf. bug #629537). That > ensures that we'll never cause data loss. Unfortunately, it does mean that the > unlinking only happens on the local machine. The individual will remain linked > on other computers you have which use folks from the same PersonaStores. For > that reason, if the first solution is possible I think it's preferable. Isn't an anti-link a requirement here? Otherwise, the Aggregator could just re-link the Personas against the user's will at a later time (especially in the case that you're unlinking a perfect match or we get automated linking working).
I think the main issue remaining is that we should try to re-use existing Personas. Otherwise, we can populate EDS with a lot of redundant contacts (which Folks clients won't notice, but will be ugly for any EDS-only clients).
(In reply to comment #6) > I think the main issue remaining is that we should try to re-use existing > Personas. Otherwise, we can populate EDS with a lot of redundant contacts > (which Folks clients won't notice, but will be ugly for any EDS-only clients). Note that this is similar to the problem in bug #642251 and may be fixed at the same time.
Created attachment 193201 [details] [review] Don't create an Edsf.Persona if there already is one Proof of concept patch - need to follow up with a test case.
Review of attachment 193201 [details] [review]: This approach can't work. :-( ::: folks/individual-aggregator.vala @@ +1156,3 @@ var local_ids = new Gee.HashSet<string> (); + Edsf.Persona? writeable_persona = null; libfolks cannot depend on its backend libraries, so using Edsf.Persona in the aggregator isn't possible.
(In reply to comment #9) > Review of attachment 193201 [details] [review]: > > This approach can't work. :-( > > ::: folks/individual-aggregator.vala > @@ +1156,3 @@ > var local_ids = new Gee.HashSet<string> (); > > + Edsf.Persona? writeable_persona = null; > > libfolks cannot depend on its backend libraries, so using Edsf.Persona in the > aggregator isn't possible. Actually, it doesn't have to be an Edsf.Persona... it just needs to have those interfaces and I guess that is a safe assumption. Is it ok to check that the Persona belongs to the configured writable store? That should be enough...
(In reply to comment #10) > (In reply to comment #9) > > Review of attachment 193201 [details] [review] [details]: > > > > This approach can't work. :-( > > > > ::: folks/individual-aggregator.vala > > @@ +1156,3 @@ > > var local_ids = new Gee.HashSet<string> (); > > > > + Edsf.Persona? writeable_persona = null; > > > > libfolks cannot depend on its backend libraries, so using Edsf.Persona in the > > aggregator isn't possible. > > Actually, it doesn't have to be an Edsf.Persona... it just needs to have those > interfaces and I guess that is a safe assumption. By safe assumption, I meant that if we find a Persona that belongs to the writeable store, it'll have those interface (still - we'd check Persona is SomeDetails..).
(In reply to comment #10) > (In reply to comment #9) > > Review of attachment 193201 [details] [review] [details]: > > > > This approach can't work. :-( > > > > ::: folks/individual-aggregator.vala > > @@ +1156,3 @@ > > var local_ids = new Gee.HashSet<string> (); > > > > + Edsf.Persona? writeable_persona = null; > > > > libfolks cannot depend on its backend libraries, so using Edsf.Persona in the > > aggregator isn't possible. > > Actually, it doesn't have to be an Edsf.Persona... it just needs to have those > interfaces and I guess that is a safe assumption. > > Is it ok to check that the Persona belongs to the configured writable store? > That should be enough... Off the top of my head while sleepy, that should work. I think the proof would be in trying and testing it.
I've fixed most of the issues above in this branch: http://cgit.collabora.com/git/user/treitter/folks.git/log/?h=bgo653728-eds-fix-link-personas However, I hit the following crasher that I don't have time to debug, so I'm punting this to post-0.6.0: ======= When linking an Individual of {Edsf.Persona, 2 * Tpf.Persona} to an Individual with {Tpf.Persona}, I hit a crasher with trace:
+ Trace 228082
return_value=0x0, n_param_values=1, param_values=0x1281820, ...
(In reply to comment #13) > I've fixed most of the issues above in this branch: > > http://cgit.collabora.com/git/user/treitter/folks.git/log/?h=bgo653728-eds-fix-link-personas > > However, I hit the following crasher that I don't have time to debug, so I'm > punting this to post-0.6.0: It's due to a missing annotation in EDS. I'm working on it (and on finishing off the branch, since it currently doesn't re-aggregate the personas).
Created attachment 193990 [details] [review] Don't create a new Persona if one already exists https://www.gitorious.org/folks/folks/commits/653728-linking-reuse-personas This squashes Travis' fixes into Raúl's branch, rebases it against the current master and adds a fix of my own (comparing persona stores by pointer rather than ID and type). This works* and is ready to commit. *However, there's one big omission: the personas aren't actually linked if an existing persona is updated rather than a new one created. This is because IndividualAggregator._personas_changed_cb() is never called, and the aggregation machinery never gets a chance to run. The correct way, I think, to fix this is to listen to notifications of changes to linkable properties (on all personas) and re-link the personas as appropriate when those notifications are received. This is a feature which is needed anyway, and having spent some time playing around with other solutions, they all seem to come back to needing this feature anyway. I've opened bug #656689 about this.
(In reply to comment #15) > Created an attachment (id=193990) [details] [review] > Don't create a new Persona if one already exists > > https://www.gitorious.org/folks/folks/commits/653728-linking-reuse-personas > > This squashes Travis' fixes into Raúl's branch, rebases it against the current > master and adds a fix of my own (comparing persona stores by pointer rather > than ID and type). > > This works* and is ready to commit. Sorry, I should've been clearer. It's ready to commit *once bug #656689 is fixed*. Otherwise, we'll have effectively broken linking.
Btw. I just commited an early version of the linking dialog in gnome-contacts: http://git.gnome.org/browse/gnome-contacts/commit/?id=9ed5741b168f79955c7261fd0dd0c542613bcc65 It has two TODOs in it: + // TODO: Unlink persona p from contact.individual and: + // TODO: Link selected_contact.individual into contact.individual + // ensure we get the same individual so that the Contact is the same If you want to test the APIs, try to implement these TODOs with them.
(Setting target bugs for Folks 0.6.1. Search for this phrase to deal with bug mail spam.)
Didn't make this release; punting to 0.6.2.
(Punting bugs to 0.6.4)
Merged: commit dbd8d0f108379fd9941f48861c1a94c267b0386a Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Wed Aug 3 20:01:56 2011 +0100 Don't create a new Persona if there already is one When calling link_personas (), reuse an Persona if there is one among the set of Personas being linked. Fixes: https://bugzilla.gnome.org/show_bug.cgi?id=653728