From b18dd359a5f44d2f580d60df745e6fd1feb305bd Mon Sep 17 00:00:00 2001 From: Caitlin Shkuratov Date: Tue, 14 Mar 2023 18:06:47 +0000 Subject: [PATCH] [Status bar] Clarify the API between TileServices and status bar icons. This CL re-names the StatusBarIconController APIs that are only used by TileServices to specifically have "tile" in them. Then, it's obvious why the implementation of the method considers those icons external instead of internal. This also fixes two bugs with the tile status bar icons. See first two linked bug fixes. Fixes: 273536393 Fixes: 273540833 Bug: 265307726 Test: Add tile service that has status icon -> verify icon is added (see b/265307726 for demo) Test: Remove tile service that has status icon -> verify icon is rmeoved (see b/265307726 for demo) Test: verify tile status icon is correct size Test: atest StatusBarIconControllerImplTest Change-Id: Ieac39be633e85ce5076272f9afc12f953839bedc --- .../systemui/qs/external/TileServices.java | 16 +++--- .../phone/StatusBarIconController.java | 36 ++++--------- .../phone/StatusBarIconControllerImpl.java | 51 ++++++++----------- .../phone/StatusBarIconControllerImplTest.kt | 40 +++++++-------- .../leaks/FakeStatusBarIconController.java | 16 +++--- 5 files changed, 66 insertions(+), 93 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 adc71657e6803..91ecaea02c476 100644 --- a/packages/SystemUI/src/com/android/systemui/qs/external/TileServices.java +++ b/packages/SystemUI/src/com/android/systemui/qs/external/TileServices.java @@ -132,9 +132,8 @@ public class TileServices extends IQSService.Stub { mServices.remove(tile); mTokenMap.remove(service.getToken()); mTiles.remove(tile.getComponent()); - final String slot = tile.getComponent().getClassName(); - // TileServices doesn't know how to add more than 1 icon per slot, so remove all - mMainHandler.post(() -> mStatusBarIconController.removeAllIconsForSlot(slot)); + final String slot = getStatusBarIconSlotName(tile.getComponent()); + mMainHandler.post(() -> mStatusBarIconController.removeIconForTile(slot)); } } @@ -308,12 +307,11 @@ public class TileServices extends IQSService.Stub { ? new StatusBarIcon(userHandle, packageName, icon, 0, 0, contentDescription) : null; + final String slot = getStatusBarIconSlotName(componentName); mMainHandler.post(new Runnable() { @Override public void run() { - StatusBarIconController iconController = mStatusBarIconController; - iconController.setIcon(componentName.getClassName(), statusIcon); - iconController.setExternalIcon(componentName.getClassName()); + mStatusBarIconController.setIconFromTile(slot, statusIcon); } }); } @@ -373,6 +371,12 @@ public class TileServices extends IQSService.Stub { mCommandQueue.removeCallback(mRequestListeningCallback); } + /** Returns the slot name that should be used when adding or removing status bar icons. */ + private String getStatusBarIconSlotName(ComponentName componentName) { + return componentName.getClassName(); + } + + private final CommandQueue.Callbacks mRequestListeningCallback = new CommandQueue.Callbacks() { @Override public void requestTileServiceListeningState(@NonNull ComponentName componentName) { 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 04cc8ce792d12..30d2295206d84 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarIconController.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarIconController.java @@ -81,28 +81,22 @@ public interface StatusBarIconController { void refreshIconGroup(IconManager iconManager); /** - * Adds or updates an icon for a given slot for a **tile service icon**. + * Adds or updates an icon that comes from an active tile service. * - * TODO(b/265307726): Merge with {@link #setIcon(String, StatusBarIcon)} or make this method - * much more clearly distinct from that method. + * If the icon is null, the icon will be removed. */ - void setExternalIcon(String slot); + void setIconFromTile(String slot, @Nullable StatusBarIcon icon); + + /** Removes an icon that had come from an active tile service. */ + void removeIconForTile(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. + * TODO(b/265307726): Re-name this to `setInternalIcon`. */ 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); @@ -152,15 +146,10 @@ public interface StatusBarIconController { */ 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**. + * TODO(b/265307726): Re-name this to `removeAllIconsForInternalSlot`. */ - void removeAllIconsForExternalSlot(String slot); + void removeAllIconsForSlot(String slot); // TODO: See if we can rename this tunable name. String ICON_HIDE_LIST = "icon_blacklist"; @@ -618,13 +607,6 @@ public interface StatusBarIconController { mGroup.removeAllViews(); } - protected void onIconExternal(int viewIndex, int height) { - ImageView imageView = (ImageView) mGroup.getChildAt(viewIndex); - imageView.setScaleType(ImageView.ScaleType.FIT_CENTER); - imageView.setAdjustViewBounds(true); - setHeightAndCenter(imageView, height); - } - protected void onDensityOrFontScaleChanged() { for (int i = 0; i < mGroup.getChildCount(); i++) { View child = mGroup.getChildAt(i); 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 0422d694b70d4..3a184239ac436 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarIconControllerImpl.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarIconControllerImpl.java @@ -32,7 +32,6 @@ import androidx.annotation.VisibleForTesting; import com.android.internal.statusbar.StatusBarIcon; import com.android.systemui.Dumpable; -import com.android.systemui.R; import com.android.systemui.dagger.SysUISingleton; import com.android.systemui.demomode.DemoMode; import com.android.systemui.demomode.DemoModeController; @@ -350,33 +349,6 @@ public class StatusBarIconControllerImpl implements Tunable, } } - // TODO(b/265307726): Determine why we have two [setExternalIcon] methods and why they're - // different. - @Override - public void setExternalIcon(String slot) { - 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)); - } - - @Override - public void setIcon(String slot, StatusBarIcon icon) { - setExternalIcon(slot, icon); - } - - private void setExternalIcon(String slot, StatusBarIcon icon) { - String slotName = createExternalSlotName(slot); - if (icon == null) { - removeAllIconsForSlot(slotName); - return; - } - - StatusBarIconHolder holder = StatusBarIconHolder.fromIcon(icon); - setIcon(slotName, holder); - } - private final CommandQueue.Callbacks mCommandQueueCallbacks = new CommandQueue.Callbacks() { @Override public void setIcon(String slot, StatusBarIcon icon) { @@ -390,6 +362,26 @@ public class StatusBarIconControllerImpl implements Tunable, } }; + @Override + public void setIconFromTile(String slot, StatusBarIcon icon) { + setExternalIcon(slot, icon); + } + + @Override + public void removeIconForTile(String slot) { + removeAllIconsForExternalSlot(slot); + } + + private void setExternalIcon(String slot, StatusBarIcon icon) { + if (icon == null) { + removeAllIconsForExternalSlot(slot); + return; + } + String slotName = createExternalSlotName(slot); + StatusBarIconHolder holder = StatusBarIconHolder.fromIcon(icon); + setIcon(slotName, holder); + } + private void setIcon(String slot, @NonNull StatusBarIconHolder holder) { boolean isNew = mStatusBarIconList.getIconHolder(slot, holder.getTag()) == null; mStatusBarIconList.setIcon(slot, holder); @@ -452,8 +444,7 @@ public class StatusBarIconControllerImpl implements Tunable, mIconGroups.forEach(l -> l.onRemoveIcon(viewIndex)); } - @Override - public void removeAllIconsForExternalSlot(String slotName) { + private void removeAllIconsForExternalSlot(String slotName) { removeAllIconsForSlot(createExternalSlotName(slotName)); } 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 index 5bde88300f960..08e89fbef486d 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarIconControllerImplTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/phone/StatusBarIconControllerImplTest.kt @@ -66,7 +66,7 @@ class StatusBarIconControllerImplTest : SysuiTestCase() { /** Regression test for b/255428281. */ @Test - fun internalAndExternalIconWithSameName_bothDisplayed() { + fun internalAndExternalIconWithSameName_externalFromTile_bothDisplayed() { val slotName = "mute" // Internal @@ -82,7 +82,7 @@ class StatusBarIconControllerImplTest : SysuiTestCase() { /* number= */ 0, "contentDescription", ) - underTest.setIcon(slotName, externalIcon) + underTest.setIconFromTile(slotName, externalIcon) assertThat(iconList.slots).hasSize(2) // Whichever was added last comes first @@ -148,17 +148,17 @@ class StatusBarIconControllerImplTest : SysuiTestCase() { /** Regression test for b/255428281. */ @Test - fun internalAndExternalIconWithSameName_externalRemoved_viaRemoveAll_internalStays() { + fun internalAndExternalIconWithSameName_externalRemoved_fromTileRemove_internalStays() { val slotName = "mute" // Internal underTest.setIcon(slotName, /* resourceId= */ 10, "contentDescription") // External - underTest.setIcon(slotName, createExternalIcon()) + underTest.setIconFromTile(slotName, createExternalIcon()) - // WHEN the external icon is removed via #removeAllIconsForExternalSlot - underTest.removeAllIconsForExternalSlot(slotName) + // WHEN the external icon is removed via #removeIconForTile + underTest.removeIconForTile(slotName) // THEN the external icon is removed but the internal icon remains assertThat(iconList.slots).hasSize(2) @@ -172,17 +172,17 @@ class StatusBarIconControllerImplTest : SysuiTestCase() { /** Regression test for b/255428281. */ @Test - fun internalAndExternalIconWithSameName_externalRemoved_viaSetNull_internalStays() { + fun internalAndExternalIconWithSameName_externalRemoved_fromTileSetNull_internalStays() { val slotName = "mute" // Internal underTest.setIcon(slotName, /* resourceId= */ 10, "contentDescription") // External - underTest.setIcon(slotName, createExternalIcon()) + underTest.setIconFromTile(slotName, createExternalIcon()) - // WHEN the external icon is removed via a #setIcon(null) - underTest.setIcon(slotName, /* icon= */ null) + // WHEN the external icon is removed via a #setIconFromTile(null) + underTest.setIconFromTile(slotName, /* icon= */ null) // THEN the external icon is removed but the internal icon remains assertThat(iconList.slots).hasSize(2) @@ -203,12 +203,12 @@ class StatusBarIconControllerImplTest : SysuiTestCase() { underTest.setIcon(slotName, /* resourceId= */ 10, "contentDescription") // External - underTest.setIcon(slotName, createExternalIcon()) + underTest.setIconFromTile(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 + // THEN the internal icon is removed but the external icon remains assertThat(iconList.slots).hasSize(2) assertThat(iconList.slots[0].name).isEqualTo(slotName + EXTERNAL_SLOT_SUFFIX) assertThat(iconList.slots[1].name).isEqualTo(slotName) @@ -227,12 +227,12 @@ class StatusBarIconControllerImplTest : SysuiTestCase() { underTest.setIcon(slotName, /* resourceId= */ 10, "contentDescription") // External - underTest.setIcon(slotName, createExternalIcon()) + underTest.setIconFromTile(slotName, createExternalIcon()) // WHEN the internal icon is removed via #removeAllIconsForSlot underTest.removeAllIconsForSlot(slotName) - // THEN the external icon is removed but the internal icon remains + // THEN the internal icon is removed but the external icon remains assertThat(iconList.slots).hasSize(2) assertThat(iconList.slots[0].name).isEqualTo(slotName + EXTERNAL_SLOT_SUFFIX) assertThat(iconList.slots[1].name).isEqualTo(slotName) @@ -260,7 +260,7 @@ class StatusBarIconControllerImplTest : SysuiTestCase() { /* number= */ 0, "externalDescription", ) - underTest.setIcon(slotName, startingExternalIcon) + underTest.setIconFromTile(slotName, startingExternalIcon) // WHEN the internal icon is updated underTest.setIcon(slotName, /* resourceId= */ 11, "newContentDescription") @@ -282,7 +282,7 @@ class StatusBarIconControllerImplTest : SysuiTestCase() { /** Regression test for b/255428281. */ @Test - fun internalAndExternalIconWithSameName_externalUpdatedIndependently() { + fun internalAndExternalIconWithSameName_fromTile_externalUpdatedIndependently() { val slotName = "mute" // Internal @@ -298,7 +298,7 @@ class StatusBarIconControllerImplTest : SysuiTestCase() { /* number= */ 0, "externalDescription", ) - underTest.setIcon(slotName, startingExternalIcon) + underTest.setIconFromTile(slotName, startingExternalIcon) // WHEN the external icon is updated val newExternalIcon = @@ -310,7 +310,7 @@ class StatusBarIconControllerImplTest : SysuiTestCase() { /* number= */ 0, "newExternalDescription", ) - underTest.setIcon(slotName, newExternalIcon) + underTest.setIconFromTile(slotName, newExternalIcon) // THEN only the external slot gets the updates val externalSlot = iconList.slots[0] @@ -375,8 +375,8 @@ class StatusBarIconControllerImplTest : SysuiTestCase() { } @Test - fun externalSlot_alreadyEndsWithSuffix_suffixNotAddedTwice() { - underTest.setIcon("myslot$EXTERNAL_SLOT_SUFFIX", createExternalIcon()) + fun externalSlot_fromTile_alreadyEndsWithSuffix_suffixNotAddedTwice() { + underTest.setIconFromTile("myslot$EXTERNAL_SLOT_SUFFIX", createExternalIcon()) assertThat(iconList.slots).hasSize(1) assertThat(iconList.slots[0].name).isEqualTo("myslot$EXTERNAL_SLOT_SUFFIX") 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 926c6c56a8620..c664c99cf2a77 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 @@ -47,7 +47,12 @@ public class FakeStatusBarIconController extends BaseLeakChecker } @Override - public void setExternalIcon(String slot) { + public void setIconFromTile(String slot, StatusBarIcon icon) { + + } + + @Override + public void removeIconForTile(String slot) { } @@ -56,11 +61,6 @@ public class FakeStatusBarIconController extends BaseLeakChecker } - @Override - public void setIcon(String slot, StatusBarIcon icon) { - - } - @Override public void setWifiIcon(String slot, WifiIconState state) { } @@ -97,10 +97,6 @@ public class FakeStatusBarIconController extends BaseLeakChecker public void removeAllIconsForSlot(String slot) { } - @Override - public void removeAllIconsForExternalSlot(String slot) { - } - @Override public void setIconAccessibilityLiveRegion(String slot, int mode) { }