Fix media selection in Aztec editor #22442
+275
−28
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| package org.wordpress.android.ui.posts.editor | ||
|
|
||
| import android.app.Activity | ||
| import org.wordpress.android.util.PermissionUtils | ||
| import org.wordpress.android.util.WPPermissionUtils | ||
|
|
||
| /** | ||
| * Helper object for camera-related permission handling in editor activities. | ||
| * This centralizes the camera permission logic to avoid duplication between | ||
| * EditPostActivity and GutenbergKitActivity. | ||
| */ | ||
| object EditorCameraHelper { | ||
| /** | ||
| * Checks for camera permissions and launches the camera if granted. | ||
| * If permissions are not granted, requests them from the user. | ||
| * | ||
| * @param activity The activity requesting the permission | ||
| * @param launchCamera Callback to launch the camera when permission is granted | ||
| */ | ||
| fun checkCameraPermissionAndLaunch(activity: Activity, launchCamera: () -> Unit) { | ||
| if (PermissionUtils.checkAndRequestCameraAndStoragePermissions( | ||
|
Comment on lines
+20
to
+21
Contributor
There was a problem hiding this comment. Critical Issue: This is likely the source of the crash reported by @dcalhoun.
Suggested fix: fun checkCameraPermissionAndLaunch(activity: Activity, launchCamera: () -> Unit) {
// Use ActivityCompat which properly handles Activity permissions
if (ActivityCompat.checkSelfPermission(activity, Manifest.permission.CAMERA)
== PackageManager.PERMISSION_GRANTED) {
launchCamera()
} else {
ActivityCompat.requestPermissions(
activity,
arrayOf(Manifest.permission.CAMERA, Manifest.permission.WRITE_EXTERNAL_STORAGE),
WPPermissionUtils.AZTEC_EDITOR_CAMERA_PERMISSION_REQUEST_CODE
)
}
}Or investigate if
Contributor
Author
There was a problem hiding this comment. This is a false warning. |
||
| activity, | ||
| WPPermissionUtils.AZTEC_EDITOR_CAMERA_PERMISSION_REQUEST_CODE | ||
| ) | ||
| ) { | ||
| launchCamera() | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Handles the camera permission result in onRequestPermissionsResult. | ||
| * Should be called when requestCode matches AZTEC_EDITOR_CAMERA_PERMISSION_REQUEST_CODE. | ||
| * | ||
| * @param requestCode The permission request code | ||
| * @param allGranted Whether all requested permissions were granted | ||
| * @param launchCamera Callback to launch the camera when permission is granted | ||
| * @return true if this helper handled the request code, false otherwise | ||
| */ | ||
| fun handlePermissionResult( | ||
| requestCode: Int, | ||
| allGranted: Boolean, | ||
| launchCamera: () -> Unit | ||
| ): Boolean { | ||
| if (requestCode == WPPermissionUtils.AZTEC_EDITOR_CAMERA_PERMISSION_REQUEST_CODE) { | ||
| if (allGranted) { | ||
| launchCamera() | ||
| } | ||
| return true | ||
| } | ||
| return false | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,6 +39,13 @@ interface EditorPhotoPickerListener { | |
| */ | ||
| interface EditorMediaActions { | ||
| fun launchCamera() | ||
|
|
||
| /** | ||
| * Checks for camera permissions and launches the camera if granted. | ||
| * If permissions are not granted, requests them from the user. | ||
| * The activity should handle the permission result and call launchCamera() when granted. | ||
| */ | ||
| fun checkCameraPermissionAndLaunch() | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -186,11 +193,31 @@ class EditorPhotoPicker( | |
| } | ||
| } | ||
|
|
||
| override fun onMediaToolbarButtonClicked(action: MediaToolbarAction?) { | ||
|
Contributor
There was a problem hiding this comment. Good Fix: Restoring this method implementation correctly fixes the Aztec editor regression. The previous version (from PR #22427) made this a no-op with The implementation properly handles all three media toolbar actions:
Note: Ensure this is only used by Aztec. The Gutenberg editors handle media through |
||
| val siteModel = siteModelProvider() | ||
| when (action) { | ||
| MediaToolbarAction.GALLERY -> { | ||
| // Show the embedded photo picker for selecting media from device | ||
| showPhotoPicker(siteModel) | ||
| } | ||
| MediaToolbarAction.CAMERA -> { | ||
| // Launch the camera to capture a photo (after checking permissions) | ||
| if (WPMediaUtils.currentUserCanUploadMedia(siteModel)) { | ||
| editorMediaActions.checkCameraPermissionAndLaunch() | ||
|
Contributor
There was a problem hiding this comment. Code Duplication: This permission check pattern is repeated on lines 205, 255, and 262. Suggestion: Extract to a private helper method: private fun canUploadMedia(): Boolean =
WPMediaUtils.currentUserCanUploadMedia(siteModelProvider())Then use it as: if (canUploadMedia()) {
editorMediaActions.checkCameraPermissionAndLaunch()
} else {
showNoUploadPermissionSnackbar()
}This improves maintainability and reduces the chance of inconsistencies. |
||
| } else { | ||
| showNoUploadPermissionSnackbar() | ||
| } | ||
| } | ||
| MediaToolbarAction.LIBRARY -> { | ||
| // Open the WP Media Library | ||
| mediaPickerLauncher.viewWPMediaLibraryPickerForResult( | ||
| activity, | ||
| siteModel, | ||
| MediaBrowserType.EDITOR_PICKER | ||
| ) | ||
| } | ||
| null -> { /* no-op */ } | ||
| } | ||
| } | ||
|
|
||
| fun onOrientationChanged(@Orientation newOrientation: Int) { | ||
|
|
@@ -226,7 +253,7 @@ class EditorPhotoPicker( | |
| when (action) { | ||
| is MediaPickerAction.OpenCameraForPhotos -> { | ||
| if (WPMediaUtils.currentUserCanUploadMedia(siteModel)) { | ||
| editorMediaActions.checkCameraPermissionAndLaunch() | ||
| } else { | ||
| showNoUploadPermissionSnackbar() | ||
| } | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| package org.wordpress.android.ui.posts.editor | ||
|
|
||
| import org.assertj.core.api.Assertions.assertThat | ||
| import org.junit.Test | ||
| import org.junit.runner.RunWith | ||
| import org.mockito.junit.MockitoJUnitRunner | ||
| import org.wordpress.android.util.WPPermissionUtils | ||
|
|
||
| @RunWith(MockitoJUnitRunner::class) | ||
| class EditorCameraHelperTest { | ||
| private var cameraLaunched = false | ||
| private val launchCamera: () -> Unit = { cameraLaunched = true } | ||
|
|
||
| @Test | ||
| fun `handlePermissionResult launches camera when requestCode matches and permissions granted`() { | ||
| // Arrange | ||
| cameraLaunched = false | ||
| val requestCode = WPPermissionUtils.AZTEC_EDITOR_CAMERA_PERMISSION_REQUEST_CODE | ||
|
|
||
| // Act | ||
| val handled = EditorCameraHelper.handlePermissionResult( | ||
| requestCode = requestCode, | ||
| allGranted = true, | ||
| launchCamera = launchCamera | ||
| ) | ||
|
|
||
| // Assert | ||
| assertThat(handled).isTrue() | ||
| assertThat(cameraLaunched).isTrue() | ||
| } | ||
|
|
||
| @Test | ||
| fun `handlePermissionResult does not launch camera when requestCode matches but permissions denied`() { | ||
| // Arrange | ||
| cameraLaunched = false | ||
| val requestCode = WPPermissionUtils.AZTEC_EDITOR_CAMERA_PERMISSION_REQUEST_CODE | ||
|
|
||
| // Act | ||
| val handled = EditorCameraHelper.handlePermissionResult( | ||
| requestCode = requestCode, | ||
| allGranted = false, | ||
| launchCamera = launchCamera | ||
| ) | ||
|
|
||
| // Assert | ||
| assertThat(handled).isTrue() | ||
| assertThat(cameraLaunched).isFalse() | ||
| } | ||
|
|
||
| @Test | ||
| fun `handlePermissionResult returns false when requestCode does not match`() { | ||
| // Arrange | ||
| cameraLaunched = false | ||
| val differentRequestCode = 999 | ||
|
|
||
| // Act | ||
| val handled = EditorCameraHelper.handlePermissionResult( | ||
| requestCode = differentRequestCode, | ||
| allGranted = true, | ||
| launchCamera = launchCamera | ||
| ) | ||
|
|
||
| // Assert | ||
| assertThat(handled).isFalse() | ||
| assertThat(cameraLaunched).isFalse() | ||
| } | ||
|
|
||
| @Test | ||
| fun `handlePermissionResult does not launch camera when requestCode does not match even if granted`() { | ||
| // Arrange | ||
| cameraLaunched = false | ||
| val differentRequestCode = WPPermissionUtils.EDITOR_MEDIA_PERMISSION_REQUEST_CODE | ||
|
|
||
| // Act | ||
| val handled = EditorCameraHelper.handlePermissionResult( | ||
| requestCode = differentRequestCode, | ||
| allGranted = true, | ||
| launchCamera = launchCamera | ||
| ) | ||
|
|
||
| // Assert | ||
| assertThat(handled).isFalse() | ||
| assertThat(cameraLaunched).isFalse() | ||
| } | ||
|
|
||
| @Test | ||
| fun `handlePermissionResult returns true for camera request code regardless of grant status`() { | ||
| // Arrange | ||
| val requestCode = WPPermissionUtils.AZTEC_EDITOR_CAMERA_PERMISSION_REQUEST_CODE | ||
|
|
||
| // Act & Assert - granted | ||
| val handledWhenGranted = EditorCameraHelper.handlePermissionResult( | ||
| requestCode = requestCode, | ||
| allGranted = true, | ||
| launchCamera = {} | ||
| ) | ||
| assertThat(handledWhenGranted).isTrue() | ||
|
|
||
| // Act & Assert - denied | ||
| val handledWhenDenied = EditorCameraHelper.handlePermissionResult( | ||
| requestCode = requestCode, | ||
| allGranted = false, | ||
| launchCamera = {} | ||
| ) | ||
| assertThat(handledWhenDenied).isTrue() | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good Addition: This correctly handles the permission result for Aztec editor camera launches.
Minor Observation: The return value from
handlePermissionResultis not used here. This is fine since you're calling it for side effects, but consider whether other permission results in this method should also early-return if handled:This would prevent potential conflicts if multiple handlers respond to the same request code.