From 1c3fb499133a6a12e59c17379b4e97913d4c7b09 Mon Sep 17 00:00:00 2001 From: Becca Hughes Date: Wed, 17 Apr 2024 12:49:07 -0700 Subject: [PATCH] Fix bug where empty section is sometimes shown There is a bug where the empty section is sometimes shown for additional providers. This is because the logic for calculating the visibility gets the providers and then the logic for displaying the list gets the providers and can result in getting out of sync. This aligns both sets of logic to use the same data. Test: ondevice & unit Bug: 330163369 Change-Id: Icf71c78d67d55c929476f8e699d5893e60a49776 --- ...CredentialManagerPreferenceController.java | 120 +++++++++++------- ...entialManagerPreferenceControllerTest.java | 85 +++++++++---- 2 files changed, 138 insertions(+), 67 deletions(-) diff --git a/src/com/android/settings/applications/credentials/CredentialManagerPreferenceController.java b/src/com/android/settings/applications/credentials/CredentialManagerPreferenceController.java index a92b755331c..ca3786a443b 100644 --- a/src/com/android/settings/applications/credentials/CredentialManagerPreferenceController.java +++ b/src/com/android/settings/applications/credentials/CredentialManagerPreferenceController.java @@ -115,6 +115,7 @@ public class CredentialManagerPreferenceController extends BasePreferenceControl private @Nullable Delegate mDelegate = null; private @Nullable String mFlagOverrideForTest = null; private @Nullable PreferenceScreen mPreferenceScreen = null; + private @Nullable PreferenceGroup mPreferenceGroup = null; private Optional mSimulateHiddenForTests = Optional.empty(); private boolean mIsWorkProfile = false; @@ -161,12 +162,6 @@ public class CredentialManagerPreferenceController extends BasePreferenceControl return UNSUPPORTED_ON_DEVICE; } - // If there is no top provider or any providers in the list then - // we should hide this pref. - if (isHiddenDueToNoProviderSet()) { - return CONDITIONALLY_UNAVAILABLE; - } - if (!hasNonPrimaryServices()) { return CONDITIONALLY_UNAVAILABLE; } @@ -355,24 +350,11 @@ public class CredentialManagerPreferenceController extends BasePreferenceControl } // Get the list of new providers and components. - List newProviders = + setAvailableServices( mCredentialManager.getCredentialProviderServices( getUser(), - CredentialManager.PROVIDER_FILTER_USER_PROVIDERS_INCLUDING_HIDDEN); - Set newComponents = buildComponentNameSet(newProviders, false); - Set newPrimaryComponents = buildComponentNameSet(newProviders, true); - - // Get the list of old components - Set oldComponents = buildComponentNameSet(mServices, false); - Set oldPrimaryComponents = buildComponentNameSet(mServices, true); - - // If the sets are equal then don't update the UI. - if (oldComponents.equals(newComponents) - && oldPrimaryComponents.equals(newPrimaryComponents)) { - return; - } - - setAvailableServices(newProviders, null); + CredentialManager.PROVIDER_FILTER_USER_PROVIDERS_INCLUDING_HIDDEN), + null); if (mPreferenceScreen != null) { displayPreference(mPreferenceScreen); @@ -396,11 +378,7 @@ public class CredentialManagerPreferenceController extends BasePreferenceControl } @VisibleForTesting - public boolean isHiddenDueToNoProviderSet() { - return isHiddenDueToNoProviderSet(getProviders()); - } - - private boolean isHiddenDueToNoProviderSet( + public boolean isHiddenDueToNoProviderSet( Pair, CombinedProviderInfo> providerPair) { if (mSimulateHiddenForTests.isPresent()) { return mSimulateHiddenForTests.get(); @@ -444,17 +422,67 @@ public class CredentialManagerPreferenceController extends BasePreferenceControl @Override public void displayPreference(PreferenceScreen screen) { - super.displayPreference(screen); + final String prefKey = getPreferenceKey(); + if (TextUtils.isEmpty(prefKey)) { + Log.w(TAG, "Skipping displayPreference because key is empty"); + return; + } - // Since the UI is being cleared, clear any refs. + // Store this reference for later. + if (mPreferenceScreen == null) { + mPreferenceScreen = screen; + mPreferenceGroup = screen.findPreference(prefKey); + } + + final Pair, CombinedProviderInfo> providerPair = getProviders(); + + maybeUpdateListOfPrefs(providerPair); + maybeUpdatePreferenceVisibility(providerPair); + } + + private void maybeUpdateListOfPrefs( + Pair, CombinedProviderInfo> providerPair) { + if (mPreferenceScreen == null || mPreferenceGroup == null) { + return; + } + + // Build the new list of prefs. + Map newPrefs = + buildPreferenceList(mPreferenceScreen.getContext(), providerPair); + + // Determine if we need to update the prefs. + Set existingPrefPackageNames = mPrefs.keySet(); + if (existingPrefPackageNames.equals(newPrefs.keySet())) { + return; + } + + // Since the UI is being cleared, clear any refs and prefs. mPrefs.clear(); + mPreferenceGroup.removeAll(); - mPreferenceScreen = screen; - PreferenceGroup group = screen.findPreference(getPreferenceKey()); - group.removeAll(); + // Populate the preference list with new data. + mPrefs.putAll(newPrefs); + for (CombiPreference pref : newPrefs.values()) { + mPreferenceGroup.addPreference(pref); + } + } - Context context = screen.getContext(); - mPrefs.putAll(buildPreferenceList(context, group)); + private void maybeUpdatePreferenceVisibility( + Pair, CombinedProviderInfo> providerPair) { + if (mPreferenceScreen == null || mPreferenceGroup == null) { + return; + } + + final boolean isAvailable = + (getAvailabilityStatus() == AVAILABLE) && !isHiddenDueToNoProviderSet(providerPair); + + if (isAvailable) { + mPreferenceScreen.addPreference(mPreferenceGroup); + mPreferenceGroup.setVisible(true); + } else { + mPreferenceScreen.removePreference(mPreferenceGroup); + mPreferenceGroup.setVisible(false); + } } /** @@ -511,9 +539,9 @@ public class CredentialManagerPreferenceController extends BasePreferenceControl /** Aggregates the list of services and builds a list of UI prefs to show. */ @VisibleForTesting public @NonNull Map buildPreferenceList( - @NonNull Context context, @NonNull PreferenceGroup group) { - // Get the providers and extract the values. - Pair, CombinedProviderInfo> providerPair = getProviders(); + @NonNull Context context, + @NonNull Pair, CombinedProviderInfo> providerPair) { + // Extract the values. CombinedProviderInfo topProvider = providerPair.second; List providers = providerPair.first; @@ -554,7 +582,6 @@ public class CredentialManagerPreferenceController extends BasePreferenceControl combinedInfo.getSettingsActivity(), combinedInfo.getDeviceAdminRestrictions(context, getUser())); output.put(packageName, pref); - group.addPreference(pref); } // Set the visibility if we have services. @@ -1023,11 +1050,12 @@ public class CredentialManagerPreferenceController extends BasePreferenceControl public void onClick(View buttonView) { // Forward the event. 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); + 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); } } } @@ -1083,8 +1111,10 @@ public class CredentialManagerPreferenceController extends BasePreferenceControl if (mSwitch != null && !TextUtils.isEmpty(appName)) { mSwitch.setContentDescription( - getContext().getString( - R.string.credman_on_off_switch_content_description, appName)); + getContext() + .getString( + R.string.credman_on_off_switch_content_description, + appName)); } } 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 c0023ee8afd..3cd15330a2f 100644 --- a/tests/unit/src/com/android/settings/applications/credentials/CredentialManagerPreferenceControllerTest.java +++ b/tests/unit/src/com/android/settings/applications/credentials/CredentialManagerPreferenceControllerTest.java @@ -17,7 +17,6 @@ package com.android.settings.applications.credentials; import static com.android.settings.core.BasePreferenceController.AVAILABLE; -import static com.android.settings.core.BasePreferenceController.CONDITIONALLY_UNAVAILABLE; import static com.android.settings.core.BasePreferenceController.UNSUPPORTED_ON_DEVICE; import static com.google.common.truth.Truth.assertThat; @@ -36,7 +35,9 @@ import android.graphics.drawable.Drawable; import android.net.Uri; import android.os.Looper; import android.provider.Settings; +import android.util.Pair; +import androidx.annotation.Nullable; import androidx.preference.Preference; import androidx.preference.PreferenceCategory; import androidx.preference.PreferenceManager; @@ -124,19 +125,37 @@ public class CredentialManagerPreferenceControllerTest { controller.setSimulateConnectedForTests(true); assertThat(controller.isConnected()).isTrue(); controller.setSimulateHiddenForTests(Optional.of(false)); - assertThat(controller.isHiddenDueToNoProviderSet()).isFalse(); + assertThat(controller.isHiddenDueToNoProviderSet(createPair())).isFalse(); assertThat(controller.getAvailabilityStatus()).isEqualTo(AVAILABLE); } @Test - public void getAvailabilityStatus_isHidden_returnsConditionallyUnavailable() { + public void isHiddenDueToNoProviderSet_hiddenDueToEmptyPair() { CredentialManagerPreferenceController controller = createControllerWithServices(Lists.newArrayList(createCredentialProviderInfo())); - controller.setSimulateConnectedForTests(true); - assertThat(controller.isConnected()).isTrue(); - controller.setSimulateHiddenForTests(Optional.of(true)); - assertThat(controller.isHiddenDueToNoProviderSet()).isTrue(); - assertThat(controller.getAvailabilityStatus()).isEqualTo(CONDITIONALLY_UNAVAILABLE); + assertThat(controller.isHiddenDueToNoProviderSet(createPair())).isTrue(); + } + + @Test + public void isHiddenDueToNoProviderSet_hiddenDueToNoPrimaryProvider() { + CredentialManagerPreferenceController controller = + createControllerWithServices(Lists.newArrayList(createCredentialProviderInfo())); + + Pair, CombinedProviderInfo> testPair = + new Pair<>(Lists.newArrayList(createCombinedProviderInfo()), null); + assertThat(controller.isHiddenDueToNoProviderSet(testPair)).isTrue(); + } + + @Test + public void isHiddenDueToNoProviderSet_validDataSoNotHidden() { + CredentialManagerPreferenceController controller = + createControllerWithServices(Lists.newArrayList(createCredentialProviderInfo())); + + Pair, CombinedProviderInfo> testPair = + new Pair<>( + Lists.newArrayList(createCombinedProviderInfo()), + createCombinedProviderInfo()); + assertThat(controller.isHiddenDueToNoProviderSet(testPair)).isFalse(); } @Test @@ -146,7 +165,7 @@ public class CredentialManagerPreferenceControllerTest { controller.setSimulateConnectedForTests(true); controller.setSimulateHiddenForTests(Optional.of(false)); - assertThat(controller.isHiddenDueToNoProviderSet()).isFalse(); + assertThat(controller.isHiddenDueToNoProviderSet(createPair())).isFalse(); assertThat(controller.isConnected()).isTrue(); assertThat(controller.getAvailabilityStatus()).isEqualTo(AVAILABLE); @@ -170,7 +189,7 @@ public class CredentialManagerPreferenceControllerTest { controller.setSimulateConnectedForTests(true); assertThat(controller.isConnected()).isTrue(); controller.setSimulateHiddenForTests(Optional.of(false)); - assertThat(controller.isHiddenDueToNoProviderSet()).isFalse(); + assertThat(controller.isHiddenDueToNoProviderSet(createPair())).isFalse(); assertThat(controller.getAvailabilityStatus()).isEqualTo(AVAILABLE); // Test the data is correct. @@ -214,7 +233,7 @@ public class CredentialManagerPreferenceControllerTest { controller.setSimulateConnectedForTests(true); assertThat(controller.isConnected()).isTrue(); controller.setSimulateHiddenForTests(Optional.of(false)); - assertThat(controller.isHiddenDueToNoProviderSet()).isFalse(); + assertThat(controller.isHiddenDueToNoProviderSet(createPair())).isFalse(); assertThat(controller.getAvailabilityStatus()).isEqualTo(AVAILABLE); // Ensure that we stay under 5 providers (one is reserved for primary). @@ -283,7 +302,7 @@ public class CredentialManagerPreferenceControllerTest { controller.setSimulateConnectedForTests(true); assertThat(controller.isConnected()).isTrue(); controller.setSimulateHiddenForTests(Optional.of(false)); - assertThat(controller.isHiddenDueToNoProviderSet()).isFalse(); + assertThat(controller.isHiddenDueToNoProviderSet(createPair())).isFalse(); assertThat(controller.getAvailabilityStatus()).isEqualTo(AVAILABLE); // Test the data is correct. @@ -336,17 +355,26 @@ public class CredentialManagerPreferenceControllerTest { createControllerWithServices( Lists.newArrayList(serviceA1, serviceB1, serviceC1, serviceC2, serviceC3)); controller.setSimulateConnectedForTests(true); - controller.setSimulateHiddenForTests(Optional.of(false)); - assertThat(controller.isHiddenDueToNoProviderSet()).isFalse(); assertThat(controller.isConnected()).isTrue(); assertThat(controller.getAvailabilityStatus()).isEqualTo(AVAILABLE); - controller.displayPreference(mScreen); - assertThat(mCredentialsPreferenceCategory.getPreferenceCount()).isEqualTo(3); + CombinedProviderInfo combinedProviderA = + new CombinedProviderInfo(Lists.newArrayList(serviceA1), null, false, false); + CombinedProviderInfo combinedProviderB = + new CombinedProviderInfo(Lists.newArrayList(serviceB1), null, false, false); + CombinedProviderInfo combinedProviderC = + new CombinedProviderInfo( + Lists.newArrayList(serviceC1, serviceC2, serviceC3), null, false, false); + Pair, CombinedProviderInfo> providerPair = + createPair( + Lists.newArrayList(combinedProviderA, combinedProviderB, combinedProviderC), + createCombinedProviderInfo()); + + assertThat(controller.isHiddenDueToNoProviderSet(providerPair)).isFalse(); Map prefs = - controller.buildPreferenceList(mContext, mCredentialsPreferenceCategory); + controller.buildPreferenceList(mContext, providerPair); assertThat(prefs.keySet()) .containsExactly(TEST_PACKAGE_NAME_A, TEST_PACKAGE_NAME_B, TEST_PACKAGE_NAME_C); assertThat(prefs.size()).isEqualTo(3); @@ -531,8 +559,7 @@ public class CredentialManagerPreferenceControllerTest { @Test public void hasNonPrimaryServices_allServicesArePrimary() { CredentialManagerPreferenceController controller = - createControllerWithServices( - Lists.newArrayList(createCredentialProviderPrimary())); + createControllerWithServices(Lists.newArrayList(createCredentialProviderPrimary())); assertThat(controller.hasNonPrimaryServices()).isFalse(); } @@ -540,8 +567,8 @@ public class CredentialManagerPreferenceControllerTest { public void hasNonPrimaryServices_mixtureOfServices() { CredentialManagerPreferenceController controller = createControllerWithServices( - Lists.newArrayList(createCredentialProviderInfo(), - createCredentialProviderPrimary())); + Lists.newArrayList( + createCredentialProviderInfo(), createCredentialProviderPrimary())); assertThat(controller.hasNonPrimaryServices()).isTrue(); } @@ -599,11 +626,25 @@ public class CredentialManagerPreferenceControllerTest { private CredentialProviderInfo createCredentialProviderPrimary() { return createCredentialProviderInfoBuilder( - "com.android.primary", "CredManProvider", "Service Label", "App Name") + "com.android.primary", "CredManProvider", "Service Label", "App Name") .setPrimary(true) .build(); } + private Pair, CombinedProviderInfo> createPair() { + return createPair(Lists.newArrayList(), null); + } + + private Pair, CombinedProviderInfo> createPair( + List providers, @Nullable CombinedProviderInfo primaryProvider) { + return new Pair<>(providers, primaryProvider); + } + + private CombinedProviderInfo createCombinedProviderInfo() { + return new CombinedProviderInfo( + Lists.newArrayList(createCredentialProviderInfo()), null, false, false); + } + private CredentialProviderInfo createCredentialProviderInfoWithSubtitle( String packageName, String className, CharSequence label, CharSequence subtitle) { ServiceInfo si = new ServiceInfo();