GNOME Bugzilla – Bug 711827
Implement period refreshes of the persona store from the phone
Last modified: 2014-01-04 15:39:27 UTC
Patch coming to implement periodic refreshes of the persona store.
Patches are here: http://cgit.collabora.com/git/user/pwith/folks.git/log/?h=711827-bluez-periodic-download
Created attachment 259539 [details] [review] bluez: Remove unnecessary OBEX List() D-Bus call It returns a mapping of VCF filenames to contact full names, which we already have from the full VCF file. Minor performance improvement (although I haven’t profiled it).
Created attachment 259540 [details] [review] core: Tighten up definition of Persona.display_id to ensure it’s unique It is defined as being unique, so cannot be a human-readable full name, since those can be non-unique. Update the EDS, oFono and Tracker backends to use their persona IIDs as display IDs, instead full names. This isn’t an API break, as the API was always documented as being unique.
Created attachment 259541 [details] [review] bluez: Support updating contacts instead of replacing them when re-querying When downloading the contact list over PBAP for anything other than the first time, support updating the properties of existing personas, rather than replacing them all wholesale. This is a step on the way towards supporting periodic refreshes of the contact list.
Created attachment 259542 [details] [review] bluez: Implement period refreshes of the persona store from the phone This implements two major changes: • Downloading contacts now happens in two phases. In the first phase, only textual properties are downloaded (not contact photos). This is fast, taking only a few seconds to download several hundred contacts over Bluetooth. In the second phase, contact photos are downloaded in the background and personas are updated to use them. • Contacts are periodically re-downloaded on an exponential timeout leading to a linear region. This keeps the persona store up-to-date with changes in the phone’s address book. For full details, see the documentation on BlueZ.PersonaStore._schedule_update_contacts().
Created attachment 259881 [details] [review] bluez: Remove unnecessary OBEX List() D-Bus call It returns a mapping of VCF filenames to contact full names, which we already have from the full VCF file. Minor performance improvement (although I haven’t profiled it).
Created attachment 259882 [details] [review] core: Tighten up definition of Persona.display_id to ensure it’s unique It is defined as being unique, so cannot be a human-readable full name, since those can be non-unique. Update the EDS, oFono and Tracker backends to use their persona IIDs as display IDs, instead full names. This isn’t an API break, as the API was always documented as being unique.
Created attachment 259883 [details] [review] bluez: Support updating contacts instead of replacing them when re-querying When downloading the contact list over PBAP for anything other than the first time, support updating the properties of existing personas, rather than replacing them all wholesale. This is a step on the way towards supporting periodic refreshes of the contact list.
Created attachment 259884 [details] [review] bluez: Implement periodic refreshes of the persona store from the phone This implements two major changes: • Downloading contacts now happens in two phases. In the first phase, only textual properties are downloaded (not contact photos). This is fast, taking only a few seconds to download several hundred contacts over Bluetooth. In the second phase, contact photos are downloaded in the background and personas are updated to use them. • Contacts are periodically re-downloaded on an exponential timeout leading to a linear region. This keeps the persona store up-to-date with changes in the phone’s address book. For full details, see the documentation on BlueZ.PersonaStore._schedule_update_contacts().
Created attachment 259885 [details] [review] bluez: Eliminate a reflexive reference in Bluez.PersonaStore Since the persona store is always scheduling the next update, it always holds a reference to itself in the timeout closure, which is unavoidable. Add a cancel_updates() internal method which can be called by the Backend to break this reflexive reference and allow the store to be finalised.
Created attachment 259886 [details] [review] bluez: Don’t block on cleanup finishing when updating contacts This speeds up updating the contacts, and also elimates the possibility of Vala’s async state machine getting into an illegal state, which somehow happened before.
Review of attachment 259881 [details] [review]: This adds some odd definition for display_id based on the full_name but it's removed in subsequent patches (which I haven't gotten to yet). So, pedantically, I'd like this fixed, but as long as we apply the following patches, it's fine as-is.
Review of attachment 259882 [details] [review]: I haven't been able to test this yet but it otherwise looks fine.
Review of attachment 259883 [details] [review]: looks good
Review of attachment 259884 [details] [review]: + * B. If the user explicitly denies the connection request on the phone, the + * phone should remember this and automatically deny all future connectio + * attempts until the consecutive failure limit is reached. The user + * shouldn’t be pestered to accept again. Is this actually happening anywhere? I don't see it. If it is (eg, implicitly by some BlueZ behavior I'm not aware of), please add a comment explaining that and apply. Otherwise, please update the patch to guarantee we are accounting for the user denying a connection.
Review of attachment 259885 [details] [review]: Looks good
Review of attachment 259886 [details] [review]: Looks good
Thanks for the reviews. I think it would confuse matters to modify how the display_id changes were made (comment #12), since the second patch needs to be separate to get the rationale logged somewhere. Re. comment #15: That’s the behaviour I would expect from the phone’s OS: if the user has denied permission to share the phone address book with a computer once, I would expect the phone to remember that and automatically deny all incoming BlueZ OBEX requests in future. The code in libfolks will then hit the 3-attempt limit and fail gracefully. I believe this is already mentioned in the comment, so I have not changed the patch at all. Attachment 259881 [details] pushed as 0d05c56 - bluez: Remove unnecessary OBEX List() D-Bus call Attachment 259882 [details] pushed as 1652c7b - core: Tighten up definition of Persona.display_id to ensure it’s unique Attachment 259883 [details] pushed as ab76542 - bluez: Support updating contacts instead of replacing them when re-querying Attachment 259884 [details] pushed as 3439f37 - bluez: Implement periodic refreshes of the persona store from the phone Attachment 259885 [details] pushed as c83bddc - bluez: Eliminate a reflexive reference in Bluez.PersonaStore Attachment 259886 [details] pushed as f98693a - bluez: Don’t block on cleanup finishing when updating contacts