GNOME Bugzilla – Bug 665376
Add API to get a TpfPersona from a TpContact
Last modified: 2011-12-13 18:44:11 UTC
I'm doing loads of refactoring in Empathy to reduce code duplication and so port most of the code to use EmpathyIndividual widget rather than their old EmpathyContact equivalent. To do so, I often need to get create an individual from a TpContact and use it as a wrapper. That means I can end up with different personas for the same TpContact which is unfortunate. I'd like to have something like: TpfPersona * ensure_tpf_persona (TpContact *contact) to avoid this duplication.
Btw, this blocks bug #665038 which the last piece of a big refactoring I've done in Empathy (getting rid of the empathy-contact-* flavor or most widgets) so it would be really really cool if this could be fixed for 3.4.
Yes, we should definitely add this look-up capability if it will help clean up a lot of your code (other clients might run into the same problems). Setting to high priority.
Created attachment 202915 [details] [review] Add API to get a Tpf.Persona from a Tp.Contact https://www.gitorious.org/folks/folks/commits/665376-tpf-persona-from-tp-contact Ta-da. Is the API what you had in mind, Guillaume?
Review of attachment 202915 [details] [review]: Ideally dup_for_contact() should create a new persona, if needed, so folks always know about all the personas and we can never end up with the same contact associated with 2 different personas. That means you'll have to keep a weak ref on the persona and remove it from the hash table when it's destroyed though.
(In reply to comment #4) > Review of attachment 202915 [details] [review]: > > Ideally dup_for_contact() should create a new persona, if needed, so folks > always know about all the personas and we can never end up with the same > contact associated with 2 different personas. When could Empathy know about a TpContact that folks doesn't? afaik, folks should already always know about all TpContacts (and thus their associated Tpf.Personas).
- When calling a phone number using a SIP account : the 'contact' isn't in our contact list (SIP doesn't have one actually) so Folks don't know about it. - When joining a XMPP muc : its participants use channel-specific handle so are a not in the contact list. That's just 2 examples I recently experienced but I'm sure there are more.
(In reply to comment #6) > - When calling a phone number using a SIP account : the 'contact' isn't in our > contact list (SIP doesn't have one actually) so Folks don't know about it. > - When joining a XMPP muc : its participants use channel-specific handle so are > a not in the contact list. > > That's just 2 examples I recently experienced but I'm sure there are more. I'll review once this is addressed.
(In reply to comment #6) > - When calling a phone number using a SIP account : the 'contact' isn't in our > contact list (SIP doesn't have one actually) so Folks don't know about it. > - When joining a XMPP muc : its participants use channel-specific handle so are > a not in the contact list. > > That's just 2 examples I recently experienced but I'm sure there are more. Assuming a Tpf.PersonaStore exists for each of these use cases (which I guess it would, but it would be empty), it would theoretically be OK to add a new Tpf.Persona to the PersonaStore using dup_for_contact() as you suggest. However, how would the PersonaStore know when to remove the Tpf.Persona? We normally do this in response to the group-members-changed-detailed signal of the PersonaStore's TpChannel, but we obviously can't do that in these cases.
(In reply to comment #8) > However, how would the PersonaStore know when to remove the Tpf.Persona? We > normally do this in response to the group-members-changed-detailed signal of > the PersonaStore's TpChannel, but we obviously can't do that in these cases. dup_for_contact() returns a strong ref; the caller is responsible of dropping it when it's done with the persona. Internally the store keeps a weak ref on the persona created; so as soon as it's destroyed it removes it from its internal hash table. Of course that means that Folks has to keep its own strong ref on personas while it's in the contact list.
(In reply to comment #9) > (In reply to comment #8) > > However, how would the PersonaStore know when to remove the Tpf.Persona? We > > normally do this in response to the group-members-changed-detailed signal of > > the PersonaStore's TpChannel, but we obviously can't do that in these cases. > > dup_for_contact() returns a strong ref; the caller is responsible of dropping > it when it's done with the persona. Internally the store keeps a weak ref on > the persona created; so as soon as it's destroyed it removes it from its > internal hash table. Of course that means that Folks has to keep its own strong > ref on personas while it's in the contact list. That makes sense. I'm still a bit hesitant about allowing client code to push personas into a persona store though. How does Empathy get these SIP and MUC TpContacts, and would it perhaps be better if folks did that instead?
In the muc case we get them from the TpChannel (tp_channel_group_dup_members_contacts()) directly. When calling (or chating, or doing anything which a contact who is not in our contact list actually), Empathy requests the TpContact to the CM using tp_connection_get_contacts_by_id()). Folks can't really do it itself. In theory it could watch all the channels but that would be racy (Empathy may know about the channel before Folks) and, tbh, it's not really its job. The other case could maybe be solved by having some kind of "search for an individual having this phone number/email/whatever and create one if you don't find one" API. We may end up doing that on the long term but that's much more complex.
Created attachment 203090 [details] [review] Add API to get a Tpf.Persona from a Tp.Contact (updated) https://www.gitorious.org/folks/folks/commits/665376-tpf-persona-from-tp-contact OK, I'm convinced. Here's a patch which attempts to do that. Is that what you had in mind? I haven't tested it much (though, notably, the /telepathy and /folks tests still all pass); do you have a branch I could test it with?
Review of attachment 203090 [details] [review]: Yeah that's what I had in mind. I'll try porting my branch and see how it goes. You should make sure to always use this method when creating personas internally so we'll never end up with 2 personas mapping the same contact. ::: backends/telepathy/lib/tpf-persona.vala @@ +1076,3 @@ + * + * If found, a new reference to the persona will be returned. If not found, + * `null` will be returned. Doc should be updated.
Created attachment 203134 [details] [review] Add API to get a Tpf.Persona from a Tp.Contact (updated again) https://www.gitorious.org/folks/folks/commits/665376-tpf-persona-from-tp-contact-rebase1 Rebased on current master and docs updated.
Created attachment 203142 [details] [review] Add API to get a Tpf.Persona from a Tp.Contact (updated again) https://www.gitorious.org/folks/folks/commits/665376-tpf-persona-from-tp-contact-rebase1 After talking with Guillaume about it on IRC, it seems that we also need to be able to ensure that a Tpf.PersonaStore also exists for a given TpAccount (i.e. silently creating it if it doesn't). This is because empathy-chat doesn't have a BackendStore running. This patch is a fairly major update over the original which moves the map of PersonaStores out of Folks.Tp.Backend and into Tpf.PersonaStore statically. The behaviour of everything should be as before if a BackendStore exists. If a BackendStore doesn't exist, Tpf.PersonaStores should appear in Folks.Tp.Backend.persona_stores as they're created using Tpf.PersonaStore.dup_for_account(). They'll only be removed when their ‘removed’ signal is emitted, since Tpf.PersonaStore holds a strong ref. on them. (This is in contrast with the approach for Tpf.Persona, where only a weak ref. is held, because Persona doesn't have a ‘removed’ signal.) This seems to work with Guillaume's test branch from some light testing: http://cgit.collabora.com/git/user/cassidy/empathy/log/?h=ensure-665853
Review of attachment 203142 [details] [review]: In general, I get a slightly bad feeling about moving this code from Tp.Backend to Tpf.PersonaStore due to their intended class divisions. But I see the reasoning and the implementation seems good (beside the issues below). Also, I hit this test failure: /Aggregation/linkable properties:same store: Program received signal SIGABRT, Aborted. 0x00007ffff62d2405 in raise () from /lib/x86_64-linux-gnu/libc.so.6 (gdb) bt
+ Trace 229246
========== backends/telepathy/lib/tpf-persona-store.vala + message ("Adding PersonaStore %p ('%s') to map.", store, store.id); + ... + message ("Removing PersonaStore %p ('%s') from map.", store, store.id); These should both be debug(); we should only output warnings and errors by default (I'm sure our clients wouldn't appreciate the noise). + * forcefully keep stores alive. Note that this is a permanent allocation + * which is leaked (but only once). */ + private static HashMap<string /* Account object path */, PersonaStore> + _persona_stores_by_account = null; + private static Map<string, PersonaStore> _persona_stores_by_account_ro = null Is this even true? It seems that we lazily destroy as necessary in _remove_store_from_map() + if (this._weakly_referenced_personas != null) + { + foreach (var p in this._weakly_referenced_personas) + { + if (p.contact != null) + { + p.contact.weak_unref (this._contact_weak_notify_cb); + } + } Surely, this needs to be added here: } else { + this._weakly_referenced_personas = new HashSet<Persona> (); + } Otherwise, this never gets created (and the block above is never taken and we'll hit a 'self != null' assertion failure in _ignore_by_handle()). + /* If we hold a weak ref. on the persona's TpContact, release that. */ + if (this._weakly_referenced_personas.remove (persona) == true && + persona.contact != null) + { + persona.contact.weak_unref (this._contact_weak_notify_cb); + } + + private void _contact_weak_notify_cb (Object obj) + { + var c = obj as Contact; + this._ignore_by_handle (c.get_handle (), null, null, + GroupDetails.ChangeReason.NONE); + } It's a little odd that there's a cycle in _ignore_by_handle() -> _contact_weak_notify_cb() -> _ignore_by_handle() (though conditionals prevent it from being an infinite cycle). + * Get a map of all the {@link Tpf.PersonaStore}s currently known about. I'm fine with ending sentences with prepositions but it depends on what they're used for. (This one's extraneous.) + /* HACK: This should be static and take a PersonaStore as an argument. Vala + * doesn't seem to want to equate that to the signal's type, though, so we + * make it non-static and abuse `this`. */ Please file a Vala bug and mention it as a FIXME here.
Created attachment 203337 [details] [review] Add API to get a Tpf.Persona from a Tp.Contact (updated again) Comments to follow.
(In reply to comment #16) > Review of attachment 203142 [details] [review]: > > In general, I get a slightly bad feeling about moving this code from Tp.Backend > to Tpf.PersonaStore due to their intended class divisions. But I see the > reasoning and the implementation seems good (beside the issues below). Yeah, I get the same bad feeling; but I can't see any other way to implement this. Do you have any suggestions? > Also, I hit this test failure: That was a joy to debug. Turns out we weren't emitting the Account.Removed D-Bus signal from our mock AccountManager test framework, which was causing the Tpf.PersonaStores from the first aggregation test to never be destroyed (because they'd never been removed). > These should both be debug(); we should only output warnings and errors by > default (I'm sure our clients wouldn't appreciate the noise). Whoops, fixed. I typically change selected debug() calls to message() calls while testing, so that I don't have to filter through page upon page of debug() output to see the ones I care about. I then forget to change them back. :-( > Is this even true? It seems that we lazily destroy as necessary in > _remove_store_from_map() It was a left over comment from the first patch iteration. Fixed. > Otherwise, this never gets created (and the block above is never taken and > we'll hit a 'self != null' assertion failure in _ignore_by_handle()). Fixed. > It's a little odd that there's a cycle in _ignore_by_handle() -> > _contact_weak_notify_cb() -> _ignore_by_handle() (though conditionals prevent > it from being an infinite cycle). I don't particularly see a problem with it. Unchanged. > I'm fine with ending sentences with prepositions but it depends on what they're > used for. (This one's extraneous.) The English language is not what I am best with. > Please file a Vala bug and mention it as a FIXME here. Turns out it was me being stupid: the signal and callback types didn't match because one was using Folks.PersonaStore and the other using Tpf.PersonaStore. I was using ‘PersonaStore’ as one, but Vala thought it was the other. Fixed.
(In reply to comment #18) > Yeah, I get the same bad feeling; but I can't see any other way to implement > this. Do you have any suggestions? Nothing comes to mind. I think we should follow through with this, but I just thought I'd state my concern. > > Also, I hit this test failure: > > That was a joy to debug. Turns out we weren't emitting the Account.Removed > D-Bus signal from our mock AccountManager test framework, which was causing the > Tpf.PersonaStores from the first aggregation test to never be destroyed > (because they'd never been removed). The TP test backend is pretty bare-bones. So almost anything new we do requires implementation there. We hadn't hit it until recently, but I had similar fun when supporting extended info in our Tp backend tests. > > Please file a Vala bug and mention it as a FIXME here. > > Turns out it was me being stupid: the signal and callback types didn't match > because one was using Folks.PersonaStore and the other using Tpf.PersonaStore. > I was using ‘PersonaStore’ as one, but Vala thought it was the other. Fixed. Hurrah.
Review of attachment 203337 [details] [review]: Looks good!
Comment on attachment 203337 [details] [review] Add API to get a Tpf.Persona from a Tp.Contact (updated again) commit 0487df55a28c7fdf8670354f15c311b5c605ac3c Author: Philip Withnall <philip@tecnocode.co.uk> Date: Tue Dec 6 15:39:25 2011 +0000 Bug 665376 — Add API to get a TpfPersona from a TpContact Add static functions to quickly look up Tpf.PersonaStores and Tpf.Personas from Tp.Accounts and Tp.Contacts (respectively). Closes: bgo#665376 NEWS | 2 + backends/telepathy/lib/tpf-persona-store.vala | 223 +++++++++++++++++++++ backends/telepathy/lib/tpf-persona.vala | 71 +++++++- backends/telepathy/tp-backend.vala | 31 +--- tests/lib/telepathy/contactlist/account-manager.c | 2 + tests/lib/telepathy/contactlist/backend.c | 3 + 6 files changed, 304 insertions(+), 28 deletions(-)