Merge "Fix bug where empty section is sometimes shown" into main

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<Boolean> 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<CredentialProviderInfo> newProviders =
+        setAvailableServices(
                 mCredentialManager.getCredentialProviderServices(
                         getUser(),
-                        CredentialManager.PROVIDER_FILTER_USER_PROVIDERS_INCLUDING_HIDDEN);
+                        CredentialManager.PROVIDER_FILTER_USER_PROVIDERS_INCLUDING_HIDDEN),
-        Set<ComponentName> newComponents = buildComponentNameSet(newProviders, false);
+                null);
-        Set<ComponentName> newPrimaryComponents = buildComponentNameSet(newProviders, true);
-
-        // Get the list of old components
-        Set<ComponentName> oldComponents = buildComponentNameSet(mServices, false);
-        Set<ComponentName> 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);
 
         if (mPreferenceScreen != null) {
             displayPreference(mPreferenceScreen);
@@ -396,11 +378,7 @@ public class CredentialManagerPreferenceController extends BasePreferenceControl
     }
 
     @VisibleForTesting
-    public boolean isHiddenDueToNoProviderSet() {
+    public boolean isHiddenDueToNoProviderSet(
-        return isHiddenDueToNoProviderSet(getProviders());
-    }
-
-    private boolean isHiddenDueToNoProviderSet(
             Pair<List<CombinedProviderInfo>, 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)) {
-        // Since the UI is being cleared, clear any refs.
+            Log.w(TAG, "Skipping displayPreference because key is empty");
-        mPrefs.clear();
+            return;
+        }
 
+        // Store this reference for later.
+        if (mPreferenceScreen == null) {
             mPreferenceScreen = screen;
-        PreferenceGroup group = screen.findPreference(getPreferenceKey());
+            mPreferenceGroup = screen.findPreference(prefKey);
-        group.removeAll();
+        }
 
-        Context context = screen.getContext();
+        final Pair<List<CombinedProviderInfo>, CombinedProviderInfo> providerPair = getProviders();
-        mPrefs.putAll(buildPreferenceList(context, group));
+
+        maybeUpdateListOfPrefs(providerPair);
+        maybeUpdatePreferenceVisibility(providerPair);
+    }
+
+    private void maybeUpdateListOfPrefs(
+            Pair<List<CombinedProviderInfo>, CombinedProviderInfo> providerPair) {
+        if (mPreferenceScreen == null || mPreferenceGroup == null) {
+            return;
+        }
+
+        // Build the new list of prefs.
+        Map<String, CombiPreference> newPrefs =
+                buildPreferenceList(mPreferenceScreen.getContext(), providerPair);
+
+        // Determine if we need to update the prefs.
+        Set<String> existingPrefPackageNames = mPrefs.keySet();
+        if (existingPrefPackageNames.equals(newPrefs.keySet())) {
+            return;
+        }
+
+        // Since the UI is being cleared, clear any refs and prefs.
+        mPrefs.clear();
+        mPreferenceGroup.removeAll();
+
+        // Populate the preference list with new data.
+        mPrefs.putAll(newPrefs);
+        for (CombiPreference pref : newPrefs.values()) {
+            mPreferenceGroup.addPreference(pref);
+        }
+    }
+
+    private void maybeUpdatePreferenceVisibility(
+            Pair<List<CombinedProviderInfo>, 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<String, CombiPreference> buildPreferenceList(
-            @NonNull Context context, @NonNull PreferenceGroup group) {
+            @NonNull Context context,
-        // Get the providers and extract the values.
+            @NonNull Pair<List<CombinedProviderInfo>, CombinedProviderInfo> providerPair) {
-        Pair<List<CombinedProviderInfo>, CombinedProviderInfo> providerPair = getProviders();
+        // Extract the values.
         CombinedProviderInfo topProvider = providerPair.second;
         List<CombinedProviderInfo> 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,7 +1050,8 @@ 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())) {
+                    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;
@@ -1083,8 +1111,10 @@ public class CredentialManagerPreferenceController extends BasePreferenceControl
 
             if (mSwitch != null && !TextUtils.isEmpty(appName)) {
                 mSwitch.setContentDescription(
-                    getContext().getString(
+                        getContext()
-                        R.string.credman_on_off_switch_content_description, appName));
+                                .getString(
+                                        R.string.credman_on_off_switch_content_description,
+                                        appName));
             }
         }
 

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.isHiddenDueToNoProviderSet(createPair())).isTrue();
-        assertThat(controller.isConnected()).isTrue();
+    }
-        controller.setSimulateHiddenForTests(Optional.of(true));
+
-        assertThat(controller.isHiddenDueToNoProviderSet()).isTrue();
+    @Test
-        assertThat(controller.getAvailabilityStatus()).isEqualTo(CONDITIONALLY_UNAVAILABLE);
+    public void isHiddenDueToNoProviderSet_hiddenDueToNoPrimaryProvider() {
+        CredentialManagerPreferenceController controller =
+                createControllerWithServices(Lists.newArrayList(createCredentialProviderInfo()));
+
+        Pair<List<CombinedProviderInfo>, 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<List<CombinedProviderInfo>, 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);
+        CombinedProviderInfo combinedProviderA =
-        assertThat(mCredentialsPreferenceCategory.getPreferenceCount()).isEqualTo(3);
+                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<List<CombinedProviderInfo>, CombinedProviderInfo> providerPair =
+                createPair(
+                        Lists.newArrayList(combinedProviderA, combinedProviderB, combinedProviderC),
+                        createCombinedProviderInfo());
+
+        assertThat(controller.isHiddenDueToNoProviderSet(providerPair)).isFalse();
 
         Map<String, CredentialManagerPreferenceController.CombiPreference> 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(
+                createControllerWithServices(Lists.newArrayList(createCredentialProviderPrimary()));
-                    Lists.newArrayList(createCredentialProviderPrimary()));
         assertThat(controller.hasNonPrimaryServices()).isFalse();
     }
 
@@ -540,8 +567,8 @@ public class CredentialManagerPreferenceControllerTest {
     public void hasNonPrimaryServices_mixtureOfServices() {
         CredentialManagerPreferenceController controller =
                 createControllerWithServices(
-                    Lists.newArrayList(createCredentialProviderInfo(),
+                        Lists.newArrayList(
-                        createCredentialProviderPrimary()));
+                                createCredentialProviderInfo(), createCredentialProviderPrimary()));
         assertThat(controller.hasNonPrimaryServices()).isTrue();
     }
 
@@ -604,6 +631,20 @@ public class CredentialManagerPreferenceControllerTest {
                 .build();
     }
 
+    private Pair<List<CombinedProviderInfo>, CombinedProviderInfo> createPair() {
+        return createPair(Lists.newArrayList(), null);
+    }
+
+    private Pair<List<CombinedProviderInfo>, CombinedProviderInfo> createPair(
+            List<CombinedProviderInfo> 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();