Created attachment 169561 [details] [review]
Squashed diff of the 628868-pidgin-import branch

http://git.collabora.co.uk/?p=user/pwith/folks;a=shortlog;h=refs/heads/628868-pidgin-backend

This branch adds a "pidgin" backend. It's read-only and trusted, and at the moment will read ~/.purple/blist.xml (if present) and use the information in it to provide alias, group and linking information for contacts. Surprisingly, this works fine without any modifications to the IndividualAggregator.

Unlinking Individuals obviously doesn't work, because the backend is read-only, so the linking information can't (and shouldn't) be removed from blist.xml.

Before bug #628553 can be fixed, we need to figure out a decent API for copying the data between backends.

If we made the Pidgin backend read-write, we could use it as a second key file, which would solve the problem of not being able to unlink Individuals from the Pidgin backend, but would mean we're trampling all over Pidgin's data files, and we could lose data easily if the user decides that they don't need their ~/.purple directory any more. This is generally a Bad Idea.

We could make libfolks implicitly copy Personas from the Pidgin backend to the key file backend on load. This is also a Bad Idea, because it's getting dangerously close to the syncing behaviour which we're trying hard to avoid.

The only other solution is to have some kind of explicit copy operation in libfolks, which allows all the Personas from one PersonaStore to be copied to another PersonaStore. Something like the following:

  async void PersonaStore.copy_personas (PersonaStore destination_store) throws PersonaStoreError

However, for this to be useful in Empathy (I imagine it would be called from the import dialogue), there would need to be some way to get instances of all the right PersonaStores, which is something we were trying to avoid exposing.

Another option would be:

  async void IndividualAggregator.copy_personas (string source_persona_store_type, string source_persona_store_id, string destination_persona_store_type, string destination_persona_store_id) throws SomeKindOfError

This fits in more with the existing IndividualAggregator.add_persona_from_details() API.

The only other problem with this approach is that the Pidgin backend will continue to be loaded after the meta-contact information has been copied over to the key file backend, and so the linking information from ~/.purple/blist.xml will continue to be used, even if (for example) it's subsequently deleted from the key file. i.e. Individuals from the Pidgin backend still won't be able to be unlinked.

It would be a bit nasty of the Empathy import code to delete or rename blist.xml once the import is finished, since the user may wish to continue using Pidgin.

A hacky solution which uses the existing API would be for Empathy to set FOLKS_BACKEND_PIDGIN_PATH to /dev/null once the import is complete (and every time Empathy is started afterwards).

A better solution would be some sort of API to prevent a backend from being loaded, such as:

  void BackendStore.disable_backend (string backend_name)

I think we'd probably have to add that on the BackendStore, since it really doesn't fit on IndividualAggregator. This means tidying up the BackendStore API and exposing it properly, which doesn't sound fun.

Or maybe instead you could have a concept of a importer backend that only enabled explicitly? That said, I like the idea of being able to have the pidgin backend stay there instead of importing.. If its decisions can be overridden by the main writeable backend.

(In reply to comment #2)
> Before bug #628553 can be fixed, we need to figure out a decent API for copying
> the data between backends.
> 
> If we made the Pidgin backend read-write, we could use it as a second key file,
> which would solve the problem of not being able to unlink Individuals from the
> Pidgin backend, but would mean we're trampling all over Pidgin's data files,
> and we could lose data easily if the user decides that they don't need their
> ~/.purple directory any more. This is generally a Bad Idea.
> 
> We could make libfolks implicitly copy Personas from the Pidgin backend to the
> key file backend on load. This is also a Bad Idea, because it's getting
> dangerously close to the syncing behaviour which we're trying hard to avoid.

Agreed that bad ideas are bad. The Pidgin backend should be read-only in every sense of the word (including renaming).

> The only other solution is to have some kind of explicit copy operation in
> libfolks, which allows all the Personas from one PersonaStore to be copied to
> another PersonaStore. Something like the following:
> 
>   async void PersonaStore.copy_personas (PersonaStore destination_store) throws
> PersonaStoreError
> 
> However, for this to be useful in Empathy (I imagine it would be called from
> the import dialogue), there would need to be some way to get instances of all
> the right PersonaStores, which is something we were trying to avoid exposing.

We were trying to avoid it (hence the kind of ugly string-identifier API for IndividualAggregator.add_persona_from_details()), but I'm leaning toward it for a future API clean-up (see bug #628970), since we probably need to expose the PersonaStores after all.

Since I'm hoping to resolve bug #628970 very soon, I think we're best off using the API above. Though I think it'd be slightly more OO-friendly to have the argument be the source store.

> Another option would be:
> 
>   async void IndividualAggregator.copy_personas (string
> source_persona_store_type, string source_persona_store_id, string
> destination_persona_store_type, string destination_persona_store_id) throws
> SomeKindOfError
> 
> This fits in more with the existing
> IndividualAggregator.add_persona_from_details() API.

See above.

> The only other problem with this approach is that the Pidgin backend will
> continue to be loaded after the meta-contact information has been copied over
> to the key file backend, and so the linking information from
> ~/.purple/blist.xml will continue to be used, even if (for example) it's
> subsequently deleted from the key file. i.e. Individuals from the Pidgin
> backend still won't be able to be unlinked.

Right - let's do something along the lines of store disabling, as below.

> It would be a bit nasty of the Empathy import code to delete or rename
> blist.xml once the import is finished, since the user may wish to continue
> using Pidgin.

They might even be running Pidgin simultaneously. Let's not mangle their data.

> A hacky solution which uses the existing API would be for Empathy to set
> FOLKS_BACKEND_PIDGIN_PATH to /dev/null once the import is complete (and every
> time Empathy is started afterwards).
> 
> A better solution would be some sort of API to prevent a backend from being
> loaded, such as:
> 
>   void BackendStore.disable_backend (string backend_name)
> 
> I think we'd probably have to add that on the BackendStore, since it really
> doesn't fit on IndividualAggregator. This means tidying up the BackendStore API
> and exposing it properly, which doesn't sound fun.

It's not a /great/ fit on IndividualAggregator, but I don't think it's too bad. Maybe it's worth having some Settings or Conf class or something.

We could just add another keyfile along the lines of ~/.local/share/folks/state which would have a key listing the disabled backends.

Yeah, let's avoid needing to expose BackendStore if at all possible. I'm not sure I'd be willing to add this feature if it required that.

As discussed on IRC, since we're past the feature freeze, it's too late to do this in Empathy this cycle (since we would need to add some code in the import wizard to do the import).

Furthermore, I think we'd need to do some extra UI/behavior changes to the import wizard,

A better option is shipping a utility with libfolks that translates blist.xml and adds it to relationships.ini. This would give our users something to use for migration immediately, so they don't have to manually re-add all meta-contacts.

It's still worth doing this backend, since we'll have late migrants.

After we add this, let's be sure to add a "Pidgin Backend" component to our bugzilla module.

(Mass changing milestones; please search for this phrase to delete the bug spam.)

With a little work (to support multiple "linking" backends, with all but the key-file backend read-only), I think we should be able to add this backend without needing any changes in Empathy to take advantage of it.

But instead of working in terms of "importing", this would just work "live" on the blist.xml (ideally also handling Pidgin writing to it - it's probably just a file replacement, as we do with relationships.ini).

As Rob pointed out, it'd be nicer to have Folks completely avoid importing. We might even want to split out folks-import into its own repository to discourage reliance upon it.

(In reply to comment #8)
> With a little work (to support multiple "linking" backends, with all but the
> key-file backend read-only), I think we should be able to add this backend
> without needing any changes in Empathy to take advantage of it.
> 
> But instead of working in terms of "importing", this would just work "live" on
> the blist.xml (ideally also handling Pidgin writing to it - it's probably just
> a file replacement, as we do with relationships.ini).

This sounds like you're suggesting we have the Pidgin backend be writeable, which we *cannot do* because blist.xml is not our file. It's not libfolks' place to start deleting meta-contacts from files which aren't its own.

(In reply to comment #9)
> (In reply to comment #8)
> > With a little work (to support multiple "linking" backends, with all but the
> > key-file backend read-only), I think we should be able to add this backend
> > without needing any changes in Empathy to take advantage of it.
> > 
> > But instead of working in terms of "importing", this would just work "live" on
> > the blist.xml (ideally also handling Pidgin writing to it - it's probably just
> > a file replacement, as we do with relationships.ini).
> 
> This sounds like you're suggesting we have the Pidgin backend be writeable,
> which we *cannot do* because blist.xml is not our file. It's not libfolks'
> place to start deleting meta-contacts from files which aren't its own.

Sorry, my wording was a little confusing. I meant we should handle the case that Pidgin does write to the blist.xml and update our Personas in the Pidgin backend accordingly. We'd certainly only be reading from blist.xml, not writing it (even indirectly).

(In reply to comment #8)

> As Rob pointed out, it'd be nicer to have Folks completely avoid importing. We
> might even want to split out folks-import into its own repository to discourage
> reliance upon it.

This will depend upon bug #629537, so users can correct any unwanted links they carry over from Pidgin. Of course, it'd be better for them to correct the problem at the source, but as reiterated in comment #9 and comment #10, we don't want to write to blist.xml (even indirectly).

And furthermore, for other backends we'll add in the future, we may have no write access anyhow (read: web services).

*** Bug 628553 has been marked as a duplicate of this bug. ***

Bumping these to the next release.

Punting to 0.3.7.

Punting bugs that won't be fixed by Folks 0.6.0.

Comment on attachment 169561 [details] [review]
Squashed diff of the 628868-pidgin-import branch

Marking patch as needs-work, because it’s quite old.

Created attachment 259088 [details] [review]
Add a read-only Pidgin backend (updated)

Here’s a dump of the most recent branch I have lying around for this. I’m going to close the bug as WONTFIX, because even if we spruced up this patch and tested it all over again, nobody is ever going to maintain it.

If someone comes along who wants to maintain a Pidgin backend for folks, they’re welcome to resurrect this patch.