From 48935b3e01ff3d095cd9303d6038f5d7d841682f Mon Sep 17 00:00:00 2001 From: josephpv Date: Mon, 17 Mar 2025 17:17:14 +0000 Subject: Add search bar trailing icon to clear search text Recording: b/402529602#comment3 Bug: 402529602 Test: Manual Flag: EXEMPT Bug fix Change-Id: Ie90d8a80a5735ec61cb852108aa5cc1ece9b7325 --- photopicker/res/values/feature_search_strings.xml | 3 ++ .../android/photopicker/features/search/Search.kt | 44 ++++++++++++++++++---- 2 files changed, 40 insertions(+), 7 deletions(-) diff --git a/photopicker/res/values/feature_search_strings.xml b/photopicker/res/values/feature_search_strings.xml index 78af2a470..dacf372f7 100644 --- a/photopicker/res/values/feature_search_strings.xml +++ b/photopicker/res/values/feature_search_strings.xml @@ -24,6 +24,9 @@ Search your videos + + Clear search text + No results found diff --git a/photopicker/src/com/android/photopicker/features/search/Search.kt b/photopicker/src/com/android/photopicker/features/search/Search.kt index 0f2c619be..9cd927e09 100644 --- a/photopicker/src/com/android/photopicker/features/search/Search.kt +++ b/photopicker/src/com/android/photopicker/features/search/Search.kt @@ -39,6 +39,7 @@ import androidx.compose.foundation.shape.CircleShape import androidx.compose.foundation.shape.RoundedCornerShape import androidx.compose.material.icons.Icons import androidx.compose.material.icons.automirrored.filled.ArrowBack +import androidx.compose.material.icons.filled.Close import androidx.compose.material.icons.filled.Today import androidx.compose.material.icons.outlined.HideImage import androidx.compose.material.icons.outlined.History @@ -118,6 +119,7 @@ import com.android.photopicker.core.theme.LocalWindowSizeClass import com.android.photopicker.extensions.navigateToPreviewMedia import com.android.photopicker.extensions.transferScrollableTouchesToHostInEmbedded import com.android.photopicker.features.preview.PreviewFeature +import com.android.photopicker.features.search.SearchViewModel.Companion.ZERO_STATE_SEARCH_QUERY import com.android.photopicker.features.search.model.SearchSuggestion import com.android.photopicker.features.search.model.SearchSuggestionType import com.android.photopicker.features.search.model.UserSearchState @@ -428,16 +430,12 @@ fun SearchInputContent( * @param searchQuery The current text entered in search bar input field. * @param focused A boolean value indicating whether the search input field is currently focused. * @param onSearchQueryChanged A callback function that is invoked when the search query text - * changes. - * * This function receives the updated search query as a parameter. - * + * changes. This function receives the updated search query as a parameter. * @param onFocused A callback function that is invoked when the focus state of the search field - * changes. - * * This function receives a boolean value indicating the new focus state. - * + * changes. This function receives a boolean value indicating the new focus state. * @param onSearch A callback function to be invoked when a text is searched. * @param modifier A Modifier that can be applied to the SearchInput composable to customize its - * * appearance and behavior. + * appearance and behavior. */ @Composable @OptIn(ExperimentalMaterial3Api::class) @@ -473,11 +471,43 @@ private fun SearchInput( expanded = focused, onExpandedChange = onFocused, leadingIcon = { SearchBarIcon(focused, onFocused, onSearchQueryChanged) }, + trailingIcon = { + SearchBarTrailingIcon( + focused && !searchQuery.equals(ZERO_STATE_SEARCH_QUERY), + onSearchQueryChanged, + ) + }, modifier = modifier.focusRequester(focusRequester), ) RequestFocusOnResume(focusRequester = focusRequester, focused) } +/** + * A composable function that displays the trailing icon in a SearchBar. The icon is shown when + * query is typed clicking on which clears the typed text. + * + * @param showClearIcon A boolean value indicating whether clear icon is to be shown + * @param onSearchQueryChanged A callback function that is invoked when the search query text + * changes. This function receives the updated search query as a parameter. + * @param viewModel The `SearchViewModel` providing the search logic and state. + */ +@Composable +private fun SearchBarTrailingIcon( + showClearIcon: Boolean, + onSearchQueryChanged: (String) -> Unit, + viewModel: SearchViewModel = obtainViewModel(), +) { + val searchState by viewModel.searchState.collectAsStateWithLifecycle() + if (showClearIcon && searchState is SearchState.Inactive) { + IconButton(onClick = { onSearchQueryChanged("") }) { + Icon( + Icons.Filled.Close, + contentDescription = stringResource(R.string.photopicker_search_clear_text), + ) + } + } +} + @Composable private fun RequestFocusOnResume( focusRequester: FocusRequester, -- cgit v1.2.3-59-g8ed1b From 1c0cf347f19ac48ff28c19ce9f9821ef9a3efd93 Mon Sep 17 00:00:00 2001 From: Dipankar Bhardwaj Date: Fri, 7 Feb 2025 16:58:52 +0000 Subject: Fix for legacy storage app op There is a bug in AppOpsManager that keeps legacy storage granted even when an app updates its targetSdkVersion from value <30 to >=30. If an app upgrades from targetSdk 29 to targetSdk 33, legacy storage remains granted and in targetSdk 33, app are required to replace R_E_S with R_M_*. If an app updates its manifest with R_M_*, permission check in MediaProvider will look for R_E_S and will not grant read access as the app would be still treated as legacy. Test: atest PermissionUtilsTest Bug: 315914683 Flag: EXEMPT bug fix Change-Id: I22d0a329fbea5afe9dea2bc5179596db7a2ef402 --- .../providers/media/LocalCallingIdentity.java | 12 ++++-- .../providers/media/util/PermissionUtils.java | 35 ++++++++++++++++ tests/Android.bp | 20 +++++++++ tests/AndroidManifest.xml | 1 + tests/AndroidTest.xml | 1 + .../providers/media/util/PermissionUtilsTest.java | 49 ++++++++++++++++++++++ tests/test_app/LegacyTestAppWithTargetSdk33.xml | 40 ++++++++++++++++++ 7 files changed, 155 insertions(+), 3 deletions(-) create mode 100644 tests/test_app/LegacyTestAppWithTargetSdk33.xml diff --git a/src/com/android/providers/media/LocalCallingIdentity.java b/src/com/android/providers/media/LocalCallingIdentity.java index 8074656bd..2a5e452ce 100644 --- a/src/com/android/providers/media/LocalCallingIdentity.java +++ b/src/com/android/providers/media/LocalCallingIdentity.java @@ -29,8 +29,8 @@ import static com.android.providers.media.util.PermissionUtils.checkPermissionIn import static com.android.providers.media.util.PermissionUtils.checkPermissionManager; import static com.android.providers.media.util.PermissionUtils.checkPermissionQueryAllPackages; import static com.android.providers.media.util.PermissionUtils.checkPermissionReadAudio; +import static com.android.providers.media.util.PermissionUtils.checkPermissionReadForLegacyStorage; import static com.android.providers.media.util.PermissionUtils.checkPermissionReadImages; -import static com.android.providers.media.util.PermissionUtils.checkPermissionReadStorage; import static com.android.providers.media.util.PermissionUtils.checkPermissionReadVideo; import static com.android.providers.media.util.PermissionUtils.checkPermissionReadVisualUserSelected; import static com.android.providers.media.util.PermissionUtils.checkPermissionSelf; @@ -576,8 +576,14 @@ public class LocalCallingIdentity { } private boolean isLegacyReadInternal() { - return hasPermission(PERMISSION_IS_LEGACY_GRANTED) - && checkPermissionReadStorage(context, pid, uid, getPackageName(), attributionTag); + boolean isLegacyStorageGranted = hasPermission(PERMISSION_IS_LEGACY_GRANTED); + if (!isLegacyStorageGranted) { + return false; + } + + boolean isTargetSdkAtleastT = getTargetSdkVersion() >= Build.VERSION_CODES.TIRAMISU; + return checkPermissionReadForLegacyStorage(context, pid, uid, getPackageName(), + attributionTag, isTargetSdkAtleastT); } /** System internals or callers holding permission have no redaction */ diff --git a/src/com/android/providers/media/util/PermissionUtils.java b/src/com/android/providers/media/util/PermissionUtils.java index 2458785e2..7dbfd23da 100644 --- a/src/com/android/providers/media/util/PermissionUtils.java +++ b/src/com/android/providers/media/util/PermissionUtils.java @@ -148,6 +148,41 @@ public class PermissionUtils { generateAppOpMessage(packageName,sOpDescription.get())); } + /** + * Check for read permission when legacy storage is granted. + * There is a bug in AppOpsManager that keeps legacy storage granted even + * when an app updates its targetSdkVersion from value <30 to >=30. + * If an app upgrades from targetSdk 29 to targetSdk 33, legacy storage + * remains granted and in targetSdk 33, app are required to replace R_E_S + * with R_M_*. If an app updates its manifest with R_M_*, permission check + * in MediaProvider will look for R_E_S and will not grant read access as + * the app would be still treated as legacy. Ensure that legacy app either has + * R_E_S or all of R_M_* to get read permission. Since this is a fix for legacy + * app op bug, we are avoiding granular permission checks based on media type. + */ + public static boolean checkPermissionReadForLegacyStorage(@NonNull Context context, + int pid, int uid, @NonNull String packageName, @Nullable String attributionTag, + boolean isTargetSdkAtleastT) { + if (isTargetSdkAtleastT) { + return checkPermissionForDataDelivery(context, READ_EXTERNAL_STORAGE, pid, uid, + packageName, attributionTag, + generateAppOpMessage(packageName, sOpDescription.get())) || ( + checkPermissionForDataDelivery(context, READ_MEDIA_IMAGES, pid, uid, + packageName, attributionTag, + generateAppOpMessage(packageName, sOpDescription.get())) + && checkPermissionForDataDelivery(context, READ_MEDIA_VIDEO, pid, uid, + packageName, attributionTag, + generateAppOpMessage(packageName, sOpDescription.get())) + && checkPermissionForDataDelivery(context, READ_MEDIA_AUDIO, pid, uid, + packageName, attributionTag, + generateAppOpMessage(packageName, sOpDescription.get()))); + } else { + return checkPermissionForDataDelivery(context, READ_EXTERNAL_STORAGE, pid, uid, + packageName, attributionTag, + generateAppOpMessage(packageName, sOpDescription.get())); + } + } + /** * Check if the given package has been granted the * android.Manifest.permission#ACCESS_MEDIA_LOCATION permission. diff --git a/tests/Android.bp b/tests/Android.bp index cdb44b814..3de35a077 100644 --- a/tests/Android.bp +++ b/tests/Android.bp @@ -155,6 +155,25 @@ android_test_helper_app { ], } +android_test_helper_app { + name: "LegacyMediaProviderTestAppFor33", + manifest: "test_app/LegacyTestAppWithTargetSdk33.xml", + srcs: [ + "test_app/src/**/*.java", + "src/com/android/providers/media/util/TestUtils.java", + ], + static_libs: [ + "cts-install-lib", + ], + sdk_version: "test_current", + target_sdk_version: "33", + min_sdk_version: "30", + test_suites: [ + "general-tests", + "mts-mediaprovider", + ], +} + // This looks a bit awkward, but we need our tests to run against either // MediaProvider or MediaProviderGoogle, and we don't know which one is // on the device being tested, so we can't sign our tests with a key that @@ -250,6 +269,7 @@ android_test { data: [ ":LegacyMediaProviderTestApp", + ":LegacyMediaProviderTestAppFor33", ":LegacyMediaProviderTestAppFor35", ":MediaProviderTestAppForPermissionActivity", ":MediaProviderTestAppForPermissionActivity33", diff --git a/tests/AndroidManifest.xml b/tests/AndroidManifest.xml index acd1a21af..e3902d6eb 100644 --- a/tests/AndroidManifest.xml +++ b/tests/AndroidManifest.xml @@ -13,6 +13,7 @@ + diff --git a/tests/AndroidTest.xml b/tests/AndroidTest.xml index 31d9e1535..44fb27930 100644 --- a/tests/AndroidTest.xml +++ b/tests/AndroidTest.xml @@ -30,6 +30,7 @@