Skip to content
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.

Sign up for GitHub

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

Improved external storage permission handling #157 #163

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Improved external storage permission handling #157 #163

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
48e112c
Improved storage permission handling
Anshu-Bijarnia Mar 29, 2024
82abade
Updated permissions
Anshu-Bijarnia Apr 19, 2024
548fdde
Updated AppTestUtil
Anshu-Bijarnia Apr 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions app/src/main/AndroidManifest.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
<?xml version="1.0" encoding="utf-8"?>
<!--
<?xml version="1.0" encoding="utf-8"?><!--
~ Copyright (C) 2023 The Android Open Source Project
~
~ Licensed under the Apache License, Version 2.0 (the "License");
Expand All @@ -25,10 +24,17 @@
android:required="false" />

<uses-permission android:name="android.permission.CAMERA" />
<uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE"
android:maxSdkVersion="28"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Anshu, you removed writing permission and replaced it with reading permissions. Won't this cause a problem with image capturing and recording where we save an image or video file onto disk?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More on this: Image capture is tested in ImageCaptureDeviceTest, where the WRITE_EXTERNAL_STORAGE permission is specified again in the test. I believe if you change AppTestUtil.APP_REQUIRED_PERMISSIONS to reflect your changes here, the test will fail, as the app wouldn't be able to save onto storage without WRITE permission

tools:ignore="ScopedStorage" />

<!-- Devices running Android 12L (API level 32) or lower -->
<uses-permission
android:name="android.permission.READ_EXTERNAL_STORAGE"
android:maxSdkVersion="32" />
<!-- Devices running Android 13 (API level 33) or higher -->
<uses-permission android:name="android.permission.READ_MEDIA_IMAGES" />
<uses-permission android:name="android.permission.READ_MEDIA_VIDEO" />
<!-- To handle the reselection within the app on devices running Android 14
or higher if your app targets Android 14 (API level 34) or higher. -->
<uses-permission android:name="android.permission.READ_MEDIA_VISUAL_USER_SELECTED" />

Comment on lines +35 to +39
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these 3 permissions should be added in AppTestUtil with the same version-checking logics they as in JcaApp

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will do that and make a PR

<application
android:name=".JetpackCameraApplication"
Expand All @@ -41,7 +47,7 @@
android:supportsRtl="true"
android:theme="@style/Theme.JetpackCamera"
tools:targetApi="33">
<profileable android:shell="true"/>
<profileable android:shell="true" />
<activity
android:name=".MainActivity"
android:exported="true"
Expand Down
54 changes: 45 additions & 9 deletions app/src/main/java/com/google/jetpackcamera/ui/JcaApp.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@
package com.google.jetpackcamera.ui

import android.Manifest
import android.Manifest.permission.READ_EXTERNAL_STORAGE
import android.Manifest.permission.READ_MEDIA_IMAGES
import android.Manifest.permission.READ_MEDIA_VIDEO
import android.Manifest.permission.READ_MEDIA_VISUAL_USER_SELECTED
import android.os.Build
import androidx.compose.foundation.layout.fillMaxSize
import androidx.compose.runtime.Composable
import androidx.compose.ui.Modifier
Expand All @@ -24,8 +29,9 @@ import androidx.navigation.compose.NavHost
import androidx.navigation.compose.composable
import androidx.navigation.compose.rememberNavController
import com.google.accompanist.permissions.ExperimentalPermissionsApi
import com.google.accompanist.permissions.MultiplePermissionsState
import com.google.accompanist.permissions.isGranted
import com.google.accompanist.permissions.rememberPermissionState
import com.google.accompanist.permissions.rememberMultiplePermissionsState
import com.google.jetpackcamera.BuildConfig
import com.google.jetpackcamera.feature.preview.PreviewMode
import com.google.jetpackcamera.feature.preview.PreviewScreen
Expand All @@ -42,22 +48,52 @@ fun JcaApp(
/*TODO(b/306236646): remove after still capture*/
previewMode: PreviewMode
) {
val permissionState =
rememberPermissionState(permission = Manifest.permission.CAMERA)
val cameraPermissionState =
rememberMultiplePermissionsState(permissions = listOf(Manifest.permission.CAMERA))
val storagePermissionState: MultiplePermissionsState

if (permissionState.status.isGranted) {
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.UPSIDE_DOWN_CAKE) {
storagePermissionState = rememberMultiplePermissionsState(
permissions = listOf(
READ_MEDIA_VISUAL_USER_SELECTED,
READ_MEDIA_IMAGES,
READ_MEDIA_VIDEO
)
)
} else if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) {
storagePermissionState =
rememberMultiplePermissionsState(
permissions = listOf(
READ_MEDIA_IMAGES,
READ_MEDIA_VIDEO
)
)
} else {
storagePermissionState =
rememberMultiplePermissionsState(permissions = listOf(READ_EXTERNAL_STORAGE))
}
if (cameraPermissionState.allPermissionsGranted && (storagePermissionState.allPermissionsGranted || storagePermissionState.permissions[0].status.isGranted)) {
JetpackCameraNavHost(
onPreviewViewModel = onPreviewViewModel,
previewMode = previewMode
)
} else {
CameraPermission(
modifier = Modifier.fillMaxSize(),
cameraPermissionState = permissionState
)
if (!cameraPermissionState.allPermissionsGranted) {
CameraPermission(
modifier = Modifier.fillMaxSize(),
cameraPermissionState = cameraPermissionState
)
}
if (!storagePermissionState.allPermissionsGranted && !storagePermissionState.permissions[0].status.isGranted) {
StoragePermission(
modifier = Modifier.fillMaxSize(),
storagePermissionState = storagePermissionState
)
}
}
}


@Composable
private fun JetpackCameraNavHost(
onPreviewViewModel: (PreviewViewModel) -> Unit,
Expand All @@ -82,4 +118,4 @@ private fun JetpackCameraNavHost(
)
}
}
}
}
34 changes: 27 additions & 7 deletions app/src/main/java/com/google/jetpackcamera/ui/PermissionsUi.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,15 @@ import androidx.compose.ui.text.font.FontWeight
import androidx.compose.ui.text.style.TextAlign
import androidx.compose.ui.unit.dp
import com.google.accompanist.permissions.ExperimentalPermissionsApi
import com.google.accompanist.permissions.PermissionState
import com.google.accompanist.permissions.MultiplePermissionsState
import com.google.jetpackcamera.R

@OptIn(ExperimentalPermissionsApi::class)
@Composable
fun CameraPermission(modifier: Modifier = Modifier, cameraPermissionState: PermissionState) {
fun CameraPermission(
modifier: Modifier = Modifier,
cameraPermissionState: MultiplePermissionsState
) {
PermissionTemplate(
modifier = modifier,
permissionState = cameraPermissionState,
Expand All @@ -61,11 +64,28 @@ fun CameraPermission(modifier: Modifier = Modifier, cameraPermissionState: Permi
)
}

@OptIn(ExperimentalPermissionsApi::class)
@Composable
fun StoragePermission(
modifier: Modifier = Modifier,
storagePermissionState: MultiplePermissionsState
) {
PermissionTemplate(
modifier = modifier,
permissionState = storagePermissionState,
painter = painterResource(id = R.drawable.photo_storage),
iconAccessibilityText = stringResource(id = R.string.storage_permission_accessibility_text),
title = stringResource(id = R.string.storage_permission_screen_title),
bodyText = stringResource(id = R.string.storage_permission_required_rationale),
requestButtonText = stringResource(id = R.string.request_permission)
)
}

@OptIn(ExperimentalPermissionsApi::class)
@Composable
fun PermissionTemplate(
modifier: Modifier = Modifier,
permissionState: PermissionState,
permissionState: MultiplePermissionsState,
onSkipPermission: (() -> Unit)? = null,
painter: Painter,
iconAccessibilityText: String,
Expand Down Expand Up @@ -127,7 +147,7 @@ fun PermissionImage(modifier: Modifier = Modifier, painter: Painter, accessibili
@Composable
fun PermissionButtonSection(
modifier: Modifier = Modifier,
permissionState: PermissionState,
permissionState: MultiplePermissionsState,
requestButtonText: String,
onSkipPermission: (() -> Unit)?
) {
Expand Down Expand Up @@ -160,7 +180,7 @@ fun PermissionButtonSection(
@Composable
fun PermissionButton(
modifier: Modifier = Modifier,
permissionState: PermissionState,
permissionState: MultiplePermissionsState,
requestButtonText: String
) {
Button(
Expand All @@ -169,7 +189,7 @@ fun PermissionButton(
containerColor = MaterialTheme.colorScheme.primaryContainer,
contentColor = MaterialTheme.colorScheme.onPrimaryContainer
),
onClick = { permissionState.launchPermissionRequest() }
onClick = { permissionState.launchMultiplePermissionRequest() }
) {
Text(
modifier = Modifier.padding(10.dp),
Expand Down Expand Up @@ -232,4 +252,4 @@ fun PermissionBodyText(modifier: Modifier = Modifier, text: String, color: Color
style = MaterialTheme.typography.bodyLarge,
textAlign = TextAlign.Center
)
}
}
5 changes: 5 additions & 0 deletions app/src/main/res/drawable/photo_storage.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<vector xmlns:android="http://schemas.android.com/apk/res/android" android:height="300dp" android:tint="#000000" android:viewportHeight="24" android:viewportWidth="24" android:width="300dp">

<path android:fillColor="@android:color/white" android:pathData="M2,20h20v-4L2,16v4zM4,17h2v2L4,19v-2zM2,4v4h20L22,4L2,4zM6,7L4,7L4,5h2v2zM2,14h20v-4L2,10v4zM4,11h2v2L4,13v-2z"/>

</vector>
3 changes: 3 additions & 0 deletions app/src/main/res/values/strings.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,11 @@
<resources>
<string name="app_name">Jetpack Camera</string>
<string name="camera_permission_screen_title">Enable Camera</string>
<string name="storage_permission_screen_title">Provide Storage Access</string>
<string name="camera_permission_required_rationale">Please provide permission to access to the camera. It is necessary for this app to function.</string>
<string name="storage_permission_required_rationale">Please provide permission to access to the storage. It is necessary for this app to function.</string>
<string name="camera_permission_accessibility_text">A symbol representing a camera</string>
<string name="storage_permission_accessibility_text">A symbol representing storage</string>
<string name="request_permission">Allow Access</string>
<string name="jca_loading">Loading App…</string>
<string name="camera_not_available">Camera not available</string>
Expand Down
Loading