Add contacts-reading plugin (KDE side)
ClosedPublic

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

Details

Summary

Add a plugin to KDE Connect which supports exporting the Android contacts databases to vcards on the desktop

When the devices are connected, the plugin sends a request for all timestamps and IDs

When a packet with timestamps and IDs is received, it verifies it has vcards for each ID and that the timestamps match and deletes any vcards for IDs which were not reported. It then sends a request for all vcards which were missing or need updating

When a packet with vcards is received they are unconditionally written to disk, possibly overwriting existing vcards

Provides one dbus method: contacts/synchronizeRemoteWithLocal which triggers the request for all timestamps and IDs

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
R224 KDE Connect
Branch
contacts
Lint
No Linters Available
Unit
No Unit Test Coverage
sredman requested review of this revision.Jan 6 2018, 1:52 AM
sredman created this revision.
sredman updated this revision to Diff 25361.Jan 15 2018, 2:27 AM

Add dbus interface for requesting unique IDs for accessing the contacts

  • Attach dbus to the correct path
sredman updated this revision to Diff 25363.Jan 15 2018, 7:35 AM

Add interface for requesting contact names given uIDs

  • Change return type of getAllContactUIDs to int instead of string
sredman updated this revision to Diff 25435.Jan 16 2018, 1:15 AM

Add interface for requesting phone numbers and email addresses

  • Add PhoneEntry container class for all the data associated with a phone number
  • Add interface for requesting phone numbers given a list of uIDs
  • Remove old interface for requesting random jumble of contact names + phone numbers
  • Combine sender methods into single helper
  • Add locks when handling a phone number response packet
  • Stop manually setting up DBus connection, since it is already handled
  • Add package type and handler for requesting contact phone numbers given a list of uIDs
  • Reorder methods
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)
apol edited the summary of this revision. (Show Details)Jan 16 2018, 8:19 PM
apol added a subscriber: apol.

The DBus API seems too convoluted, I suggest using vcard for now, which will be understood by every PIM suite in existance.

apol requested changes to this revision.Mar 16 2018, 10:23 PM
This revision now requires changes to proceed.Mar 16 2018, 10:23 PM
This revision was not accepted when it landed; it landed in state Needs Revision.Mar 24 2018, 1:20 PM
This revision was automatically updated to reflect the committed changes.
sredman reopened this revision.Mar 25 2018, 5:16 PM
sredman updated this revision to Diff 30533.Mar 25 2018, 5:18 PM

Update contacts plugin for VCard API

  • Combine sender methods into single helper
  • Strip down contacts plugin for new VCards api
  • Merge remote-tracking branch 'origin/master' into contacts
  • Write VCards to local files
  • Move vcards storage path to class-level
  • Implment reading local cache for missing and remove unreported contacts
  • Update locally-cached vcards if the timestamp differs
  • Synchronize contacts when plugin loads
sredman updated this revision to Diff 30539.Mar 25 2018, 6:28 PM

Cleanup TODOs

This revision was not accepted when it landed; it landed in state Needs Review.Mar 25 2018, 6:28 PM
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
sredman retitled this revision from Add contacts-reading plugin to Add contacts-reading plugin (KDE 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)
sredman added a subscriber: KDE Connect.
sredman updated this revision to Diff 30769.Mar 28 2018, 12:43 AM

Update diff with ALL changes. Arc is too powerful for me!

  • Add first-cut contacts-reading plugin
  • Add dbus interface for requesting unique IDs for accessing the contacts
  • Attach dbus to the correct path
  • Change return type of getAllContactUIDs to int instead of string
  • Add interface for requesting contact names given uIDs
  • Add PhoneEntry container class for all the data associated with a phone number
  • Add interface for requesting phone numbers given a list of uIDs
  • Remove old interface for requesting random jumble of contact names + phone numbers
  • Combine sender methods into single helper
  • Add locks when handling a phone number response packet
  • Stop manually setting up DBus connection, since it is already handled
  • Add package type and handler for requesting contact phone numbers given a list of uIDs
  • Reorder methods
  • Change NetworkPackage to NetworkPacket
  • Strip down contacts plugin for new VCards api
  • Print VCards
  • Add dependency on KPeople for contacts plugin
  • Revert "Add dependency on KPeople for contacts plugin"
  • Write VCards to local files
  • Move vcards storage path to class-level
  • Implment reading local cache for missing and remove unreported contacts
  • Update locally-cached vcards if the timestamp differs
  • Synchronize contacts when plugin loads
  • Upgrade TODOs to action items

Same here.

Okay. I am now more confident that I have done this correctly :)

apol requested changes to this revision.Mar 28 2018, 12:08 PM

Some nitpicking... looking much better.
Please extrapolate the comments to similar situations.

plugins/contacts/contactsplugin.cpp
72

in KDE Connect we don't add a new line after the if:
} else if () {

Fix others too.

103

mark uIDs const

110

const QString & ID

181

auto &

plugins/contacts/contactsplugin.h
81

How about:

typedef qint64 UID;

typedef QList<uID_t> uIDList_t;
145

const QString &

155

const QString & and uIDs

This revision now requires changes to proceed.Mar 28 2018, 12:08 PM
mtijink added inline comments.Mar 29 2018, 7:45 PM
plugins/contacts/contactsplugin.cpp
71

It's not really common in C++ to write this->.

plugins/contacts/contactsplugin.h
70

You can get the uid's by calling map.keys(), so the separate list is not needed.

76

Wouldn't kdeconnect/vcard make more sense?

123

Why protected? We're not going to inherit from this, right?

plugins/contacts/kdeconnect_contacts.json
14

How will the contact integrate with the desktop? E.g. will I see the contacts in KAddressBook? Or can I sent people e-mails using this?

If the contacts conflict with what the user has already set up, I'd disable it by default. Otherwise, this is fine.

sredman updated this revision to Diff 30898.Mar 30 2018, 5:07 AM

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

In D9691#235891, @apol wrote:

Some nitpicking... looking much better.
Please extrapolate the comments to similar situations.

Nitpicking is good. That is how I learn!

plugins/contacts/contactsplugin.h
76

The KPeople plugin KPeopleVCard expects vcards to be in that folder

123

The future is uncertain... Maybe someone wants to make a BetterContactsPlugin which supports more packets but lives alongside this plugin? I don't see any reason to stop them.

plugins/contacts/kdeconnect_contacts.json
14

vcards are written to the folder where KPeopleVCard expects to find them. Then, once KPeople and KPeopleVCard are properly configured, any application which uses KPeople will automatically find the contacts

Writing contacts back to the phone is not (yet) supported

Kpeople supports merging contacts, and each KDE Connect device writes to its own folder, so if multiple contacts with the same information exist they can be merged within KPeople

sredman marked 6 inline comments as done.Mar 30 2018, 5:09 AM
sredman added inline comments.Mar 30 2018, 5:13 AM
plugins/contacts/contactsplugin.cpp
71

Does it do any harm? It's a habit from writing too much Python (which I'd rather keep so I can still writing Python) and being overly clear never hurt anybody

plugins/contacts/contactsplugin.h
70

I believe this comment meant to apply somewhere else in the code? But in general I like to give names to things so that other lines are shorter and clearer

sredman updated this revision to Diff 31071.Mar 31 2018, 9:42 PM

Emit signal reporting changed vcards

sredman marked 11 inline comments as done.Apr 18 2018, 7:04 PM
apol added a comment.Apr 18 2018, 10:44 PM

Patch looks good to me, I'd say let's merge it so we can start testing.

plugins/contacts/contactsplugin.h
123

Well that's not a great reason. If we ever need more abstraction we'll have to modify the code, and that's fine.
I'm not concerned about this though.

Note that public before Q_SIGNAL it does nothing and can be removed.

apol accepted this revision.Apr 18 2018, 10:44 PM
This revision is now accepted and ready to land.Apr 18 2018, 10:44 PM
This revision was automatically updated to reflect the committed changes.