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
236	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
236	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
230	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	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	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	The plugin min is 18

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

src/org/kde/kdeconnect/Helpers/ContactsHelper.java
117	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
269	+ 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 (347 lines)
A	M	src/org/kde/kdeconnect/Plugins/ContactsPlugin/ContactsPlugin.java (256 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
86a40fb433b5	144b57eb54cd	6e933f530a77	Simon Redman	Reduce catch block using inherited Exception	May 17 2018, 3:41 PM
6e933f530a77	c58235e671f8	f4f6e1ccc872	Simon Redman	Fix reference bug with every reply from getColumnsFromContactsForIDs getting… (Show More…)	Mar 31 2018, 9:31 PM
f4f6e1ccc872	fc1060799ed7	b11c7d8a9eb7	Simon Redman	Revert "Add helper for getting SMS threads" (Show More…)	Mar 30 2018, 9:37 PM
b11c7d8a9eb7	956d28186032	542640539be5	Simon Redman	Inline access to R.string.contacts_permission_explaintion	Mar 30 2018, 5:04 AM
542640539be5	d4415434c715	4b4954e2bbe0	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
4b4954e2bbe0	9c50f3c08346	d8abab60fe68	Simon Redman	Change to always using the slow-but-reliable way to access vcards	Mar 28 2018, 11:20 PM
d8abab60fe68	c2ce1ade1464	e42794a6f19e	Simon Redman	Add VCardBuilder class	Mar 28 2018, 11:01 PM
e42794a6f19e	57e4bbc08059	7752ac3c74e8	Simon Redman	Run autoformatter	Mar 28 2018, 10:59 PM
7752ac3c74e8	ab9668ff843a	c8cb58ff7f42	Simon Redman	Add helper for getting SMS threads	Mar 25 2018, 7:59 PM
c8cb58ff7f42	0ebc95739ac1	9f1fa6d5f220	Simon Redman	Remove leftover contactsProjection	Mar 25 2018, 7:59 PM
9f1fa6d5f220	f4bc152fd56c	fb4ccdc8f0fa	Simon Redman	Replace TODOs with comments requesting bug reports	Mar 25 2018, 6:24 PM
fb4ccdc8f0fa	6928e9425e57	dd5da51c94e4	Simon Redman	Run automatic code formatter	Mar 25 2018, 5:13 PM
dd5da51c94e4	c1ba8a08f655	3ffca5da0dc2	Simon Redman	Enable ContactsPlugin by default	Mar 25 2018, 5:10 PM
3ffca5da0dc2	3f0a2259b2ee	543f85988c96	Simon Redman	Add metadata to outgoing vcards	Mar 25 2018, 2:29 PM
543f85988c96	6e877072cd8c	e210e9f1c1b1	Simon Redman	Strip down contacts plugin for new VCards API	Mar 25 2018, 11:12 AM
e210e9f1c1b1	ee5ced92bb5a	e3e94041d1fb	Simon Redman	Set contacts plugin to only load from SDK 18	Mar 25 2018, 11:10 AM
e3e94041d1fb	a2674a52105e	d62368d028a6	Simon Redman	Split vcard access into fast and slow, where fast requires API >= 21	Mar 24 2018, 8:32 PM
d62368d028a6	b4d5dc523d53	f34ab033e7e6	Simon Redman	Remove 'pictures' parameter from getVCards since it was not possible to use	Mar 24 2018, 7:11 PM
f34ab033e7e6	0dbb8c4ab3eb	27fe29c1450a	Simon Redman	Add ContactsHelper method for extracting vcards	Mar 24 2018, 5:21 PM
27fe29c1450a	fdac938430d6	974448eef3e1	Simon Redman	Change type of IDs parameter to Collection as the comments suggested they… (Show More…)	Mar 24 2018, 5:20 PM
974448eef3e1	7b39a9b4f5c0	caea3307184e	Simon Redman	Update Contacts plugin to use NetworkPacket instead of NetworkPackage	Mar 23 2018, 10:32 PM
caea3307184e	3acd03fae266	9d12f52bcf96	Simon Redman	Run automatic code reformatter	Jan 16 2018, 12:57 AM
9d12f52bcf96	00074f01535e	a354c68d7146	Simon Redman	Add package type and handler for requesting contact phone numbers given a list… (Show More…)	Jan 16 2018, 12:55 AM
a354c68d7146	16990ef6e40e	eb22eef096b1	Simon Redman	Remove old interface for requesting random jumble of contact names + phone… (Show More…)	Jan 15 2018, 10:56 PM
eb22eef096b1	e36375415e8b	3c2ecd9de531	Simon Redman	Add package type and handler for requesting contact phone numbers given a list… (Show More…)	Jan 15 2018, 10:53 PM
3c2ecd9de531	bff96f1a0da2	360d4c6b21a5	Simon Redman	Add ContactsHelper method for accessing the Data table	Jan 15 2018, 10:51 PM
360d4c6b21a5	c021fc7c03c2	4a7c0657f50d	Simon Redman	Add network packets for requesting contact names given uIDs	Jan 15 2018, 7:29 AM
4a7c0657f50d	e323dd1ec799	e9afbbd878bb	Simon Redman	Add method to ContactsHelper for extracting information from the contacts… (Show More…)	Jan 15 2018, 7:16 AM
e9afbbd878bb	b3b8ddae7531	1ea8ff6290db	Simon Redman	Add package type and handler for requesting unique identifiers for every… (Show More…)	Jan 15 2018, 2:18 AM
1ea8ff6290db	41f7c13d9f17	c23848e49d53	Simon Redman	Add ContactsHelper for reading all of the RAW_CONTACT_IDs which contribute a… (Show More…)	Jan 15 2018, 2:16 AM
c23848e49d53	23a7210882c1	85c51ee7207e	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 34377

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 34377

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