-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle runtime permissions in K-9 #2110
Comments
The three commits I have so far cover:
Next I need to look at the RecipientSelectView stuff for reading contacts while writing email. If anyone has any feedback about when we should request Contacts access please leave your comments. |
Switching to runtime permissions is a serious change in the user experience. I think we should figure out how to do this properly before attempting to write any code. This is something for the design overhaul, not something we do in between. I think the only two permissions we need to worry about are contacts and storage permission. I'm not sure yet what the best time to request the contacts permission is. For an email app it's quite useful to have, but not essential. Asking for it during onboarding might seem strange to users because it's not used right away. Maybe we wait until the user starts composing a message. Ideally, I want to get rid of the storage permission (on newer Android versions). I believe the storage access framework should make this possible for all use cases other than moving the account database to external storage. Since nowadays "external storage" really is on internal storage anyway, I think we can get rid of that "feature", too. |
For me, the UX problem with permissions (in general) is that it's hard to make clear why the permission is requested and the consequences. For contacts, I agree that technically one could use K-9 without contacts access, but that seems like a fringe case even to me, because email contents seem more likely to be sensitive than contacts, and if one doesn't trust K-9 to behave, there's no hope. But, I could see presenting a screen that says "K-9 is about to request access to contacts, which will be used to let you choose who to mail to and [other]. If you do not permit access to contacts, K-9 will still work, but you will have to type all email addresses exactly, with no autocompletion". Then a "continue" button to launch the permissions dialog. I'm not 100% clear on storage, but one thing K-9 does that I think is important to retain is to export settings to external storage. (I realize "external" is often internal, but to me the point is that this storage can be read by other apps so you can copy files off easily, e.g. using syncthing to sync the entire external storage to a regular computer.) But, it seems fine to defer asking for permission to do that until it is needed. |
Exporting settings using the storage access framework (SAF) will allow you to select the destination. That includes device storage, device-external storage and everything else that hooks into SAF, e.g. Google Drive. |
I'm all in favour of improving use of storage. But if we have to perfect our use of storage in order to update the targetSdkVersion (which we have to do to solve Doze) then that feels very broken to me. The commits I have make the app not crash with runtime permissions set. They ask for Storage permissions sensibly to enable the way it currently behaves to work and they don't bother asking for Contacts permissions yet. I figure we make the app ask for contact permissions on upgrade and then we open an issue up to make better use of the storage architecture. Leaving the app stuck on 22 (and breaking core functionality like sync) until we get round to handling a very low priority issue (for me at least) of exporting settings seems absurd.
That's pretty much the advice of the Android documentation. Course this makes this issue even larger (and so less likely be to resolved). I'm much more in favour of incremental improvements that make the app better each time and can lead to rapid user feedback than massive sweeping changes that take ages to get released and then still have to be iterated on anyway following user feedback. We can go back and add these screens later. |
I think that Contacts is something that should be asked upon first opening of the app (either after installation or upgrade). And I also agree with @gdt with this message, but as long as this doesn’t go into oblivion, I think @philipwhiuk is right that releasing a first version making use of runtime permissions and keep that message for later is the way to go. |
Is there a reason we can't have runtime permissions even while targetting api level < 23? Users can revoke permissions, and we can handle them in the more clear cases (like saving attachments) even if we don't fully support them in all places yet. That'll allow us to handle some more situations than we do right now, and yield better feedback for those implementations. |
I don't understand what you're asking. Runtime permissions can't be used without targetSdkVersion=23 or higher. |
I'm refering to this comment in the docs:
which sounds like we can check permissions even with lower sdk, we just don't have to? Maybe not. |
That'll trigger a compatibility behavior. E.g. when querying contacts you'll get an empty list, rather than a SecurityException. |
Please mind that "runtime permission management" (the ability for users to retract permissions granted upon installation of an app) is also relevant for API levels < 26: Thus please make the proper handling of retracted permissions available on all API levels supported by K-9 Mail. |
Yes, but AFAIK in Cyanogenmod/LineageOS apps cannot influence/work with the system there, i.e. there are no extra/special APIs for that. It's just you can pay attention your app does not crash when a permission is not there, which you might expect, but I guess that works. |
Ack.
Yeah, it would be really nice if K-9 Mail would do that (instead of crashing)! |
Exactly.
Does it do so? |
@rugk, yes, see issue #3498 and the original report by @philipwhiuk above, in the section Actual Behavior. |
I haven't looked at the state of the current build or touched k9 for months but if I remember correctly I had fixed this back in January and was rejected. See commits here. |
The UX of requesting permissions can certainly be improved. But I think we have the functionality covered now. Closing this issue. |
Expected behaviour
K-9 should:
Actual behavior
If you try and save an attachment with no Storage permission
Steps to reproduce
Environment
K-9 Mail version: e238ee5
Android version: 7.1.1
Account type (IMAP, POP3, WebDAV/Exchange): N/A
The text was updated successfully, but these errors were encountered: