Fix permission handling in Multimedia editor #5841

david-allison · 2020-03-20T19:47:41Z

Purpose / Description

Editing a note, selecting "Record Audio", with denied & remembered permissions would soft crash the app.

Fixes

Approach

Explained in #5407

Define 3 transitions (between Multimedia UIs):

Init - initial load

UI - Menu Item is clicked, (changing from "Record Audio" to "Insert Image").

Treat this as cancellable

Activity - Pronunciation calls "Record Audio" with a known clip to allow for preview and pronunciation recreation.

This is invoked by the caller, so there's no way to know whether the previous UI is in a valid state. Treat this is non-cancellable.

Allow "Record Audio" to fail a transition

UI - Failing will keep the application on the previous multimedia UI.

Requires changes to avoid destroying the previous context until the new context validates.

Init - Cancel the Activity

Activity - Allow access to "Record Audio" without permissions

Save the state of a transition to allow the async onRequestPermissionsResult to reapply the context.

Mark the state as having "permissions requested", so they're not re-requested when calling recreateEditingUi

Show a toast notification on "Permission Denied"

Ensure "Record Audio" can be used without permissions.

How Has This Been Tested?

Deny permissions:
Selecting from the attachment fails and takes you back to the main screen/
A preview from pronunciation succeeds

Toasts are displayed on permissions issues.

Checklist

You have not changed whitespace unnecessarily (it makes diffs hard to read)
You have a descriptive commit message with a short title (first line, max 50 chars).
Your code follows the style of the project (e.g. never omit braces in if statements)
You have commented your code, particularly in hard-to-understand areas
You have performed a self-review of your own code

david-allison · 2020-03-20T19:48:27Z

I worry this is too complex, I hope splitting this into commits makes it easier to review.

david-allison · 2020-03-20T20:55:01Z

I'll get on with the auto fixes tomorrow

AnkiDroid/src/main/res/values/03-dialogs.xml

mikehardy

I thought the separate commits made it pretty easy to review. I don't mind larger PRs especially if they show the transform in bite-size chunks like that - I keep the big diff open on one tab to make sure a comment I'd write isn't addressed somewhere later, and then flip through each commit - no problem.
Lots of little things I think but nothing serious that seemed off, and if it works, it works - I'll pull it and test it after this round
Good work digging through the advanced editor it's not easy to reason about

AnkiDroid/src/main/res/values/03-dialogs.xml

AnkiDroid/src/main/java/com/ichi2/anki/multimediacard/utils/Permissions.java

AnkiDroid/src/main/java/com/ichi2/anki/multimediacard/activity/MultimediaEditFieldActivity.java

We can get to the AudioView screen from the pronunciation screen, this is a one-way transition, and should succeed, even if we can't use the screen to record audio.

Previously, we would get into a loop of asking for permissions if the user ticked "deny & remember". Now we close the dialog and cancel the activity. We now only try and redraw if we were successful. Fixes ankidroid#5407

If we went through the permission check once, we don't need to go through it again

Previously, onLostFocus would kill the previous activity. If we fail navigation due to permissions, then we had nowhere to go Now, we only perform this if we have permission and can create a field controller

Co-Authored-By: Mike Hardy <[email protected]>

david-allison · 2020-03-22T11:22:45Z

Review fixes done.

timrae · 2020-03-22T11:24:52Z

Here is an overview of what got changed by this pull request:

Issues
======
- Added 1
           

Complexity increasing per file
==============================
- AnkiDroid/src/main/java/com/ichi2/utils/Permissions.java  1

See the complete overview on Codacy

AnkiDroid/src/main/java/com/ichi2/utils/Permissions.java

mikehardy · 2020-03-22T16:05:10Z

Same as the other - this one looks great just waiting on CI to go through owing to macOS intermittency 💪

mikehardy · 2020-03-22T20:17:20Z

2.10alpha54 released just now has this whenever google delivers it to you

mikehardy · 2020-03-26T23:44:23Z

released in 2.9.6beta4

mikehardy added the Needs Author Reply Waiting for a reply from the original author label Mar 21, 2020

mikehardy reviewed Mar 22, 2020

View reviewed changes

AnkiDroid/src/main/res/values/03-dialogs.xml Show resolved Hide resolved

mikehardy requested changes Mar 22, 2020

View reviewed changes

david-allison and others added 21 commits March 22, 2020 10:32

NF: Extract Permissions to Class

614eff0

Ensure we have permissions before audio recording

f0682a4

We can get to the AudioView screen from the pronunciation screen, this is a one-way transition, and should succeed, even if we can't use the screen to record audio.

NF: Extract method: performPermissionRequest

7860f4f

NF: Extract method: setupUIController

f35b3d1

MultimediaEditor: Delay changing field

28b1dd0

Multimedia: Only rerender on permission success

de4ec31

Previously, we would get into a loop of asking for permissions if the user ticked "deny & remember". Now we close the dialog and cancel the activity. We now only try and redraw if we were successful. Fixes ankidroid#5407

Multimedia: Stop second permission check

a49bfab

If we went through the permission check once, we don't need to go through it again

Multimedia: Only kill previous UI if successful

e1b604d

Previously, onLostFocus would kill the previous activity. If we fail navigation due to permissions, then we had nowhere to go Now, we only perform this if we have permission and can create a field controller

Review: Made access modifiers explicit

dacbc44

Review: Rename Class

00f0800

NF: Fix spacing

4c675c3

Review: Fix Advanced Editor Strings

42f042d

Co-Authored-By: Mike Hardy <[email protected]>

Review: Improve Advanced Editor strings

fbdd22f

Co-Authored-By: Mike Hardy <[email protected]>

Review change: Rename method

c3357de

Review change: Added Logging

484bda7

Review change: Assertion Failure

670107f

Review change: Moved Permissions file

d089cd2

Review change: Extract hasStorageAccessPermission

c22db4f

Refactor: use canUseCamera

93da7f5

Refactor: extract canUseWakeLock

d3315ad

Refactor: Improve permissions check

dc6615b

david-allison force-pushed the issue-5407-multimedia-permission-loop branch from 4f74876 to dc6615b Compare March 22, 2020 11:22

timrae reviewed Mar 22, 2020

View reviewed changes

AnkiDroid/src/main/java/com/ichi2/utils/Permissions.java Show resolved Hide resolved

mikehardy removed the Needs Author Reply Waiting for a reply from the original author label Mar 22, 2020

mikehardy approved these changes Mar 22, 2020

View reviewed changes

mikehardy added this to the 2.9.6 Release milestone Mar 22, 2020

mikehardy changed the title ~~Fixes #5407 - Multimedia Permission Denied Soft Crash Loop~~ Fix permission handling in Multimedia editor Mar 22, 2020

mikehardy merged commit eb89b5b into ankidroid:master Mar 22, 2020

david-allison deleted the issue-5407-multimedia-permission-loop branch March 22, 2020 20:28

This was referenced Mar 23, 2020

2.10Alpha54: Crash when pressing "edit field" for img field #5856

Closed

[2.9 ACRA] crash releasing audio recorder #5521

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix permission handling in Multimedia editor #5841

Fix permission handling in Multimedia editor #5841

david-allison commented Mar 20, 2020

david-allison commented Mar 20, 2020

david-allison commented Mar 20, 2020

mikehardy left a comment

david-allison commented Mar 22, 2020

timrae commented Mar 22, 2020

mikehardy commented Mar 22, 2020

mikehardy commented Mar 22, 2020

mikehardy commented Mar 26, 2020

Fix permission handling in Multimedia editor #5841

Fix permission handling in Multimedia editor #5841

Conversation

david-allison commented Mar 20, 2020

Purpose / Description

Fixes

Approach

How Has This Been Tested?

Checklist

david-allison commented Mar 20, 2020

david-allison commented Mar 20, 2020

mikehardy left a comment

Choose a reason for hiding this comment

david-allison commented Mar 22, 2020

timrae commented Mar 22, 2020

mikehardy commented Mar 22, 2020

mikehardy commented Mar 22, 2020

mikehardy commented Mar 26, 2020