Differential D9690

Add contacts-reading plugin (Android side)
ClosedPublic
Actions

Authored by sredman on Jan 6 2018, 1:48 AM.

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
master
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
mtijink added a subscriber: mtijink.Jan 10 2018, 7:24 PM

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.

sredman added a comment.Jan 14 2018, 7:31 PM
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!

sredman updated this revision to Diff 25360.Jan 15 2018, 2:20 AM

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.

sredman updated this revision to Diff 25364.Jan 15 2018, 7:36 AM

Add network packets for requesting contact names given uIDs

  • Add method to ContactsHelper for extracting information from the contacts database given raw contact ids
sredman updated this revision to Diff 25434.Jan 16 2018, 1:14 AM

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.
andyholmes added a subscriber: andyholmes.Mar 3 2018, 4:38 PM

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.

sredman marked 6 inline comments as done.Mar 3 2018, 7:06 PM
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

andyholmes added a comment.Mar 3 2018, 7:11 PM

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
sredman updated this revision to Diff 30532.Mar 25 2018, 5:15 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
sredman updated this revision to Diff 30538.Mar 25 2018, 6:26 PM

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.
Restricted Application added a project: KDE Connect. · View Herald TranscriptMar 27 2018, 4:39 PM
sredman reopened this revision.Mar 27 2018, 4:41 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)
mtijink added a comment.Mar 27 2018, 6:59 PM

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.

nicolasfella added a subscriber: nicolasfella.Mar 27 2018, 7:20 PM
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

sredman updated this revision to Diff 30770.EditedMar 28 2018, 12:47 AM

Update diff with all changes

sredman updated this revision to Diff 30773.Mar 28 2018, 1:16 AM

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
sredman updated this revision to Diff 30780.Mar 28 2018, 4:56 AM

Remove SMSHelper from unrelated diff

mtijink requested changes to this revision.Mar 29 2018, 8:01 PM

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

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

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
sredman updated this revision to Diff 30899.Mar 30 2018, 5:08 AM
sredman marked 2 inline comments as done.

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

sredman added a comment.Mar 30 2018, 5:10 AM

Thanks for the eyeballs!

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

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?

sredman updated this revision to Diff 30900.Mar 30 2018, 5:18 AM

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

sredman updated this revision to Diff 31072.Mar 31 2018, 9:43 PM

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
221

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
261

FileNotFoundException is an IOException, so one catch is enough

Restricted Application added a subscriber: kdeconnect. · View Herald TranscriptMay 12 2018, 4:55 PM
sredman updated this revision to Diff 34377.May 17 2018, 3:44 PM
sredman marked 5 inline comments as done.

Address reviewer comments

res/values/strings.xml
221

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

sredman updated this revision to Diff 34383.May 17 2018, 5:37 PM

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
106

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
106

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
106

The plugin min is 18

sredman added inline comments.May 31 2018, 12:15 AM
src/org/kde/kdeconnect/Helpers/ContactsHelper.java
106

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)

nicolasfella requested changes to this revision.May 31 2018, 12:26 AM

It does not build

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

+ ID instead of + uID?

This revision now requires changes to proceed.May 31 2018, 12:26 AM
sredman updated this revision to Diff 35225.May 31 2018, 12:37 AM
sredman marked an inline comment as done.

Fix error created when cleaning warnings
Reword permission request string

sredman added a comment.May 31 2018, 12:37 AM

Ug. Whoops. Yes, ID is correct

nicolasfella accepted this revision.May 31 2018, 12:49 AM

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

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.

Revision Contents
Changeset List

Diff 25360

View Options

res/values/strings.xml

Loading...
View Options

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

Loading...
View Options

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

Loading...
View Options

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

Loading...