Runtime permissions for contacts #3632

GoneUp · 2018-09-30T13:06:41Z

As the title says the pull request is basically a rebased commit of #3176
A thing to note is that the permission request will only show up when the targetsdk is upped >23, which cannot be changed until save attachment is based on SAF or gets a permission request. I think its still good to merge this to get closer to the goal of #2110.

GoneUp · 2018-10-08T22:40:41Z

I fixed the broken travis build, let me know if you want something changed.

Valodim

This is a good step towards runtime permissions, thanks! Left some comments inline

Valodim · 2018-11-21T09:56:42Z

app/core/src/main/java/com/fsck/k9/helper/Contacts.java

@@ -263,6 +267,14 @@ public Uri getPhotoUri(String address) {
        }
    }

+    private Boolean hasPerm() {


why Boolean? This can't return null. The method is also not particularly readable, maybe introduce canRead and canWrite variables and then return canRead && canWrite

The method should also have a more descriptive name than "hasPerm", even if it's just privately used in this class

Valodim · 2018-11-21T09:57:34Z

app/core/src/main/java/com/fsck/k9/helper/Contacts.java

-                SORT_ORDER);
+
+        if (!hasPerm()) {
+            // return blank cursor


This is pretty ugly :( Perhaps create a new EmptyCursor class for it?

Valodim · 2018-11-21T09:58:37Z

app/ui/src/test/java/com/fsck/k9/activity/compose/RecipientLoaderTest.java

+
+    @Test
+    public void queryContactProviderWithoutPermission() throws Exception {
+        shadowApp.denyPermissions(Manifest.permission.READ_CONTACTS);


Valodim · 2018-11-21T10:02:00Z

app/ui/src/test/java/com/fsck/k9/activity/compose/RecipientLoaderTest.java

@@ -47,36 +50,39 @@
            ContactsContract.Data.CONTACT_ID,
            ContactsContract.CommonDataKinds.Nickname.NAME
    };
-    static final String[] PROJECTION_CRYPTO_ADDRESSES = { "address", "uid_address" };
-    static final String[] PROJECTION_CRYPTO_STATUS = { "address", "uid_key_status", "autocrypt_key_status" };
+    static final String[] PROJECTION_CRYPTO_ADDRESSES = {"address", "uid_address"};


there are a lot of unrelated formatting changes in this file. those make it harder to see what actually changed, please avoid this in the future. If the formatting is annoying in some place, it's best to fix it in a simple to review formatting-only PR

Sure, that was just my old habit to use autoformat while coding.

Valodim · 2018-11-21T10:06:36Z

app/ui/src/main/java/com/fsck/k9/activity/K9Activity.java

+                if (grantResults.length > 0
+                        && grantResults[0] == PackageManager.PERMISSION_GRANTED) {
+
+                    Toast.makeText(this, R.string.contact_permission_thanks,


seems unnecessary

The thanks message? I just left that in from the orignal pr, I will remove it.

Valodim · 2018-11-21T10:08:18Z

app/ui/src/main/java/com/fsck/k9/activity/K9Activity.java

+        switch (requestCode) {
+            case PERMISSIONS_REQUEST_READ_CONTACTS:
+            case PERMISSIONS_REQUEST_WRITE_CONTACTS: {
+                // If request is cancelled, the result arrays are empty.


it's nicer to express this in code:

boolean permissionWasGranted = grantResults.length > 0 && grantResults[0] == PackageManager.PERMISSION_GRANTED; if (permissionWasGranted) {

same amount of lines, but in a formal syntax :)

Sure, but in this form it is extendable. I expect at least runtime permissions for storage to be here soon.

Oh, you meant the if statement, changed that.

Valodim · 2018-11-21T10:10:02Z

app/ui/build.gradle

@@ -39,6 +39,7 @@ dependencies {
    implementation "net.jcip:jcip-annotations:1.0"
    implementation "com.jakewharton.timber:timber:${versions.timber}"
    implementation "org.apache.james:apache-mime4j-core:${versions.mime4j}"
+    implementation "org.apache.james:apache-mime4j-dom:${versions.mime4j}"


This is really weird. Is it sufficient to put testImplementation here? Either way, please leave a comment describing why this is necessary

I had a strange error on the testbench without it, I wrote that in the commit message. One of the tests (RecipentLoaderTest) could not find a class that is in org.apache.james:apache-mime4j-dom. Normally I would expect it to pass since the dependency is present (https://github.com/k9mail/k-9/blob/012befd59e08c43caddec47d42f5a5d73ac3c2f1/mail/common/build.gradle#L15).

I will try to reduce it to a testImplementation.

Nvm, works without a extra entry now. Clearing all caches works wonders sometimes ;)

A thing to note is that the permission request will only show up when the targetsdk is upped >23. Old commit message that explains the change: -- The approach was to make it so that any K9Activity can easily request whatever permission in the future. The Contacts permission is now requested in two locations: 1. When a list of Messages is displayed 2. When a new message is first started to be composed. The permission request is displayed ONCE per onCreate(). Any more than this and it got really annoying. A typical user who reads or writes emails WILL see the request, trust me. Once they see the message 2x, they also have the option to block the requests from appearing. If they DECLINE the request (or decline + DENY any further attempts), the app should continues to work, albeit without incorporating contact data (thumbnails, autocomplete, etc.). Contacts may still be added to the Contacts app, as this uses an Intent and does not require any permission. Once the Read Contacts permission is enabled, the app immediately begins to use it. To add other permissions in the future (such as External Storage access), the request can be made in a similar way and the permission request result handled appropriately by just adding it to K9Activity (or overriding in a particular Activity).

…ilable. Without it DefaultAddressParser is not found in the RecipentLoaderTest even though the dependecy is declared in common. Looks like a gradle bug to me.

…duce duplicated code

GoneUp · 2018-11-28T00:25:48Z

Okay, I addressed the feedback and rebased the PR.

Valodim · 2018-11-28T00:45:45Z

looks good to me! will test tomorrow, should be good to merge if it checks out. thanks for your work :)

cketti · 2018-11-28T04:24:54Z

I fixed two issues:

The permission was requested every time the Activity was created, i.e. also after orientation changes. This is rather annoying if a dialog is displayed.
Permanently denying the permission would still display the "explanation toast" every time the Activity was created.

Valodim · 2018-11-28T09:56:23Z

Looks good. Let's merge this now so people can test, we can fix other problems in follow-up PRs. Thanks 👍

GoneUp · 2018-11-28T11:12:45Z

Nice to see it merged. I will look at storage permission next.

cketti · 2018-11-28T12:02:19Z

Nice to see it merged. I will look at storage permission next.

Please note that we plan to mostly get rid of the need to request the storage permission. See #3752.

I believe the only situation where we really need the storage permission is when users decide to move their message database to "external" storage.

GoneUp · 2018-11-28T12:13:13Z

That makes it so much easier. I implemented saving attachments with SAF some time ago (see GoneUp@47f1dc0, or master...GoneUp:runtime_storage), but it became pretty messy with the compatibility behaviour. I will open a PR soon so we can discuss it there.

Valodim suggested changes Nov 21, 2018

View reviewed changes

GoneUp and others added 3 commits November 27, 2018 23:16

Added mime4j-dom dependency in app:ui to make sure the package is ava…

455228f

…ilable. Without it DefaultAddressParser is not found in the RecipentLoaderTest even though the dependecy is declared in common. Looks like a gradle bug to me.

Addressed Review feedback

72c3ac5

GoneUp force-pushed the runtime_contacts branch from 53cfb91 to 72c3ac5 Compare November 27, 2018 23:48

Addressed Review feedback, extracted EmptyCursor to a own class to re…

40143ae

…duce duplicated code

Valodim approved these changes Nov 28, 2018

View reviewed changes

cketti added 2 commits November 28, 2018 03:15

Don't request permission when recreating Activity from saved state

9cc5b0f

Move common permissions code to K9Activity

4a79664

cketti changed the title ~~Implements runtime permissions for contacts, rebase of #3176~~ Runtime permissions for contacts Nov 28, 2018

cketti added 3 commits November 28, 2018 05:16

Use request permission rationale mechanism instead of showing toast

c76d85a

Code style cleanup

1da164b

Increase targetSdkVersion to 23

0c911db

Valodim merged commit 0455157 into thunderbird:master Nov 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Runtime permissions for contacts #3632

Runtime permissions for contacts #3632

GoneUp commented Sep 30, 2018

GoneUp commented Oct 8, 2018

Valodim left a comment

Valodim Nov 21, 2018

Valodim Nov 21, 2018

Valodim Nov 21, 2018

Valodim Nov 21, 2018

Valodim Nov 21, 2018

GoneUp Nov 27, 2018

Valodim Nov 21, 2018

GoneUp Nov 27, 2018

Valodim Nov 21, 2018

GoneUp Nov 27, 2018

GoneUp Nov 27, 2018

Valodim Nov 21, 2018

GoneUp Nov 27, 2018

GoneUp Nov 28, 2018

GoneUp commented Nov 28, 2018

Valodim commented Nov 28, 2018

cketti commented Nov 28, 2018

Valodim commented Nov 28, 2018

GoneUp commented Nov 28, 2018

cketti commented Nov 28, 2018

GoneUp commented Nov 28, 2018 •

edited

Loading

Runtime permissions for contacts #3632

Runtime permissions for contacts #3632

Conversation

GoneUp commented Sep 30, 2018

GoneUp commented Oct 8, 2018

Valodim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GoneUp commented Nov 28, 2018

Valodim commented Nov 28, 2018

cketti commented Nov 28, 2018

Valodim commented Nov 28, 2018

GoneUp commented Nov 28, 2018

cketti commented Nov 28, 2018

GoneUp commented Nov 28, 2018 • edited Loading

GoneUp commented Nov 28, 2018 •

edited

Loading