Merge "[Status Bar] Append "__external" to external icons." into tm-qpr-dev am: 9cf44a2aa0

Original change: https://googleplex-android-review.googlesource.com/c/platform/frameworks/base/+/20961401

Change-Id: Id976f2b84cf753501a1c99833a478e240c2f8a49
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

packages/SystemUI/src/com/android/systemui/qs/external/TileServices.java

@@ -132,7 +132,7 @@ public class TileServices extends IQSService.Stub {
             final String slot = tile.getComponent().getClassName();
             // TileServices doesn't know how to add more than 1 icon per slot, so remove all
             mMainHandler.post(() -> mHost.getIconController()
-                    .removeAllIconsForSlot(slot));
+                    .removeAllIconsForExternalSlot(slot));
         }
     }
 

packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarIconController.java

@@ -79,12 +79,30 @@ public interface StatusBarIconController {
 
     /** Refresh the state of an IconManager by recreating the views */
     void refreshIconGroup(IconManager iconManager);
-    /** */
+
+    /**
+     * Adds or updates an icon for a given slot for a **tile service icon**.
+     *
+     * TODO(b/265307726): Merge with {@link #setIcon(String, StatusBarIcon)} or make this method
+     *   much more clearly distinct from that method.
+     */
     void setExternalIcon(String slot);
-    /** */
+
+    /**
+     * Adds or updates an icon for the given slot for **internal system icons**.
+     *
+     * TODO(b/265307726): Rename to `setInternalIcon`, or merge this appropriately with the
+     * {@link #setIcon(String, StatusBarIcon)} method.
+     */
     void setIcon(String slot, int resourceId, CharSequence contentDescription);
-    /** */
+
+    /**
+     * Adds or updates an icon for the given slot for an **externally-provided icon**.
+     *
+     * TODO(b/265307726): Rename to `setExternalIcon` or something similar.
+     */
     void setIcon(String slot, StatusBarIcon icon);
+
     /** */
     void setWifiIcon(String slot, WifiIconState state);
 
@@ -133,9 +151,17 @@ public interface StatusBarIconController {
      * TAG_PRIMARY to refer to the first icon at a given slot.
      */
     void removeIcon(String slot, int tag);
+
     /** */
     void removeAllIconsForSlot(String slot);
 
+    /**
+     * Removes all the icons for the given slot.
+     *
+     * Only use this for icons that have come from **an external process**.
+     */
+    void removeAllIconsForExternalSlot(String slot);
+
     // TODO: See if we can rename this tunable name.
     String ICON_HIDE_LIST = "icon_blacklist";
 

packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarIconControllerImpl.java

@@ -28,6 +28,8 @@ import android.util.ArraySet;
 import android.util.Log;
 import android.view.ViewGroup;
 
+import androidx.annotation.VisibleForTesting;
+
 import com.android.internal.statusbar.StatusBarIcon;
 import com.android.systemui.Dumpable;
 import com.android.systemui.R;
@@ -63,6 +65,10 @@ public class StatusBarIconControllerImpl implements Tunable,
         ConfigurationListener, Dumpable, CommandQueue.Callbacks, StatusBarIconController, DemoMode {
 
     private static final String TAG = "StatusBarIconController";
+    // Use this suffix to prevent external icon slot names from unintentionally overriding our
+    // internal, system-level slot names. See b/255428281.
+    @VisibleForTesting
+    protected static final String EXTERNAL_SLOT_SUFFIX = "__external";
 
     private final StatusBarIconList mStatusBarIconList;
     private final ArrayList<IconManager> mIconGroups = new ArrayList<>();
@@ -346,21 +352,26 @@ public class StatusBarIconControllerImpl implements Tunable,
 
     @Override
     public void setExternalIcon(String slot) {
-        int viewIndex = mStatusBarIconList.getViewIndex(slot, 0);
+        String slotName = createExternalSlotName(slot);
+        int viewIndex = mStatusBarIconList.getViewIndex(slotName, 0);
         int height = mContext.getResources().getDimensionPixelSize(
                 R.dimen.status_bar_icon_drawing_size);
         mIconGroups.forEach(l -> l.onIconExternal(viewIndex, height));
     }
 
-    //TODO: remove this (used in command queue and for 3rd party tiles?)
+    // Override for *both* CommandQueue.Callbacks AND StatusBarIconController.
+    // TODO(b/265307726): Pull out the CommandQueue callbacks into a member variable to
+    //  differentiate between those callback methods and StatusBarIconController methods.
+    @Override
     public void setIcon(String slot, StatusBarIcon icon) {
+        String slotName = createExternalSlotName(slot);
         if (icon == null) {
-            removeAllIconsForSlot(slot);
+            removeAllIconsForSlot(slotName);
             return;
         }
 
         StatusBarIconHolder holder = StatusBarIconHolder.fromIcon(icon);
-        setIcon(slot, holder);
+        setIcon(slotName, holder);
     }
 
     private void setIcon(String slot, @NonNull StatusBarIconHolder holder) {
@@ -406,10 +417,12 @@ public class StatusBarIconControllerImpl implements Tunable,
         }
     }
 
-    /** */
+    // CommandQueue.Callbacks override
+    // TODO(b/265307726): Pull out the CommandQueue callbacks into a member variable to
+    //  differentiate between those callback methods and StatusBarIconController methods.
     @Override
     public void removeIcon(String slot) {
-        removeAllIconsForSlot(slot);
+        removeAllIconsForExternalSlot(slot);
     }
 
     /** */
@@ -423,6 +436,11 @@ public class StatusBarIconControllerImpl implements Tunable,
         mIconGroups.forEach(l -> l.onRemoveIcon(viewIndex));
     }
 
+    @Override
+    public void removeAllIconsForExternalSlot(String slotName) {
+        removeAllIconsForSlot(createExternalSlotName(slotName));
+    }
+
     /** */
     @Override
     public void removeAllIconsForSlot(String slotName) {
@@ -506,4 +524,12 @@ public class StatusBarIconControllerImpl implements Tunable,
     public void onDensityOrFontScaleChanged() {
         refreshIconGroups();
     }
+
+    private String createExternalSlotName(String slot) {
+        if (slot.endsWith(EXTERNAL_SLOT_SUFFIX)) {
+            return slot;
+        } else {
+            return slot + EXTERNAL_SLOT_SUFFIX;
+        }
+    }
 }

packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarIconControllerImplTest.kt

@@ -0,0 +1,309 @@
+/*
+ * Copyright (C) 2023 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.systemui.statusbar.phone
+
+import android.os.UserHandle
+import androidx.test.filters.SmallTest
+import com.android.internal.statusbar.StatusBarIcon
+import com.android.systemui.SysuiTestCase
+import com.android.systemui.statusbar.phone.StatusBarIconController.TAG_PRIMARY
+import com.android.systemui.statusbar.phone.StatusBarIconControllerImpl.EXTERNAL_SLOT_SUFFIX
+import com.android.systemui.util.mockito.mock
+import com.google.common.truth.Truth.assertThat
+import org.junit.Before
+import org.junit.Test
+import org.mockito.Mockito.verify
+
+@SmallTest
+class StatusBarIconControllerImplTest : SysuiTestCase() {
+
+    private lateinit var underTest: StatusBarIconControllerImpl
+
+    private lateinit var iconList: StatusBarIconList
+    private val iconGroup: StatusBarIconController.IconManager = mock()
+
+    @Before
+    fun setUp() {
+        iconList = StatusBarIconList(arrayOf())
+        underTest =
+            StatusBarIconControllerImpl(
+                context,
+                mock(),
+                mock(),
+                mock(),
+                mock(),
+                mock(),
+                iconList,
+                mock(),
+            )
+        underTest.addIconGroup(iconGroup)
+    }
+
+    /** Regression test for b/255428281. */
+    @Test
+    fun internalAndExternalIconWithSameName_bothDisplayed() {
+        val slotName = "mute"
+
+        // Internal
+        underTest.setIcon(slotName, /* resourceId= */ 10, "contentDescription")
+
+        // External
+        val externalIcon =
+            StatusBarIcon(
+                "external.package",
+                UserHandle.ALL,
+                /* iconId= */ 2,
+                /* iconLevel= */ 0,
+                /* number= */ 0,
+                "contentDescription",
+            )
+        underTest.setIcon(slotName, externalIcon)
+
+        assertThat(iconList.slots).hasSize(2)
+        // Whichever was added last comes first
+        assertThat(iconList.slots[0].name).isEqualTo(slotName + EXTERNAL_SLOT_SUFFIX)
+        assertThat(iconList.slots[1].name).isEqualTo(slotName)
+        assertThat(iconList.slots[0].hasIconsInSlot()).isTrue()
+        assertThat(iconList.slots[1].hasIconsInSlot()).isTrue()
+    }
+
+    /** Regression test for b/255428281. */
+    @Test
+    fun internalAndExternalIconWithSameName_externalRemoved_viaRemoveIcon_internalStays() {
+        val slotName = "mute"
+
+        // Internal
+        underTest.setIcon(slotName, /* resourceId= */ 10, "contentDescription")
+
+        // External
+        underTest.setIcon(slotName, createExternalIcon())
+
+        // WHEN the external icon is removed via #removeIcon
+        underTest.removeIcon(slotName)
+
+        // THEN the external icon is removed but the internal icon remains
+        // Note: [StatusBarIconList] never removes slots from its list, it just sets the holder for
+        // the slot to null when an icon is removed.
+        assertThat(iconList.slots).hasSize(2)
+        assertThat(iconList.slots[0].name).isEqualTo(slotName + EXTERNAL_SLOT_SUFFIX)
+        assertThat(iconList.slots[1].name).isEqualTo(slotName)
+        assertThat(iconList.slots[0].hasIconsInSlot()).isFalse() // Indicates removal
+        assertThat(iconList.slots[1].hasIconsInSlot()).isTrue()
+
+        verify(iconGroup).onRemoveIcon(0)
+    }
+
+    /** Regression test for b/255428281. */
+    @Test
+    fun internalAndExternalIconWithSameName_externalRemoved_viaRemoveAll_internalStays() {
+        val slotName = "mute"
+
+        // Internal
+        underTest.setIcon(slotName, /* resourceId= */ 10, "contentDescription")
+
+        // External
+        underTest.setIcon(slotName, createExternalIcon())
+
+        // WHEN the external icon is removed via #removeAllIconsForExternalSlot
+        underTest.removeAllIconsForExternalSlot(slotName)
+
+        // THEN the external icon is removed but the internal icon remains
+        assertThat(iconList.slots).hasSize(2)
+        assertThat(iconList.slots[0].name).isEqualTo(slotName + EXTERNAL_SLOT_SUFFIX)
+        assertThat(iconList.slots[1].name).isEqualTo(slotName)
+        assertThat(iconList.slots[0].hasIconsInSlot()).isFalse() // Indicates removal
+        assertThat(iconList.slots[1].hasIconsInSlot()).isTrue()
+
+        verify(iconGroup).onRemoveIcon(0)
+    }
+
+    /** Regression test for b/255428281. */
+    @Test
+    fun internalAndExternalIconWithSameName_externalRemoved_viaSetNull_internalStays() {
+        val slotName = "mute"
+
+        // Internal
+        underTest.setIcon(slotName, /* resourceId= */ 10, "contentDescription")
+
+        // External
+        underTest.setIcon(slotName, createExternalIcon())
+
+        // WHEN the external icon is removed via a #setIcon(null)
+        underTest.setIcon(slotName, /* icon= */ null)
+
+        // THEN the external icon is removed but the internal icon remains
+        assertThat(iconList.slots).hasSize(2)
+        assertThat(iconList.slots[0].name).isEqualTo(slotName + EXTERNAL_SLOT_SUFFIX)
+        assertThat(iconList.slots[1].name).isEqualTo(slotName)
+        assertThat(iconList.slots[0].hasIconsInSlot()).isFalse() // Indicates removal
+        assertThat(iconList.slots[1].hasIconsInSlot()).isTrue()
+
+        verify(iconGroup).onRemoveIcon(0)
+    }
+
+    /** Regression test for b/255428281. */
+    @Test
+    fun internalAndExternalIconWithSameName_internalRemoved_viaRemove_externalStays() {
+        val slotName = "mute"
+
+        // Internal
+        underTest.setIcon(slotName, /* resourceId= */ 10, "contentDescription")
+
+        // External
+        underTest.setIcon(slotName, createExternalIcon())
+
+        // WHEN the internal icon is removed via #removeIcon
+        underTest.removeIcon(slotName, /* tag= */ 0)
+
+        // THEN the external icon is removed but the internal icon remains
+        assertThat(iconList.slots).hasSize(2)
+        assertThat(iconList.slots[0].name).isEqualTo(slotName + EXTERNAL_SLOT_SUFFIX)
+        assertThat(iconList.slots[1].name).isEqualTo(slotName)
+        assertThat(iconList.slots[0].hasIconsInSlot()).isTrue()
+        assertThat(iconList.slots[1].hasIconsInSlot()).isFalse() // Indicates removal
+
+        verify(iconGroup).onRemoveIcon(1)
+    }
+
+    /** Regression test for b/255428281. */
+    @Test
+    fun internalAndExternalIconWithSameName_internalRemoved_viaRemoveAll_externalStays() {
+        val slotName = "mute"
+
+        // Internal
+        underTest.setIcon(slotName, /* resourceId= */ 10, "contentDescription")
+
+        // External
+        underTest.setIcon(slotName, createExternalIcon())
+
+        // WHEN the internal icon is removed via #removeAllIconsForSlot
+        underTest.removeAllIconsForSlot(slotName)
+
+        // THEN the external icon is removed but the internal icon remains
+        assertThat(iconList.slots).hasSize(2)
+        assertThat(iconList.slots[0].name).isEqualTo(slotName + EXTERNAL_SLOT_SUFFIX)
+        assertThat(iconList.slots[1].name).isEqualTo(slotName)
+        assertThat(iconList.slots[0].hasIconsInSlot()).isTrue()
+        assertThat(iconList.slots[1].hasIconsInSlot()).isFalse() // Indicates removal
+
+        verify(iconGroup).onRemoveIcon(1)
+    }
+
+    /** Regression test for b/255428281. */
+    @Test
+    fun internalAndExternalIconWithSameName_internalUpdatedIndependently() {
+        val slotName = "mute"
+
+        // Internal
+        underTest.setIcon(slotName, /* resourceId= */ 10, "contentDescription")
+
+        // External
+        val startingExternalIcon =
+            StatusBarIcon(
+                "external.package",
+                UserHandle.ALL,
+                /* iconId= */ 20,
+                /* iconLevel= */ 0,
+                /* number= */ 0,
+                "externalDescription",
+            )
+        underTest.setIcon(slotName, startingExternalIcon)
+
+        // WHEN the internal icon is updated
+        underTest.setIcon(slotName, /* resourceId= */ 11, "newContentDescription")
+
+        // THEN only the internal slot gets the updates
+        val internalSlot = iconList.slots[1]
+        val internalHolder = internalSlot.getHolderForTag(TAG_PRIMARY)!!
+        assertThat(internalSlot.name).isEqualTo(slotName)
+        assertThat(internalHolder.icon!!.contentDescription).isEqualTo("newContentDescription")
+        assertThat(internalHolder.icon!!.icon.resId).isEqualTo(11)
+
+        // And the external slot has its own values
+        val externalSlot = iconList.slots[0]
+        val externalHolder = externalSlot.getHolderForTag(TAG_PRIMARY)!!
+        assertThat(externalSlot.name).isEqualTo(slotName + EXTERNAL_SLOT_SUFFIX)
+        assertThat(externalHolder.icon!!.contentDescription).isEqualTo("externalDescription")
+        assertThat(externalHolder.icon!!.icon.resId).isEqualTo(20)
+    }
+
+    /** Regression test for b/255428281. */
+    @Test
+    fun internalAndExternalIconWithSameName_externalUpdatedIndependently() {
+        val slotName = "mute"
+
+        // Internal
+        underTest.setIcon(slotName, /* resourceId= */ 10, "contentDescription")
+
+        // External
+        val startingExternalIcon =
+            StatusBarIcon(
+                "external.package",
+                UserHandle.ALL,
+                /* iconId= */ 20,
+                /* iconLevel= */ 0,
+                /* number= */ 0,
+                "externalDescription",
+            )
+        underTest.setIcon(slotName, startingExternalIcon)
+
+        // WHEN the external icon is updated
+        val newExternalIcon =
+            StatusBarIcon(
+                "external.package",
+                UserHandle.ALL,
+                /* iconId= */ 21,
+                /* iconLevel= */ 0,
+                /* number= */ 0,
+                "newExternalDescription",
+            )
+        underTest.setIcon(slotName, newExternalIcon)
+
+        // THEN only the external slot gets the updates
+        val externalSlot = iconList.slots[0]
+        val externalHolder = externalSlot.getHolderForTag(TAG_PRIMARY)!!
+        assertThat(externalSlot.name).isEqualTo(slotName + EXTERNAL_SLOT_SUFFIX)
+        assertThat(externalHolder.icon!!.contentDescription).isEqualTo("newExternalDescription")
+        assertThat(externalHolder.icon!!.icon.resId).isEqualTo(21)
+
+        // And the internal slot has its own values
+        val internalSlot = iconList.slots[1]
+        val internalHolder = internalSlot.getHolderForTag(TAG_PRIMARY)!!
+        assertThat(internalSlot.name).isEqualTo(slotName)
+        assertThat(internalHolder.icon!!.contentDescription).isEqualTo("contentDescription")
+        assertThat(internalHolder.icon!!.icon.resId).isEqualTo(10)
+    }
+
+    @Test
+    fun externalSlot_alreadyEndsWithSuffix_suffixNotAddedTwice() {
+        underTest.setIcon("myslot$EXTERNAL_SLOT_SUFFIX", createExternalIcon())
+
+        assertThat(iconList.slots).hasSize(1)
+        assertThat(iconList.slots[0].name).isEqualTo("myslot$EXTERNAL_SLOT_SUFFIX")
+    }
+
+    private fun createExternalIcon(): StatusBarIcon {
+        return StatusBarIcon(
+            "external.package",
+            UserHandle.ALL,
+            /* iconId= */ 2,
+            /* iconLevel= */ 0,
+            /* number= */ 0,
+            "contentDescription",
+        )
+    }
+}

packages/SystemUI/tests/utils/src/com/android/systemui/utils/leaks/FakeStatusBarIconController.java

@@ -97,6 +97,10 @@ public class FakeStatusBarIconController extends BaseLeakChecker<IconManager>
     public void removeAllIconsForSlot(String slot) {
     }
 
+    @Override
+    public void removeAllIconsForExternalSlot(String slot) {
+    }
+
     @Override
     public void setIconAccessibilityLiveRegion(String slot, int mode) {
     }