You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by mad-nuts <gi...@git.apache.org> on 2017/01/06 14:33:36 UTC
[GitHub] cordova-plugin-contacts pull request #146: CB-12326
GitHub user mad-nuts opened a pull request:
https://github.com/apache/cordova-plugin-contacts/pull/146
CB-12326
fix:
Crash on Android: CommonDataKinds.*.LABEL
<!--
Please make sure the checklist boxes are all checked before submitting the PR. The checklist
is intended as a quick reference, for complete details please see our Contributor Guidelines:
http://cordova.apache.org/contribute/contribute_guidelines.html
Thanks!
-->
### Platforms affected
Android
### What does this PR do?
Adding the missing "LABEL" CommonDataKinds columns to the ContactAccessorSdk5 to fix the app crash issue.
Moving the label type queries inside the try catch blocks, replacing the "getColumnIndex" with "getColumnIndexOrThrow" and catching the IllegalArgumentExceptions handler to make the code more solid.
### What testing has been done on this change?
Tested on Samsung Galaxy S6
### Checklist
- [ Yes ] [Reported an issue](http://cordova.apache.org/contribute/issues.html) in the JIRA database
- [ Yes ] Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
- [ No ] Added automated test coverage as appropriate for this change.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mad-nuts/cordova-plugin-contacts master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-plugin-contacts/pull/146.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #146
----
commit 3bb509e1f473531354b70fd299ba2003905d3f5b
Author: andreas.kausler <an...@camao.de>
Date: 2017-01-06T13:59:21Z
CB-12326
fix:
Crash on Android: CommonDataKinds.*.LABEL
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-contacts issue #146: CB-12326
Posted by filmaj <gi...@git.apache.org>.
Github user filmaj commented on the issue:
https://github.com/apache/cordova-plugin-contacts/pull/146
Oh, and @mad-nuts have you signed an Apache ICLA? (individual contributor license agreement)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-contacts issue #146: CB-12326
Posted by filmaj <gi...@git.apache.org>.
Github user filmaj commented on the issue:
https://github.com/apache/cordova-plugin-contacts/pull/146
It _does_ look like [iOS returns label information](https://github.com/apache/cordova-plugin-contacts/blob/master/src/ios/CDVContact.h#L92) in contacts, so there is precedent to enhance Android to do that as well.
> Adding the missing "LABEL" CommonDataKinds columns to the ContactAccessorSdk5 to fix the app crash issue.
@mad-nuts can you elaborate on what 'the app crash issue' is? I'm curious what scenario caused a crash.
@infil00p if you have a moment, take a look at the generic try/catch flow this pull request introduces and let us know if you have any comments on that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-contacts issue #146: CB-12326
Posted by mad-nuts <gi...@git.apache.org>.
Github user mad-nuts commented on the issue:
https://github.com/apache/cordova-plugin-contacts/pull/146
@filmaj I haven' t signed the Apache ICLA yet. I'll have a look into it (as well as the rebase & force-push) within the next few days and if I'm comfortable with it and it doesn't include a contract to buy a refrigerator or forces me to leave all my intellectual property to Apache, I'll probably sign it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-contacts issue #146: CB-12326
Posted by mad-nuts <gi...@git.apache.org>.
Github user mad-nuts commented on the issue:
https://github.com/apache/cordova-plugin-contacts/pull/146
I've tried for hours to reproduce the error, but the app just won't crash anymore.
Something must have changed since then and I don't know what it is...
Despite the fact, that the app won't crash anymore, the code is still buggy and should be fixed some day.
**Here is what had happened in detail, maybe it is useful nevertheless:**
Executing `ContactAccessorSdk5.search` with one or more of the following fields caused the crash:
```
- navigator.contacts.fieldType.phoneNumbers
- phoneQuery
- CommonDataKinds.Phone.LABEL
- navigator.contacts.fieldType.emails
- emailQuery
- CommonDataKinds.Email.LABEL
- navigator.contacts.fieldType.postalCode
- addressQuery
- CommonDataKinds.StructuredPostal.LABEL
- navigator.contacts.fieldType.organizations
- organizationQuery
- CommonDataKinds.Organization.LABEL
- navigator.contacts.fieldType.urls
- websiteQuery
- CommonDataKinds.Website.LABEL
```
- `ContactAccessorSdk5.search` is called with the `JSONArray fields` containing one / some / all fields of the above
- the contentResolver projection `columnsToFetch` is created by adding the required columns for each passed field (`columnsToFetch.add(...)`)
- but without adding the columns for `CommonDataKinds.[...].LABEL` to `columnsToFetch`
- `columnsToFetch `is fed to `mApp.getActivity().getContentResolver().query(...)`, resulting in Cursor `c`
- at this state the cursor is already missing the later required `LABEL` columns
- `populateContactArray(limit, populate, c);` is called
- within the `while` loop query methods are executed to fetch the required fields from the cursor.
- in those query methods `getColumnIndex` is called to retrieve the none existing `LABEL` column, returning `-1`
- `cursor.getString(-1)` is called
- returns nullable now, which is fine
- threw an `java.lang.IllegalStateException` back then
`getContactById` is using `populateContactArray `too, but should be fine for now, because "null" is passed as the projection param, which returns a cursor, pointing to all columns.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-contacts issue #146: CB-12326
Posted by cordova-qa <gi...@git.apache.org>.
Github user cordova-qa commented on the issue:
https://github.com/apache/cordova-plugin-contacts/pull/146
Cordova CI Build has completed successfully.
**Commit** - [Link](https://github.com/apache/cordova-plugin-contacts/pull/146/commits/3bb509e1f473531354b70fd299ba2003905d3f5b)
**Dashboard** - [Link](http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-contacts-pr/71/)
| Builder Name | Console Output | Test Report | Device Logs |
| :---: | :---: | :---: | :---: |
| [Windows 10 Store]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-contacts-pr/71//PLATFORM=windows-10-store/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-contacts-pr/71//PLATFORM=windows-10-store/console) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-contacts-pr/71//PLATFORM=windows-10-store/testReport/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-contacts-pr/71//PLATFORM=windows-10-store/artifact/) |
| [iOS]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-contacts-pr/71//PLATFORM=ios/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-contacts-pr/71//PLATFORM=ios/console) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-contacts-pr/71//PLATFORM=ios/testReport/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-contacts-pr/71//PLATFORM=ios/artifact/) |
| [Android 4.4]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-contacts-pr/71//PLATFORM=android-4.4/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-contacts-pr/71//PLATFORM=android-4.4/console) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-contacts-pr/71//PLATFORM=android-4.4/testReport/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-contacts-pr/71//PLATFORM=android-4.4/artifact/) |
| [Android 5.1]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-contacts-pr/71//PLATFORM=android-5.1/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-contacts-pr/71//PLATFORM=android-5.1/console) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-contacts-pr/71//PLATFORM=android-5.1/testReport/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-contacts-pr/71//PLATFORM=android-5.1/artifact/) |
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-contacts issue #146: CB-12326
Posted by filmaj <gi...@git.apache.org>.
Github user filmaj commented on the issue:
https://github.com/apache/cordova-plugin-contacts/pull/146
Thanks for the review @infil00p !
@mad-nuts can you do me a favour and rebase w/ the latest master and force-push up to your branch, for one more CI / cordova-qa run, please?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-contacts pull request #146: CB-12326
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cordova-plugin-contacts/pull/146
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-contacts issue #146: CB-12326
Posted by infil00p <gi...@git.apache.org>.
Github user infil00p commented on the issue:
https://github.com/apache/cordova-plugin-contacts/pull/146
Honestly, the Contacts API on Android itself is buggy and unpredictable. Just run adb logcat without debugging anything and you'll see a stack trace from Facebook, Twitter, Snapchat or some other app that has access to contacts on your device in less than an hour.
In short, good luck reproducing a lot of these Contacts bugs. I'll take your word that things are broken and accept this one.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org