From f36829993135c0d6de6abbc29f55fd9057fb269f Mon Sep 17 00:00:00 2001 From: Becca Hughes Date: Fri, 9 Feb 2024 21:08:18 +0000 Subject: [PATCH] Fix bug where maximum provider limit was not being reflected The limit of 5 providers had two issues, one it did not reserve one slot for primary which meant that you could enable 6 and also even though it wouldn't save and would show an error when you hit that limit the toggle was not reset which made it look like it was enabled. Test: on device + unit Bug: 324426504 Change-Id: Ibec4efd6394835729869194181161fe8ae743e76 --- ...CredentialManagerPreferenceController.java | 37 ++++++++++++++----- ...entialManagerPreferenceControllerTest.java | 31 +++++++++++----- 2 files changed, 49 insertions(+), 19 deletions(-) diff --git a/src/com/android/settings/applications/credentials/CredentialManagerPreferenceController.java b/src/com/android/settings/applications/credentials/CredentialManagerPreferenceController.java index d98bc51c794..be0658e8c64 100644 --- a/src/com/android/settings/applications/credentials/CredentialManagerPreferenceController.java +++ b/src/com/android/settings/applications/credentials/CredentialManagerPreferenceController.java @@ -568,7 +568,7 @@ public class CredentialManagerPreferenceController extends BasePreferenceControl */ @VisibleForTesting public boolean togglePackageNameEnabled(String packageName) { - if (mEnabledPackageNames.size() >= MAX_SELECTABLE_PROVIDERS) { + if (hasProviderLimitBeenReached()) { return false; } else { mEnabledPackageNames.add(packageName); @@ -623,6 +623,19 @@ public class CredentialManagerPreferenceController extends BasePreferenceControl return mIconResizer.createIconThumbnail(providerIcon); } + private boolean hasProviderLimitBeenReached() { + return hasProviderLimitBeenReached(mEnabledPackageNames.size()); + } + + @VisibleForTesting + public static boolean hasProviderLimitBeenReached(int enabledAdditionalProviderCount) { + // If the number of package names has reached the maximum limit then + // we should stop any new packages from being added. We will also + // reserve one place for the primary provider so if the max limit is + // five providers this will be four additional plus the primary. + return (enabledAdditionalProviderCount + 1) >= MAX_SELECTABLE_PROVIDERS; + } + private CombiPreference addProviderPreference( @NonNull Context prefContext, @NonNull CharSequence title, @@ -648,19 +661,18 @@ public class CredentialManagerPreferenceController extends BasePreferenceControl pref.setPreferenceListener( new CombiPreference.OnCombiPreferenceClickListener() { @Override - public void onCheckChanged(CombiPreference p, boolean isChecked) { + public boolean onCheckChanged(CombiPreference p, boolean isChecked) { if (isChecked) { - if (mEnabledPackageNames.size() >= MAX_SELECTABLE_PROVIDERS) { + if (hasProviderLimitBeenReached()) { // Show the error if too many enabled. - pref.setChecked(false); final DialogFragment fragment = newErrorDialogFragment(); if (fragment == null || mFragmentManager == null) { - return; + return false; } fragment.show(mFragmentManager, ErrorDialogFragment.TAG); - return; + return false; } togglePackageNameEnabled(packageName); @@ -672,6 +684,8 @@ public class CredentialManagerPreferenceController extends BasePreferenceControl } else { togglePackageNameDisabled(packageName); } + + return true; } @Override @@ -989,8 +1003,13 @@ public class CredentialManagerPreferenceController extends BasePreferenceControl @Override public void onClick(View buttonView) { // Forward the event. - if (mSwitch != null) { - mOnClickListener.onCheckChanged(CombiPreference.this, mSwitch.isChecked()); + if (mSwitch != null && mOnClickListener != null) { + if (!mOnClickListener.onCheckChanged(CombiPreference.this, mSwitch.isChecked())) { + // The update was not successful since there were too + // many enabled providers to manually reset any state. + mChecked = false; + mSwitch.setChecked(false); + } } } } @@ -1004,7 +1023,7 @@ public class CredentialManagerPreferenceController extends BasePreferenceControl public interface OnCombiPreferenceClickListener { /** Called when the check is updated */ - void onCheckChanged(CombiPreference p, boolean isChecked); + boolean onCheckChanged(CombiPreference p, boolean isChecked); /** Called when the left side is clicked. */ void onLeftSideClicked(); diff --git a/tests/unit/src/com/android/settings/applications/credentials/CredentialManagerPreferenceControllerTest.java b/tests/unit/src/com/android/settings/applications/credentials/CredentialManagerPreferenceControllerTest.java index b5aeac75ff7..0a04a739b53 100644 --- a/tests/unit/src/com/android/settings/applications/credentials/CredentialManagerPreferenceControllerTest.java +++ b/tests/unit/src/com/android/settings/applications/credentials/CredentialManagerPreferenceControllerTest.java @@ -217,33 +217,33 @@ public class CredentialManagerPreferenceControllerTest { assertThat(controller.isHiddenDueToNoProviderSet()).isFalse(); assertThat(controller.getAvailabilityStatus()).isEqualTo(AVAILABLE); - // Ensure that we stay under 5 providers. + // Ensure that we stay under 5 providers (one is reserved for primary). assertThat(controller.togglePackageNameEnabled("com.android.provider1")).isTrue(); assertThat(controller.togglePackageNameEnabled("com.android.provider2")).isTrue(); assertThat(controller.togglePackageNameEnabled("com.android.provider3")).isTrue(); assertThat(controller.togglePackageNameEnabled("com.android.provider4")).isTrue(); - assertThat(controller.togglePackageNameEnabled("com.android.provider5")).isTrue(); + assertThat(controller.togglePackageNameEnabled("com.android.provider5")).isFalse(); assertThat(controller.togglePackageNameEnabled("com.android.provider6")).isFalse(); // Check that they are all actually registered. Set enabledProviders = controller.getEnabledProviders(); - assertThat(enabledProviders.size()).isEqualTo(5); + assertThat(enabledProviders.size()).isEqualTo(4); assertThat(enabledProviders.contains("com.android.provider1")).isTrue(); assertThat(enabledProviders.contains("com.android.provider2")).isTrue(); assertThat(enabledProviders.contains("com.android.provider3")).isTrue(); assertThat(enabledProviders.contains("com.android.provider4")).isTrue(); - assertThat(enabledProviders.contains("com.android.provider5")).isTrue(); + assertThat(enabledProviders.contains("com.android.provider5")).isFalse(); assertThat(enabledProviders.contains("com.android.provider6")).isFalse(); // Check that the settings string has the right component names. List enabledServices = controller.getEnabledSettings(); - assertThat(enabledServices.size()).isEqualTo(6); + assertThat(enabledServices.size()).isEqualTo(5); assertThat(enabledServices.contains("com.android.provider1/ClassA")).isTrue(); assertThat(enabledServices.contains("com.android.provider1/ClassB")).isTrue(); assertThat(enabledServices.contains("com.android.provider2/ClassA")).isTrue(); assertThat(enabledServices.contains("com.android.provider3/ClassA")).isTrue(); assertThat(enabledServices.contains("com.android.provider4/ClassA")).isTrue(); - assertThat(enabledServices.contains("com.android.provider5/ClassA")).isTrue(); + assertThat(enabledServices.contains("com.android.provider5/ClassA")).isFalse(); assertThat(enabledServices.contains("com.android.provider6/ClassA")).isFalse(); // Toggle the provider disabled. @@ -251,22 +251,22 @@ public class CredentialManagerPreferenceControllerTest { // Check that the provider was removed from the list of providers. Set currentlyEnabledProviders = controller.getEnabledProviders(); - assertThat(currentlyEnabledProviders.size()).isEqualTo(4); + assertThat(currentlyEnabledProviders.size()).isEqualTo(3); assertThat(currentlyEnabledProviders.contains("com.android.provider1")).isTrue(); assertThat(currentlyEnabledProviders.contains("com.android.provider2")).isFalse(); assertThat(currentlyEnabledProviders.contains("com.android.provider3")).isTrue(); assertThat(currentlyEnabledProviders.contains("com.android.provider4")).isTrue(); - assertThat(currentlyEnabledProviders.contains("com.android.provider5")).isTrue(); + assertThat(currentlyEnabledProviders.contains("com.android.provider5")).isFalse(); assertThat(currentlyEnabledProviders.contains("com.android.provider6")).isFalse(); // Check that the provider was removed from the list of services stored in the setting. List currentlyEnabledServices = controller.getEnabledSettings(); - assertThat(currentlyEnabledServices.size()).isEqualTo(5); + assertThat(currentlyEnabledServices.size()).isEqualTo(4); assertThat(currentlyEnabledServices.contains("com.android.provider1/ClassA")).isTrue(); assertThat(currentlyEnabledServices.contains("com.android.provider1/ClassB")).isTrue(); assertThat(currentlyEnabledServices.contains("com.android.provider3/ClassA")).isTrue(); assertThat(currentlyEnabledServices.contains("com.android.provider4/ClassA")).isTrue(); - assertThat(currentlyEnabledServices.contains("com.android.provider5/ClassA")).isTrue(); + assertThat(currentlyEnabledServices.contains("com.android.provider5/ClassA")).isFalse(); assertThat(currentlyEnabledServices.contains("com.android.provider6/ClassA")).isFalse(); } @@ -528,6 +528,17 @@ public class CredentialManagerPreferenceControllerTest { assertThat(thumbnail.getIntrinsicWidth()).isEqualTo(getIconSize()); } + @Test + public void testProviderLimitReached() { + // The limit is 5 with one slot reserved for primary. + assertThat(CredentialManagerPreferenceController.hasProviderLimitBeenReached(0)).isFalse(); + assertThat(CredentialManagerPreferenceController.hasProviderLimitBeenReached(1)).isFalse(); + assertThat(CredentialManagerPreferenceController.hasProviderLimitBeenReached(2)).isFalse(); + assertThat(CredentialManagerPreferenceController.hasProviderLimitBeenReached(3)).isFalse(); + assertThat(CredentialManagerPreferenceController.hasProviderLimitBeenReached(4)).isTrue(); + assertThat(CredentialManagerPreferenceController.hasProviderLimitBeenReached(5)).isTrue(); + } + private int getIconSize() { final Resources resources = mContext.getResources(); return (int) resources.getDimension(android.R.dimen.app_icon_size);