D9690: Add first-cut contacts-reading plugin

[Matthijs Tijink]

mtijink added a comment.


  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.

INLINE COMMENTS

> ContactsPlugin.java:106
> +            // The reply is formatted as a series of:
> +            // <int>: Name, Category, Number
> +            // Where int is a unique (incrementing) integer,

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.

> ContactsPlugin.java:140
> +
> +                    int nameIndex = contactsCursor.getColumnIndex(ContactsContract.Contacts.DISPLAY_NAME);
> +

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.

> ContactsPlugin.java:169
> +                                    , ContactsContract.CommonDataKinds.Phone.NUMBER
> +                                    , ContactsContract.CommonDataKinds.Phone.TYPE
> +                                    // Stores the label of the type of the number

Maybe e-mail addresses are useful too?

> ContactsPlugin.java:236
> +        {
> +            Log.e("ContactsPlugin", "Contacts plugin received an unexpected packet!");
> +            return false;

Maybe better to move this check to the beginning of the function: that improves readability and reduces indenting.

REPOSITORY
  R225 KDE Connect - Android application

REVISION DETAIL
  https://phabricator.kde.org/D9690

To: sredman, kdeconnect
Cc: mtijink
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdeconnect/attachments/20180110/e8f79ecd/attachment.html>