Simplified permission handling #22

mzorz · 2019-08-09T15:24:16Z

Note: please review and merge #23 first :)

Permission handling was a bit here and there, now all required permissions are requested by the Activity (ComposeLoopFrameActivity) when the user requires the camera preview.

In Camera2BasicHandling we only care to re-request them if not available in openCamera as per a mandatory security warning (the user potentially could have revoked permissions in between, so we need to check for them). Could have avoided the check by adding a @SupressWarnings annotation to the openCamera() method but decided it's safer to still re-check than to avoid warnings, as it's a potential cause of crash if we were to reuse things in the future and this piece of code would be used separately (for example, not contained by this specific Activity with the assumption it will request permissions for us).

This is just some housework to keep things a bit cleaner, we'll get back at permissions handling once we get to actually work on #15

…esn't need to know when to request them

aforcier · 2019-08-12T18:04:47Z

photoeditor/src/main/java/com/automattic/photoeditor/camera/Camera2BasicHandling.kt

@@ -468,7 +454,7 @@ class Camera2BasicHandling : Fragment(), View.OnClickListener,
    private fun openCamera(width: Int, height: Int) {
        val permission = ContextCompat.checkSelfPermission(activity!!, Manifest.permission.CAMERA)
        if (permission != PackageManager.PERMISSION_GRANTED) {


If this is tripped and we request permissions here, we'll get a crash:

Caused by: kotlin.UninitializedPropertyAccessException: lateinit property previewSize has not been initialized at com.automattic.photoeditor.camera.Camera2BasicHandling.createCameraRecordingSession(Camera2BasicHandling.kt:779) at com.automattic.photoeditor.state.BackgroundSurfaceManager.switchCameraPreviewOn(BackgroundSurfaceManager.kt:139) at com.automattic.portkey.compose.ComposeLoopFrameActivity.onRequestPermissionsResult(ComposeLoopFrameActivity.kt:100) at android.app.Activity.dispatchRequestPermissionsResult(Activity.java:7616) at android.app.Activity.dispatchActivityResult(Activity.java:7466) at android.app.ActivityThread.deliverResults(ActivityThread.java:4391) at android.app.ActivityThread.handleSendResult(ActivityThread.java:4440) at android.app.servertransaction.ActivityResultItem.execute(ActivityResultItem.java:49) at android.app.servertransaction.TransactionExecutor.executeCallbacks(TransactionExecutor.java:108) at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:68) at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1816) at android.os.Handler.dispatchMessage(Handler.java:106) at android.os.Looper.loop(Looper.java:193) at android.app.ActivityThread.main(ActivityThread.java:6718) at java.lang.reflect.Method.invoke(Native Method) at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:493) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:858)

because in this case we're calling createCameraRecordingSession() without having called setUpCameraOutputs() first.

(Note that I triggered this by changing the code as a test - I'm not sure if we'd practically ever get to this point without permissions being already checked and granted.)

Permissions are interesting, I also thought we could pretty much just ask for everything in the beginning, but users could modify things on the fly so, we should be checkin for the needed permissions every time (and request them if we find out they're not granted). This may mean a better refactor needs to be done - I think we can leave a proper analysis / optimization for later when we deal with #15

mzorz · 2019-08-12T18:14:12Z

app/src/main/java/com/automattic/portkey/compose/ComposeLoopFrameActivity.kt

+                Manifest.permission.WRITE_EXTERNAL_STORAGE,
+                Manifest.permission.CAMERA
+            )
+            PermissionUtils.requestPermissions(this, permissions)


I guess this array here could be referenced from https://github.com/Automattic/portkey-android/pull/22/files#diff-17873655147830fd3d87310ec402fb49R21

Just mentioning it here as I'll probably change it in a future PR 👍 :)

aforcier · 2019-08-12T18:18:23Z

photoeditor/src/main/java/com/automattic/photoeditor/camera/Camera2BasicHandling.kt

-                            FRAGMENT_DIALOG
-                        )
-            }
+        if (!PermissionUtils.allRequiredPermissionsGranted(activity!!)) {


I'm not sure that this function (onRequestPermissionsResult()) would ever get called, even if we did trigger a permissions request further down (which at the moment will cause a crash anyway). I believe we'd need to use Fragment.requestPermissions() for that to happen, but we're actually using ActivityCompat.requestPermissions, which will only trigger the host activity's callback function.

At least, I haven't been able to trigger it through testing. If my logic is sound maybe we can drop this entirely?

TIL that requesting permissions from the Fragment as opposed to requesting them from the Activity would make a difference - let's remove that piece of code for now, and we'll get back at reviewing permissions in a more holistic way in #15 👍

After testing for some time, removed this piece of code in e2fe8ee - noting here there's still some possibility of crashing due to other reasons when preview/recording start/stop state handling is not reflecting the actual current state as per backgroundSurfaceManager.switchCameraPreviewOn() only keeps flags and assumes things are going to be ok. Deferring a refactor of that part for later, as it exceeds this PR.

aforcier · 2019-08-12T18:23:17Z

photoeditor/src/main/java/com/automattic/photoeditor/util/PermissionUtils.kt

+        }
+    }
+
+    class ConfirmationDialog : DialogFragment() {


This appears to be unused at the moment, was that intentional?

Thanks for pointing that out - I moved this Dialog from a separate file into the PermissionUtils class and have plans for it as we tackle #15 - but we could be removing it entirely right now so, removed in fe7ee0d

aforcier · 2019-08-12T18:27:49Z

photoeditor/src/main/java/com/automattic/photoeditor/camera/Camera2BasicHandling.kt

@@ -468,7 +454,7 @@ class Camera2BasicHandling : Fragment(), View.OnClickListener,
    private fun openCamera(width: Int, height: Int) {
        val permission = ContextCompat.checkSelfPermission(activity!!, Manifest.permission.CAMERA)
        if (permission != PackageManager.PERMISSION_GRANTED) {
-            requestCameraPermission()
+            PermissionUtils.requestAllRequiredPermissions(activity!!)


We can get rid of the force unwrap !! here and above and just wrap the whole body of this function in activity.let {}:

activity?.let { activity -> ... }

👍 thanks for the observation, addressed in 16d9a41

aforcier · 2019-08-12T18:30:09Z

photoeditor/src/main/java/com/automattic/photoeditor/util/PermissionUtils.kt

-                    grantResults[0] == PackageManager.PERMISSION_GRANTED,
-                    permissions[0]
-                )
+                PERMISSION_REQUEST_CODE -> {


Can lift the return out here:

return when (requestCode) { PERMISSION_REQUEST_CODE -> { onRequestPermissionChecker.isPermissionGranted( grantResults[0] == PackageManager.PERMISSION_GRANTED, permissions[0] ) true } else -> false }

👍 da2e480

Looks like you missed the return when part, once you add that 20033ac isn't needed anymore (and I think at the moment the earlier true and falses aren't doing anything).

🤕 🤦‍♂ fixed in 733f9d4

aforcier · 2019-08-12T18:31:02Z

Looks good @mzorz, left a few small comments 👍

mzorz · 2019-08-13T18:09:19Z

Ready for another round @aforcier ! Thanks for your comments, made me look closer 🙇
Also worth noting, planning to further work on this in detail when we tackle #15

aforcier · 2019-08-13T18:24:23Z

photoeditor/src/main/java/com/automattic/photoeditor/camera/Camera2BasicHandling.kt

+            }
+            setUpCameraOutputs(width, height)
+            configureTransform(width, height)
+            val manager = activity!!.getSystemService(Context.CAMERA_SERVICE) as CameraManager


Forgot to remove the !! here :D (it's now just unnecessary though, it's using the correct activity that we know is not null)

ah got it, fixed in c1b239e

aforcier · 2019-08-13T18:25:26Z

Looks good! Two more small follow-ups: #22 (comment) and #22 (comment).

aforcier · 2019-08-13T19:02:52Z

simplified permission handling a bit, now BackgroundSurfaceManager do…

c7ae505

…esn't need to know when to request them

mzorz requested a review from aforcier August 9, 2019 15:24

This was referenced Aug 9, 2019

Introduce mp4composer lib as a module #18

Merged

Convert mp4composer to Kotlin #21

Merged

aforcier self-assigned this Aug 12, 2019

aforcier added this to the Demo 1: Basic Creation milestone Aug 12, 2019

aforcier reviewed Aug 12, 2019

View reviewed changes

mzorz commented Aug 12, 2019

View reviewed changes

aforcier reviewed Aug 12, 2019

View reviewed changes

aforcier changed the base branch from app/foundation-add-mp4composer-kotlin to app/foundation August 12, 2019 21:49

mzorz added 5 commits August 13, 2019 13:30

removed unused Dialog

fe7ee0d

get rid of force unwrap

16d9a41

removed return statement

da2e480

added needed default return statement

20033ac

removed onRequestPermissionsResult handling in fragment

e2fe8ee

Merge branch 'app/foundation' into app/foundation-permissions-refactor

3f0ad4b

aforcier reviewed Aug 13, 2019

View reviewed changes

mzorz mentioned this pull request Aug 13, 2019

App foundation: introducing CameraX #23

Merged

mzorz added 2 commits August 13, 2019 15:57

fixed return when statement

733f9d4

removed unnecessary null unwrap

c1b239e

aforcier merged commit 9bf8692 into app/foundation Aug 13, 2019

aforcier deleted the app/foundation-permissions-refactor branch August 13, 2019 19:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplified permission handling #22

Simplified permission handling #22

mzorz commented Aug 9, 2019 •

edited

Loading

aforcier Aug 12, 2019

mzorz Aug 12, 2019

mzorz Aug 12, 2019

mzorz Aug 12, 2019

aforcier Aug 12, 2019

mzorz Aug 13, 2019

mzorz Aug 13, 2019

aforcier Aug 12, 2019

mzorz Aug 13, 2019

aforcier Aug 12, 2019

mzorz Aug 13, 2019

aforcier Aug 12, 2019

mzorz Aug 13, 2019

aforcier Aug 13, 2019

mzorz Aug 13, 2019

aforcier commented Aug 12, 2019

mzorz commented Aug 13, 2019

aforcier Aug 13, 2019

mzorz Aug 13, 2019

aforcier commented Aug 13, 2019

aforcier commented Aug 13, 2019

Simplified permission handling #22

Simplified permission handling #22

Conversation

mzorz commented Aug 9, 2019 • edited Loading

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

aforcier commented Aug 12, 2019

mzorz commented Aug 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aforcier commented Aug 13, 2019

aforcier commented Aug 13, 2019

mzorz commented Aug 9, 2019 •

edited

Loading