Details

Reviewers

mtijink
nicolasfella

Group Reviewers

KDE Connect

Maniphest Tasks

T8283: Sync Contacts

Commits

R225:a2a06e386276: Add network packets for requesting contact names given uIDs
R225:263906db06a1: Add ContactsHelper for reading all of the RAW_CONTACT_IDs which contribute a…
R225:6da5104a1f85: Add method to ContactsHelper for extracting information from the contacts…
R225:b92288ea99cd: Replace TODOs with comments requesting bug reports
R225:6cffc74fd57d: Add first-cut contacts-reading plugin
R225:19d2e159205f: Add package type and handler for requesting contact phone numbers given a list…
R225:1919376a8be8: Remove old interface for requesting random jumble of contact names + phone…
R225:f52d823299a2: Add package type and handler for requesting unique identifiers for every…
R225:6f0c4764b112: Add package type and handler for requesting contact phone numbers given a list…
R225:3b19b6ebe25f: Add contacts-reading plugin (Android side)
R225:bfd11259a792: Add ContactsHelper method for accessing the Data table
R225:e70c8ced0c31: Run automatic code reformatter

Summary

Add a plugin to the KDE Connect Android application which supports reading the Android contacts databases and sending the requested data as vcards

Android automatically has support for exporting vcards with all the fields you would expect (phone, email, photo, etc.)
- I add two custom fields, one for the modification timestamp and another for the NAME_RAW_CONTACT_ID so that the contacts can be correlated back to the Android database

This does not (yet) support writing contacts back to the phone nor does it automatically listen to the phone's contacts database to change

BUG: 367999

Test Plan

Connect the device to the desktop and verify that vcards are created in QStandardPaths::GenericDataLocation / kpeoplevcard". On my system this is ~/.local/share/kpeoplevcard

Create a dummy contact on the device and verify it is synchronized (Currently not automatic, have to disconnect and reconnect or use dbus)

Modify the dummy contact and verify the modifications are synchronized (Currently not automatic, have to disconnect and reconnect or use dbus)

Delete the dummy contact and verify the deletion is synchronized (Currently not automatic, have to disconnect and reconnect or use dbus)

Diff Detail

Repository

R225 KDE Connect - Android application

Branch

contacts-rebase

Lint

No Linters Available

Unit

No Unit Test Coverage

There are a very large number of changes, so older changes are hidden. Show Older Changes

sredman added a reviewer: kdeconnect.Jan 6 2018, 1:54 AM

Looks promising (didn't test it though), so I added a few initial comments. It's best if you think what you want to use it for. Otherwise you're implementing something which may never be used, or is unsuitable for an use case.

src/org/kde/kdeconnect/Plugins/ContactsPlugin/ContactsPlugin.java
107	Two things: wouldn't it make more sense to keep the structure of the Android contacts (i.e. multiple numbers for one contact, so only send the name once)? And the index is never used (and does not uniquely correspond to a single contact), so you don't need to send it.
141	Can you separate this into a new function? Then we can reuse this stuff no matter how we query the contacts database (e.g. searching, single contacts, etc.) in the future. Also, some more comments what's happening here would be useful: not everyone has experience with ContentProvider code.
170	Maybe e-mail addresses are useful too?
237	Maybe better to move this check to the beginning of the function: that improves readability and reduces indenting.

In D9690#189003, @mtijink wrote:

Looks promising (didn't test it though), so I added a few initial comments. It's best if you think what you want to use it for. Otherwise you're implementing something which may never be used, or is unsuitable for an use case.

Thanks for the feedback!

For the in-line comments I didn't reply to, thank you for the good suggestions! I will implement them.

src/org/kde/kdeconnect/Plugins/ContactsPlugin/ContactsPlugin.java
107	I did it this way because I couldn't figure out how to use NetworkPackage to just send a pile of things and get them out on the other side, so I assign them unique (sequential) integers here, and the pull them out using the same scheme on the other side... However, I just had a much better idea. One problem I haven't been able to solve is how to distinguish between two contacts with the same name, but Android has already solved this problem by having unique keys assigned to every contact. So my new thought is to ship the list of IDs, then have each contact in the package stored with that ID as the key. I will have a new version of this plugin up soon!

Add network package type for requesting a list of unique IDs for every contact in the contacts book

Add ContactsHelper for reading all of the RAW_CONTACT_IDs which contribute a name to the Contacts database
Add package type and handler for requesting unique identifiers for every contact in the phone's database

sredman added inline comments.Jan 15 2018, 2:29 AM

src/org/kde/kdeconnect/Plugins/ContactsPlugin/ContactsPlugin.java
237	On thinking more about it, I want to be able to check against many types of packets, so having helper methods for each different type is a better way to clean this up. I have started implementing this.

Add network packets for requesting contact names given uIDs

Add method to ContactsHelper for extracting information from the contacts database given raw contact ids

Add interfaces for requesting data from Android

Add ContactsHelper method for accessing the Data table
Add package type and handler for requesting contact phone numbers given a list of uIDs
Remove old interface for requesting random jumble of contact names + phone numbers
Add package type and handler for requesting contact phone numbers given a list of uIDs
Run automatic code reformatter

sredman retitled this revision from Add first-cut contacts-reading plugin to Add contacts-reading plugin.Jan 16 2018, 1:41 AM

sredman edited the summary of this revision. (Show Details)

sredman edited the test plan for this revision. (Show Details)

sredman edited reviewers, added: KDE Connect; removed: kdeconnect.

With regards to contact IDs from Android, have you happened to notice if these are the same as reported by the Google Contacts API? If so, that may make some desktop integration slightly easier/more performant since at least libfolks offers these and the IDs could be used to pull photos and such from the local cache.

In D9690#217595, @andyholmes wrote:

With regards to contact IDs from Android, have you happened to notice if these are the same as reported by the Google Contacts API? If so, that may make some desktop integration slightly easier/more performant since at least libfolks offers these and the IDs could be used to pull photos and such from the local cache.

Interesting question. I don't have any experience with the Google Contacts API, so I can't answer, but my gut feeling would be not: You can have contacts on your phone which aren't synchronized to Google, and I think the contact ID I am using is just whatever primary key is assigned by Android's sqlite database

I suppose that makes sense, I know a number of people who use NextCloud for contacts. Google Contacts uses URNs such as "http://www.google.com/m8/feeds/contacts/my_name%40gmail.com/full/7c7ce738a953a16", which are obviously google specific.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 23 2018, 10:42 PM

Closed by commit R225:6cffc74fd57d: Add first-cut contacts-reading plugin (authored by sredman). · Explain Why

This revision was automatically updated to reflect the committed changes.

sredman reopened this revision.Mar 25 2018, 5:14 PM

Upgrade contacts synchronization plugin to VCard-based API

Add ContactsHelper for reading all of the RAW_CONTACT_IDs which contribute a name to the Contacts database
Add package type and handler for requesting unique identifiers for every contact in the phone's database
Add method to ContactsHelper for extracting information from the contacts database given raw contact ids
Add network packets for requesting contact names given uIDs
Add ContactsHelper method for accessing the Data table
Add package type and handler for requesting contact phone numbers given a list of uIDs
Remove old interface for requesting random jumble of contact names + phone numbers
Add package type and handler for requesting contact phone numbers given a list of uIDs
Update Contacts plugin to use NetworkPacket instead of NetworkPackage
Change type of IDs parameter to Collection as the comments suggested they should be
Add ContactsHelper method for extracting vcards
Remove 'pictures' parameter from getVCards since it was not possible to use
Split vcard access into fast and slow, where fast requires API >= 21
Set contacts plugin to only load from SDK 18
Strip down contacts plugin for new VCards API
Merge remote-tracking branch 'origin/master' into contacts
Add metadata to outgoing vcards
Enable ContactsPlugin by default

Replace TODOs about more information with bug reports asking for more information

This revision was not accepted when it landed; it landed in state Needs Review.Mar 25 2018, 6:28 PM

Closed by commit R225:b92288ea99cd: Replace TODOs with comments requesting bug reports (authored by sredman). · Explain Why

This revision was automatically updated to reflect the committed changes.

sredman added a task: T8283: Sync Contacts.Mar 27 2018, 4:39 PM

Restricted Application added a project: KDE Connect. · View Herald TranscriptMar 27 2018, 4:39 PM

What am I doing that makes this keep closing? Q.Q

sredman retitled this revision from Add contacts-reading plugin to Add contacts-reading plugin (Android side).Mar 27 2018, 5:21 PM

sredman edited the summary of this revision. (Show Details)

sredman edited the test plan for this revision. (Show Details)

The diff doesn't appear correctly either, right? Maybe it's because the code already is in a branch?

You could try deleting the branch on KDE git and then create a new diff.

In D9690#235446, @sredman wrote:

What am I doing that makes this keep closing? Q.Q

When you push to the remote branch Pabricator sees it and closes this. Just remove the 'Differential Revision' from the commit message

Update diff with all changes

Same diff but without accidentally sqashing into the HEAD of master

Add first-cut contacts-reading plugin
Add ContactsHelper for reading all of the RAW_CONTACT_IDs which contribute a name to the Contacts database
Add package type and handler for requesting unique identifiers for every contact in the phone's database
Add method to ContactsHelper for extracting information from the contacts database given raw contact ids
Add network packets for requesting contact names given uIDs
Add ContactsHelper method for accessing the Data table
Add package type and handler for requesting contact phone numbers given a list of uIDs
Remove old interface for requesting random jumble of contact names + phone numbers
Add package type and handler for requesting contact phone numbers given a list of uIDs
Run automatic code reformatter
Update Contacts plugin to use NetworkPacket instead of NetworkPackage
Change type of IDs parameter to Collection as the comments suggested they should be
Add ContactsHelper method for extracting vcards
Remove 'pictures' parameter from getVCards since it was not possible to use
Split vcard access into fast and slow, where fast requires API >= 21
Set contacts plugin to only load from SDK 18
Strip down contacts plugin for new VCards API
Add metadata to outgoing vcards
Enable ContactsPlugin by default
Run automatic code formatter
Replace TODOs with comments requesting bug reports
Remove leftover contactsProjection
Add helper for getting SMS threads

Remove SMSHelper from unrelated diff

The code generally looks good to me! I added a few comments.

src/org/kde/kdeconnect/Helpers/ContactsHelper.java
239	Do the individual vcards extracted here contain the id? If so, you can extract that and sort on that.
src/org/kde/kdeconnect/Plugins/ContactsPlugin/ContactsPlugin.java
111	You can use the `R.string.[...]` constant directly here.
118	Comment does not correspond with the code.
157	These comments do not apply to the lines immediately below? Maybe it should be moved a few lines down.

This revision now requires changes to proceed.Mar 29 2018, 8:01 PM

Update ContactsPlugin to use LOOKUP_KEY instead of RAW_CONTACT_ID since that is what LOOKUP_KEY is for and address reviewer com
ments

Thanks for the eyeballs!

src/org/kde/kdeconnect/Helpers/ContactsHelper.java
239	No. The vcards are basically exactly what you get if you use the Android Contacts GUI to export to vcard. That is, all the behind-the-scenes database nonsense is gone, and only real contact information remains. We could do tricks like take the phone number and go back into the contacts database to look it up, but that would (probably) end up being slower than the serial method
src/org/kde/kdeconnect/Plugins/ContactsPlugin/ContactsPlugin.java
157	They do directly apply in the sense that getting the timestamp is a many-step operation... Do you think it would make more sense to move the comment further down?

Same diff, minus SMS Helper (again...)

Fix bug causing all reads from databaase to return the same value

nicolasfella added inline comments.Apr 23 2018, 8:35 PM

res/values/strings.xml
231	The explanation doesn't really fit the use-case any more.

nicolasfella added inline comments.May 12 2018, 4:55 PM

src/org/kde/kdeconnect/Helpers/ContactsHelper.java
233	FileNotFoundException is an IOException, so one catch is enough

Restricted Application added a subscriber: kdeconnect. · View Herald TranscriptMay 12 2018, 4:55 PM

Address reviewer comments

res/values/strings.xml
231	I think it could be worded differently, but the basic use-case is still the same. The desktop can always fall back to showing some kind of raw value (email, phone number, etc.), but to show the name we need this permission

Make linter not complain

philipc added a subscriber: philipc.May 17 2018, 6:01 PM

philipc added inline comments.

src/org/kde/kdeconnect/Helpers/ContactsHelper.java
117 ↗	(On Diff #34383)	Couldn't we create these two streams in a try-with-resources block? I believe Android Studio has a quick fix for that

sredman marked 2 inline comments as done.May 31 2018, 12:08 AM

sredman added inline comments.

src/org/kde/kdeconnect/Helpers/ContactsHelper.java
117 ↗	(On Diff #34383)	It looks like we could, but try-with-resources requires API level 19 (current min is 9)

nicolasfella added inline comments.May 31 2018, 12:10 AM

src/org/kde/kdeconnect/Helpers/ContactsHelper.java
117 ↗	(On Diff #34383)	The plugin min is 18

sredman added inline comments.May 31 2018, 12:15 AM

src/org/kde/kdeconnect/Helpers/ContactsHelper.java
117 ↗	(On Diff #34383)	True, but this is within the ContactsHelper, which should ideally be useful to many plugins and not enforce a min API (but the method we are currently commenting on is annotated to API 11)

It does not build

src/org/kde/kdeconnect/Helpers/ContactsHelper.java
272 ↗	(On Diff #34383)	+ ID instead of + uID?

This revision now requires changes to proceed.May 31 2018, 12:26 AM

Fix error created when cleaning warnings
Reword permission request string

Ug. Whoops. Yes, ID is correct

It works as expected for me and I can't find anything obviously wrong. Given that Matthijs seems pretty satisfied as well I think you can land this

MatMaul added a subscriber: MatMaul.Jun 2 2018, 11:17 AM

This revision was not accepted when it landed; it landed in state Needs Review.Jun 8 2018, 3:06 AM

Closed by commit R225:3b19b6ebe25f: Add contacts-reading plugin (Android side) (authored by sredman). · Explain Why

This revision was automatically updated to reflect the committed changes.

		Path
M		res/values/strings.xml (3 lines)
M		src/org/kde/kdeconnect/Helpers/ContactsHelper.java (365 lines)
A	M	src/org/kde/kdeconnect/Plugins/ContactsPlugin/ContactsPlugin.java (258 lines)
M		src/org/kde/kdeconnect/Plugins/PluginFactory.java (2 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	24806	22696f7		Jan 6 2018, 1:48 AM	★	★
Diff 2	25360	22696f7	Add network package type for requesting a list of unique IDs for every contact…	Jan 15 2018, 2:20 AM	★	★
Diff 3	25364	22696f7	Add network packets for requesting contact names given uIDs	Jan 15 2018, 7:36 AM	★	★
Diff 4	25434	22696f7	Add interfaces for requesting data from Android	Jan 16 2018, 1:14 AM	★	★
Diff 5	30360	22696f7	R225:6cffc74fd57d68aec87cb985b304468ee4726c1d	Jan 6 2018, 1:48 AM	★	★
Diff 6	30532	ac4aaf1	Upgrade contacts synchronization plugin to VCard-based API	Mar 25 2018, 5:15 PM	★	★
Diff 7	30538	75997cd	Replace TODOs about more information with bug reports asking for more…	Mar 25 2018, 6:26 PM	★	★
Diff 8	30541	75997cd	R225:b92288ea99cd4231c910c35b5191c9284c34a72f	Mar 25 2018, 6:24 PM	★	★
Diff 9	30770	64fd08f	Update diff with all changes	Mar 28 2018, 12:47 AM	★	★
Diff 10	30773	7c8301a	Same diff but without accidentally sqashing into the HEAD of master	Mar 28 2018, 1:16 AM	★	★
Diff 11	30780	7c8301a	Remove SMSHelper from unrelated diff	Mar 28 2018, 4:56 AM	★	★
Diff 12	30899	7c8301a	Update ContactsPlugin to use LOOKUP_KEY instead of RAW_CONTACT_ID since that is…	Mar 30 2018, 5:08 AM	★	★
Diff 13	30900	7c8301a	Same diff, minus SMS Helper (again...)	Mar 30 2018, 5:18 AM	★	★
Diff 14	31072	00b6677	Fix bug causing all reads from databaase to return the same value	Mar 31 2018, 9:43 PM	★	★
Diff 15	34377	85c51ee	Address reviewer comments	May 17 2018, 3:44 PM	★	★
Diff 16	34383	85c51ee	Make linter not complain	May 17 2018, 5:37 PM	★	★
Diff 17	35225	8cc908e	Fix error created when cleaning warnings	May 31 2018, 12:37 AM	★	★
Diff 18	35815	0dc424e	R225:3b19b6ebe25fda28f8434cddeef8e8b3a54b17c3	Jun 8 2018, 3:04 AM	★	★

Commit	Tree	Parents	Author	Summary	Date
5bd087ac7851	d9f72b368c6b	b67683f003ff	Simon Redman	Reword contacts plugin permission request string	May 31 2018, 12:33 AM
b67683f003ff	6cc51ffc3deb	106a4029e822	Simon Redman	Fix error from warnings cleanup	May 31 2018, 12:29 AM
106a4029e822	dc33b0ce0f99	58430a7b9872	Simon Redman	Make linter not complain	May 17 2018, 5:35 PM
58430a7b9872	a5685db093ee	e00174324c07	Simon Redman	Reduce catch block using inherited Exception	May 17 2018, 3:41 PM
e00174324c07	b8a066139e2d	1b401fdca266	Simon Redman	Fix reference bug with every reply from getColumnsFromContactsForIDs getting… (Show More…)	Mar 31 2018, 9:31 PM
1b401fdca266	4f9352d4c209	8f70aaee1e21	Simon Redman	Revert "Add helper for getting SMS threads" (Show More…)	Mar 30 2018, 9:37 PM
8f70aaee1e21	c500a1bfc0b2	709195b54941	Simon Redman	Inline access to R.string.contacts_permission_explaintion	Mar 30 2018, 5:04 AM
709195b54941	a3664cbb8bc8	74377937a216	Simon Redman	Change to abstract uID type and use LOOKUP_KEY instead of RAW_CONTACT_ID, since… (Show More…)	Mar 30 2018, 4:56 AM
74377937a216	cd3700b24541	fd21e680c8b4	Simon Redman	Change to always using the slow-but-reliable way to access vcards	Mar 28 2018, 11:20 PM
fd21e680c8b4	492f55f1249b	1caf21f27505	Simon Redman	Add VCardBuilder class	Mar 28 2018, 11:01 PM
1caf21f27505	1afb9444ba5f	42b3833fc9bd	Simon Redman	Run autoformatter	Mar 28 2018, 10:59 PM
42b3833fc9bd	8efcf162dd5c	b2d6d57d4ffd	Simon Redman	Add helper for getting SMS threads	Mar 25 2018, 7:59 PM
b2d6d57d4ffd	0c96006d1986	09e1311af881	Simon Redman	Remove leftover contactsProjection	Mar 25 2018, 7:59 PM
09e1311af881	09979ff314fc	2a04c8717b35	Simon Redman	Replace TODOs with comments requesting bug reports	Mar 25 2018, 6:24 PM
2a04c8717b35	ae73991b4762	19cd56261f1c	Simon Redman	Run automatic code formatter	Mar 25 2018, 5:13 PM
19cd56261f1c	d7c9665438eb	d790f152f9fe	Simon Redman	Enable ContactsPlugin by default	Mar 25 2018, 5:10 PM
d790f152f9fe	a579e3bd3f63	8fc590b27df5	Simon Redman	Add metadata to outgoing vcards	Mar 25 2018, 2:29 PM
8fc590b27df5	5768381e0a8f	1de2fc697c13	Simon Redman	Strip down contacts plugin for new VCards API	Mar 25 2018, 11:12 AM
1de2fc697c13	72f2b220695a	0d3559ca143f	Simon Redman	Set contacts plugin to only load from SDK 18	Mar 25 2018, 11:10 AM
0d3559ca143f	f8c3e3265716	b2daf2b34ce8	Simon Redman	Split vcard access into fast and slow, where fast requires API >= 21	Mar 24 2018, 8:32 PM
b2daf2b34ce8	224cac86f804	14134e579f53	Simon Redman	Remove 'pictures' parameter from getVCards since it was not possible to use	Mar 24 2018, 7:11 PM
14134e579f53	5cdd66dc8b77	358b6aa6eb0e	Simon Redman	Add ContactsHelper method for extracting vcards	Mar 24 2018, 5:21 PM
358b6aa6eb0e	88edb375b1bb	515976d05ca7	Simon Redman	Change type of IDs parameter to Collection as the comments suggested they… (Show More…)	Mar 24 2018, 5:20 PM
515976d05ca7	e1ef664cc3c8	33febcae56ab	Simon Redman	Update Contacts plugin to use NetworkPacket instead of NetworkPackage	Mar 23 2018, 10:32 PM
33febcae56ab	2ae06cf76147	dc8c9d96b317	Simon Redman	Run automatic code reformatter	Jan 16 2018, 12:57 AM
dc8c9d96b317	830194473c2a	084ff5af2e04	Simon Redman	Add package type and handler for requesting contact phone numbers given a list… (Show More…)	Jan 16 2018, 12:55 AM
084ff5af2e04	d4ebb6dd7acd	6cbd20bad0f8	Simon Redman	Remove old interface for requesting random jumble of contact names + phone… (Show More…)	Jan 15 2018, 10:56 PM
6cbd20bad0f8	0f8106a942e1	6cba3dcd95aa	Simon Redman	Add package type and handler for requesting contact phone numbers given a list… (Show More…)	Jan 15 2018, 10:53 PM
6cba3dcd95aa	23056166bbfa	a59a80b92a49	Simon Redman	Add ContactsHelper method for accessing the Data table	Jan 15 2018, 10:51 PM
a59a80b92a49	3fe37fa6a836	d564180d0ffa	Simon Redman	Add network packets for requesting contact names given uIDs	Jan 15 2018, 7:29 AM
d564180d0ffa	5d9f1e8bf11a	c64dffb637d4	Simon Redman	Add method to ContactsHelper for extracting information from the contacts… (Show More…)	Jan 15 2018, 7:16 AM
c64dffb637d4	430877d9c546	11e705bebcab	Simon Redman	Add package type and handler for requesting unique identifiers for every… (Show More…)	Jan 15 2018, 2:18 AM
11e705bebcab	62ce460b0f19	26974ca11a40	Simon Redman	Add ContactsHelper for reading all of the RAW_CONTACT_IDs which contribute a… (Show More…)	Jan 15 2018, 2:16 AM
26974ca11a40	97262e9d64a4	8cc908edb003	Simon Redman	Add first-cut contacts-reading plugin (Show More…)	Jan 6 2018, 1:39 AM

Add contacts-reading plugin (Android side)
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 35225

res/values/strings.xml

src/org/kde/kdeconnect/Helpers/ContactsHelper.java

src/org/kde/kdeconnect/Plugins/ContactsPlugin/ContactsPlugin.java

src/org/kde/kdeconnect/Plugins/PluginFactory.java

Add contacts-reading plugin (Android side)ClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 35225

res/values/strings.xml

src/org/kde/kdeconnect/Helpers/ContactsHelper.java

src/org/kde/kdeconnect/Plugins/ContactsPlugin/ContactsPlugin.java

src/org/kde/kdeconnect/Plugins/PluginFactory.java

Add contacts-reading plugin (Android side)
ClosedPublic
Actions

Revision Contents
Changeset List