From 0487df55a28c7fdf8670354f15c311b5c605ac3c Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 6 Dec 2011 15:39:25 +0000 Subject: [PATCH] =?utf8?q?Bug=20665376=20=E2=80=94=20Add=20API=20to=20get?= =?utf8?q?=20a=20TpfPersona=20from=20a=20TpContact?= MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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(-) diff --git a/NEWS b/NEWS index 17e0d0a..1db5861 100644 --- a/NEWS +++ b/NEWS @@ -9,6 +9,7 @@ Bugs fixed: * Bug 665728 — TpfPersonaStore: prepare() isn't mutually exclusive inside a single thread * Bug 665692 — Use constructors correctly +* Bug 665376 — Add API to get a TpfPersona from a TpContact API changes: * Add Edsf.PersonaStore.source @@ -26,6 +27,7 @@ API changes: normal setter) * Add ObjectCache.type_id * Add ObjectCache.id +* Add Tpf.PersonaStore.dup_for_account() and Tpf.Persona.dup_for_contact() Overview of changes from libfolks 0.6.4.1 to libfolks 0.6.5 ============================================================= diff --git a/backends/telepathy/lib/tpf-persona-store.vala b/backends/telepathy/lib/tpf-persona-store.vala index d5c2dde..a311ec6 100644 --- a/backends/telepathy/lib/tpf-persona-store.vala +++ b/backends/telepathy/lib/tpf-persona-store.vala @@ -78,6 +78,7 @@ public class Tpf.PersonaStore : Folks.PersonaStore private HashSet _persona_set; /* universal, contact owner handles (not channel-specific) */ private HashMap _handle_persona_map; + private HashSet _weakly_referenced_personas; private HashMap> _channel_group_personas_map; private HashMap> _channel_group_incoming_adds; private HashMap> _group_outgoing_adds; @@ -260,6 +261,9 @@ public class Tpf.PersonaStore : Folks.PersonaStore this._debug = Debug.dup (); this._debug.print_status.connect (this._debug_print_status); + // Add to the map of persona stores by account + PersonaStore._add_store_to_map (this); + // Set up the cache this._cache = new PersonaStoreCache (this); @@ -270,6 +274,9 @@ public class Tpf.PersonaStore : Folks.PersonaStore { debug ("Destroying Tpf.PersonaStore %p ('%s').", this, this.id); + // Remove from the map of persona stores by account + PersonaStore._remove_store_from_map (this); + this._debug.print_status.disconnect (this._debug_print_status); this._debug = null; if (this._logger != null) @@ -510,6 +517,19 @@ public class Tpf.PersonaStore : Folks.PersonaStore this._conn = null; } + 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); + } + } + } + + this._weakly_referenced_personas = new HashSet (); + this._handle_persona_map = new HashMap (); this._channel_group_personas_map = new HashMap> (); @@ -1533,6 +1553,13 @@ public class Tpf.PersonaStore : Folks.PersonaStore if (persona == null) return; + /* 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); + } + /* * remove all persona-keyed entries */ @@ -1930,6 +1957,56 @@ public class Tpf.PersonaStore : Folks.PersonaStore return personas; } + 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); + } + + internal Tpf.Persona? _ensure_persona_from_contact (Contact contact) + { + uint handle = contact.get_handle (); + + debug ("Ensuring contact %p (handle: %u) exists in Tpf.PersonaStore " + + "%p ('%s').", contact, handle, this, this.id); + + if (handle == 0) + { + return null; + } + + /* If the persona already exists, return them. */ + var persona = this._handle_persona_map[handle]; + + if (persona != null) + { + return persona; + } + + /* Otherwise, add the persona to the store. See bgo#665376 for details of + * why this is necessary. Since the TpContact is coming from a source + * other than the TpChannels which are associated with this store, we only + * hold a weak reference to it and remove it from the store as soon as + * it's destroyed. */ + persona = this._add_persona_from_contact (contact, false); + + if (persona == null) + { + return null; + } + + /* Weak ref. on the contact. */ + contact.weak_ref (this._contact_weak_notify_cb); + this._weakly_referenced_personas.add (persona); + + /* Signal the addition of the new persona. */ + var personas = new HashSet (); + this._emit_personas_changed (personas, null); + + return persona; + } + private Tpf.Persona? _add_persona_from_contact (Contact contact, bool from_contact_list) { @@ -2332,4 +2409,150 @@ public class Tpf.PersonaStore : Folks.PersonaStore return info_list; } + + /* Must be locked before being accessed. A ref. is held on each PersonaStore, + * and they're only removed when they're finalised or their removed signal is + * emitted. The map as a whole is lazily constructed and destroyed according + * to when PersonaStores are constructed and destroyed. */ + private static HashMap + _persona_stores_by_account = null; + private static Map _persona_stores_by_account_ro = null; + + /** + * Get a map of all the currently constructed {@link Tpf.PersonaStore}s. + * + * If a {@link BackendStore} has been prepared, this map will be complete, + * containing every store known to the Telepathy account manager. If no + * {@link BackendStore} has been prepared, this map will only contain the + * stores which have been created by calling + * {@link Tpf.PersonaStore.dup_for_account}. + * + * This map is read-only. Use {@link BackendStore} or + * {@link Tpf.PersonaStore.dup_for_account} to add stores. + * + * @return map from {@link PersonaStore.id} to {@link PersonaStore} + * @since UNRELEASED + */ + public static unowned Map list_persona_stores () + { + unowned Map store; + + lock (PersonaStore._persona_stores_by_account) + { + if (PersonaStore._persona_stores_by_account == null) + { + PersonaStore._persona_stores_by_account = + new HashMap (); + PersonaStore._persona_stores_by_account_ro = + PersonaStore._persona_stores_by_account.read_only_view; + } + + store = PersonaStore._persona_stores_by_account_ro; + } + + return store; + } + + private static void _store_removed_cb (Folks.PersonaStore store) + { + /* Remove the store from the map. */ + PersonaStore._remove_store_from_map ((Tpf.PersonaStore) store); + } + + private static void _add_store_to_map (PersonaStore store) + { + debug ("Adding PersonaStore %p ('%s') to map.", store, store.id); + + lock (PersonaStore._persona_stores_by_account) + { + /* Lazy construction. */ + if (PersonaStore._persona_stores_by_account == null) + { + PersonaStore._persona_stores_by_account = + new HashMap (); + PersonaStore._persona_stores_by_account_ro = + PersonaStore._persona_stores_by_account.read_only_view; + } + + /* Bail if a store already exists for this account. */ + assert (!PersonaStore._persona_stores_by_account.has_key (store.id)); + + /* Add the store. */ + PersonaStore._persona_stores_by_account.set (store.id, store); + store.removed.connect (PersonaStore._store_removed_cb); + } + } + + private static void _remove_store_from_map (PersonaStore store) + { + debug ("Removing PersonaStore %p ('%s') from map.", store, store.id); + + lock (PersonaStore._persona_stores_by_account) + { + /* Bail if no store existed for this account. This can happen if the + * store emits its removed() signal (correctly) before being + * finalised; we remove the store from the map in both cases. */ + if (PersonaStore._persona_stores_by_account == null || + !PersonaStore._persona_stores_by_account.unset (store.id)) + { + return; + } + + store.removed.disconnect (PersonaStore._store_removed_cb); + + /* Lazy destruction. */ + if (PersonaStore._persona_stores_by_account.size == 0) + { + PersonaStore._persona_stores_by_account_ro = null; + PersonaStore._persona_stores_by_account = null; + } + } + } + + /** + * Look up a {@link Tpf.PersonaStore} by its {@link TelepathyGLib.Account}. + * + * If found, a new reference to the persona store will be returned. If not + * found, a new {@link Tpf.PersonaStore} will be created for the account. + * + * See the documentation for {@link Tpf.PersonaStore.list_persona_stores} for + * information on the lifecycle of these stores when a {@link BackendStore} is + * and is not present. + * + * @param account the Telepathy account of the persona store + * @return the persona store associated with the account + * @since UNRELEASED + */ + public static PersonaStore dup_for_account (Account account) + { + PersonaStore? store = null; + + debug ("Tpf.PersonaStore.dup_for_account (%p):", account); + + lock (PersonaStore._persona_stores_by_account) + { + /* If the store already exists, return it. */ + if (PersonaStore._persona_stores_by_account != null) + { + store = + PersonaStore._persona_stores_by_account.get ( + account.get_object_path ()); + } + + /* Otherwise, we have to create it. It's added to the map in its + * constructor. */ + if (store == null) + { + debug (" Creating new PersonaStore."); + store = new PersonaStore (account); + } + else + { + debug (" Found existing PersonaStore %p ('%s').", store, + store.id); + } + } + + return store; + } } diff --git a/backends/telepathy/lib/tpf-persona.vala b/backends/telepathy/lib/tpf-persona.vala index bfe1783..60b9c84 100644 --- a/backends/telepathy/lib/tpf-persona.vala +++ b/backends/telepathy/lib/tpf-persona.vala @@ -464,6 +464,16 @@ public class Tpf.Persona : Folks.Persona, this.notify_property ("groups"); } + /* This has to be weak since, in general, we can't force any TpContacts to + * remain alive if we want to solve bgo#665376. */ + private weak Contact? _contact = null; + + private void _contact_weak_notify_cb (Object obj) + { + this._contact = null; + this.notify_property ("contact"); + } + /** * The Telepathy contact represented by this persona. * @@ -473,7 +483,28 @@ public class Tpf.Persona : Folks.Persona, * are being retrieved from a cache and may not be current (though there's no * way to tell this). */ - public Contact? contact { get; construct; } + public Contact? contact + { + get + { + if (this._contact == null) + { + return null; + } + + return this._contact; + } + + construct + { + if (value != null) + { + value.weak_ref (this._contact_weak_notify_cb); + } + + this._contact = value; + } + } private HashSet _phone_numbers = new HashSet ( @@ -967,6 +998,11 @@ public class Tpf.Persona : Folks.Persona, ~Persona () { debug ("Destroying Tpf.Persona '%s': %p", this.uid, this); + + if (this._contact != null) + { + this._contact.weak_unref (this._contact_weak_notify_cb); + } } private static Account? _account_for_connection (Connection conn) @@ -1046,4 +1082,37 @@ public class Tpf.Persona : Folks.Persona, this.notify_property ("avatar"); } } + + /** + * Look up a {@link Tpf.Persona} by its {@link TelepathyGLib.Contact}. + * + * If the {@link TelepathyGLib.Account} for the contact's + * {@link TelepathyGLib.Connection} is `null`, or if a + * {@link Tpf.PersonaStore} can't be found for that account, `null` will be + * returned. Otherwise, if a {@link Tpf.Persona} already exists for the given + * contact, that will be returned; if one doesn't exist a new one will be + * created and returned. In this case, the {@link Tpf.Persona} will be added + * to the {@link PersonaStore} associated with the account, and will be + * removed when `contact` is destroyed. + * + * @param contact the Telepathy contact of the persona + * @return the persona associated with the contact, or `null` + * @since UNRELEASED + */ + public static Persona? dup_for_contact (Contact contact) + { + var account = contact.connection.get_account (); + + debug ("Tpf.Persona.dup_for_contact (%p): got account %p", contact, + account); + + /* Account could be null; see the docs for tp_connection_get_account(). */ + if (account == null) + { + return null; + } + + var store = PersonaStore.dup_for_account (account); + return store._ensure_persona_from_contact (contact); + } } diff --git a/backends/telepathy/tp-backend.vala b/backends/telepathy/tp-backend.vala index 3f770b1..9df7827 100644 --- a/backends/telepathy/tp-backend.vala +++ b/backends/telepathy/tp-backend.vala @@ -36,8 +36,6 @@ public class Folks.Backends.Tp.Backend : Folks.Backend private bool _is_prepared = false; private bool _prepare_pending = false; /* used by unprepare() too */ private bool _is_quiescent = false; - private HashMap _persona_stores; - private Map _persona_stores_ro; /** * {@inheritDoc} @@ -49,7 +47,7 @@ public class Folks.Backends.Tp.Backend : Folks.Backend */ public override Map persona_stores { - get { return this._persona_stores_ro; } + get { return Tpf.PersonaStore.list_persona_stores (); } } /** @@ -60,12 +58,6 @@ public class Folks.Backends.Tp.Backend : Folks.Backend Object (); } - construct - { - this._persona_stores = new HashMap (); - this._persona_stores_ro = this._persona_stores.read_only_view; - } - /** * Whether this Backend has been prepared. * @@ -151,14 +143,6 @@ public class Folks.Backends.Tp.Backend : Folks.Backend this._account_validity_changed_cb); this._account_manager = null; - foreach (var persona_store in this._persona_stores.values) - { - this.persona_store_removed (persona_store); - } - - this._persona_stores.clear (); - this.notify_property ("persona-stores"); - this._is_quiescent = false; this.notify_property ("is-quiescent"); @@ -180,25 +164,18 @@ public class Folks.Backends.Tp.Backend : Folks.Backend private void _account_enabled_cb (Account account) { - var store = this._persona_stores.get (account.get_object_path ()); - - if (store != null) - return; - - store = new Tpf.PersonaStore (account); - - this._persona_stores.set (store.id, store); + var store = Tpf.PersonaStore.dup_for_account (account); store.removed.connect (this._store_removed_cb); - this.notify_property ("persona-stores"); + this.notify_property ("persona-stores"); this.persona_store_added (store); } private void _store_removed_cb (PersonaStore store) { store.removed.disconnect (this._store_removed_cb); + this.persona_store_removed (store); - this._persona_stores.unset (store.id); this.notify_property ("persona-stores"); } } diff --git a/tests/lib/telepathy/contactlist/account-manager.c b/tests/lib/telepathy/contactlist/account-manager.c index 26a1a3f..6c3191b 100644 --- a/tests/lib/telepathy/contactlist/account-manager.c +++ b/tests/lib/telepathy/contactlist/account-manager.c @@ -201,6 +201,7 @@ tp_test_account_manager_add_account (TpTestAccountManager *self, const gchar *account_path) { g_ptr_array_add (self->priv->valid_accounts, g_strdup (account_path)); + tp_svc_account_manager_emit_account_validity_changed (self, account_path, TRUE); g_object_notify (G_OBJECT (self), "valid-accounts"); } @@ -221,4 +222,5 @@ tp_test_account_manager_remove_account (TpTestAccountManager *self, } g_object_notify (G_OBJECT (self), "valid-accounts"); + tp_svc_account_manager_emit_account_removed (self, account_path); } diff --git a/tests/lib/telepathy/contactlist/backend.c b/tests/lib/telepathy/contactlist/backend.c index 9c7a2e0..f7d6db1 100644 --- a/tests/lib/telepathy/contactlist/backend.c +++ b/tests/lib/telepathy/contactlist/backend.c @@ -22,6 +22,7 @@ #include #include #include +#include #include "account.h" #include "account-manager.h" @@ -295,6 +296,8 @@ tp_test_backend_remove_account (TpTestBackend *self, tp_base_connection_change_status (data->conn, TP_CONNECTION_STATUS_DISCONNECTED, TP_CONNECTION_STATUS_REASON_REQUESTED); + tp_svc_account_emit_removed (data->account); + tp_dbus_daemon_unregister_object (priv->daemon, data->account); tp_clear_object (&data->account); -- 2.7.4