diff options
author | Caitlin Shkuratov <caitlinshk@google.com> | 2023-01-12 18:39:18 +0000 |
---|---|---|
committer | Android Build Coastguard Worker <android-build-coastguard-worker@google.com> | 2023-01-17 20:08:45 +0000 |
commit | d2b4515086d810fb2928fbc9c805177e2383637d (patch) | |
tree | 9f154e0720a0830b5264507f7df6447d8cd02bc5 | |
parent | e7a5f0bf676f93d233476d98c8c34b775a1e88fe (diff) | |
download | base-d2b4515086d810fb2928fbc9c805177e2383637d.tar.gz |
[Status Bar] Append "__external" to external icons.
Bug: 255428281
Bug: 265307726
Test: manual: (1) Be in silent mode. (2) Make a phone call. (3) Mute the
phone call. -> Verify that *both* silent *and* mute icons appear in the
status bar. Verify that they can each be toggled independently without
affecting each other.
Test: atest StatusBarIconControllerImplTest (verified that all added
tests would have failed without the changes in this CL)
Change-Id: I54cebf7b9b96c1b333f1f2a9ca3d427e9e765790
(cherry picked from commit afd1fc252bcb0336b300da71178b716767c82f9a)
Merged-In: I54cebf7b9b96c1b333f1f2a9ca3d427e9e765790
5 files changed, 375 insertions, 10 deletions
diff --git a/packages/SystemUI/src/com/android/systemui/qs/external/TileServices.java b/packages/SystemUI/src/com/android/systemui/qs/external/TileServices.java index 3d48fd109e39..84a18d8dd365 100644 --- a/packages/SystemUI/src/com/android/systemui/qs/external/TileServices.java +++ b/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)); } } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarIconController.java b/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarIconController.java index 1a14a0363763..24ad55d67bb0 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarIconController.java +++ b/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"; diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarIconControllerImpl.java b/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarIconControllerImpl.java index 9fbe6cbc0e32..416bc7141eeb 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarIconControllerImpl.java +++ b/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; + } + } } diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarIconControllerImplTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarIconControllerImplTest.kt new file mode 100644 index 000000000000..3bc288a2f823 --- /dev/null +++ b/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", + ) + } +} diff --git a/packages/SystemUI/tests/utils/src/com/android/systemui/utils/leaks/FakeStatusBarIconController.java b/packages/SystemUI/tests/utils/src/com/android/systemui/utils/leaks/FakeStatusBarIconController.java index 2d6d29a50a74..926c6c56a862 100644 --- a/packages/SystemUI/tests/utils/src/com/android/systemui/utils/leaks/FakeStatusBarIconController.java +++ b/packages/SystemUI/tests/utils/src/com/android/systemui/utils/leaks/FakeStatusBarIconController.java @@ -98,6 +98,10 @@ public class FakeStatusBarIconController extends BaseLeakChecker<IconManager> } @Override + public void removeAllIconsForExternalSlot(String slot) { + } + + @Override public void setIconAccessibilityLiveRegion(String slot, int mode) { } |