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 0727c5af0dc78..0422d694b70d4 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarIconControllerImpl.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarIconControllerImpl.java @@ -62,7 +62,7 @@ import javax.inject.Inject; */ @SysUISingleton public class StatusBarIconControllerImpl implements Tunable, - ConfigurationListener, Dumpable, CommandQueue.Callbacks, StatusBarIconController, DemoMode { + ConfigurationListener, Dumpable, StatusBarIconController, DemoMode { private static final String TAG = "StatusBarIconController"; // Use this suffix to prevent external icon slot names from unintentionally overriding our @@ -93,7 +93,7 @@ public class StatusBarIconControllerImpl implements Tunable, mStatusBarPipelineFlags = statusBarPipelineFlags; configurationController.addCallback(this); - commandQueue.addCallback(this); + commandQueue.addCallback(mCommandQueueCallbacks); tunerService.addTunable(this, ICON_HIDE_LIST); demoModeController.addCallback(this); dumpManager.registerDumpable(getClass().getSimpleName(), this); @@ -350,6 +350,8 @@ 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); @@ -359,11 +361,12 @@ public class StatusBarIconControllerImpl implements Tunable, mIconGroups.forEach(l -> l.onIconExternal(viewIndex, height)); } - // 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) { + setExternalIcon(slot, icon); + } + + private void setExternalIcon(String slot, StatusBarIcon icon) { String slotName = createExternalSlotName(slot); if (icon == null) { removeAllIconsForSlot(slotName); @@ -374,6 +377,19 @@ public class StatusBarIconControllerImpl implements Tunable, setIcon(slotName, holder); } + private final CommandQueue.Callbacks mCommandQueueCallbacks = new CommandQueue.Callbacks() { + @Override + public void setIcon(String slot, StatusBarIcon icon) { + // Icons that come from CommandQueue are from external services. + setExternalIcon(slot, icon); + } + + @Override + public void removeIcon(String slot) { + removeAllIconsForExternalSlot(slot); + } + }; + private void setIcon(String slot, @NonNull StatusBarIconHolder holder) { boolean isNew = mStatusBarIconList.getIconHolder(slot, holder.getTag()) == null; mStatusBarIconList.setIcon(slot, holder); @@ -417,14 +433,6 @@ 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) { - removeAllIconsForExternalSlot(slot); - } - /** */ @Override public void removeIcon(String slot, int tag) { 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 3bc288a2f8234..5bde88300f960 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 @@ -20,13 +20,17 @@ 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.CommandQueue 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.kotlinArgumentCaptor 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.Mock import org.mockito.Mockito.verify +import org.mockito.MockitoAnnotations @SmallTest class StatusBarIconControllerImplTest : SysuiTestCase() { @@ -34,15 +38,19 @@ class StatusBarIconControllerImplTest : SysuiTestCase() { private lateinit var underTest: StatusBarIconControllerImpl private lateinit var iconList: StatusBarIconList + private lateinit var commandQueueCallbacks: CommandQueue.Callbacks private val iconGroup: StatusBarIconController.IconManager = mock() + @Mock private lateinit var commandQueue: CommandQueue + @Before fun setUp() { + MockitoAnnotations.initMocks(this) iconList = StatusBarIconList(arrayOf()) underTest = StatusBarIconControllerImpl( context, - mock(), + commandQueue, mock(), mock(), mock(), @@ -51,6 +59,9 @@ class StatusBarIconControllerImplTest : SysuiTestCase() { mock(), ) underTest.addIconGroup(iconGroup) + val commandQueueCallbacksCaptor = kotlinArgumentCaptor() + verify(commandQueue).addCallback(commandQueueCallbacksCaptor.capture()) + commandQueueCallbacks = commandQueueCallbacksCaptor.value } /** Regression test for b/255428281. */ @@ -83,17 +94,45 @@ class StatusBarIconControllerImplTest : SysuiTestCase() { /** Regression test for b/255428281. */ @Test - fun internalAndExternalIconWithSameName_externalRemoved_viaRemoveIcon_internalStays() { + fun internalAndExternalIconWithSameName_externalFromCommandQueue_bothDisplayed() { val slotName = "mute" // Internal underTest.setIcon(slotName, /* resourceId= */ 10, "contentDescription") // External - underTest.setIcon(slotName, createExternalIcon()) + val externalIcon = + StatusBarIcon( + "external.package", + UserHandle.ALL, + /* iconId= */ 2, + /* iconLevel= */ 0, + /* number= */ 0, + "contentDescription", + ) + commandQueueCallbacks.setIcon(slotName, externalIcon) - // WHEN the external icon is removed via #removeIcon - underTest.removeIcon(slotName) + 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_fromCommandQueue_internalStays() { + val slotName = "mute" + + // Internal + underTest.setIcon(slotName, /* resourceId= */ 10, "contentDescription") + + // External + commandQueueCallbacks.setIcon(slotName, createExternalIcon()) + + // WHEN the external icon is removed via CommandQueue.Callbacks#removeIcon + commandQueueCallbacks.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 @@ -288,6 +327,53 @@ class StatusBarIconControllerImplTest : SysuiTestCase() { assertThat(internalHolder.icon!!.icon.resId).isEqualTo(10) } + /** Regression test for b/255428281. */ + @Test + fun internalAndExternalIconWithSameName_fromCommandQueue_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", + ) + commandQueueCallbacks.setIcon(slotName, startingExternalIcon) + + // WHEN the external icon is updated + val newExternalIcon = + StatusBarIcon( + "external.package", + UserHandle.ALL, + /* iconId= */ 21, + /* iconLevel= */ 0, + /* number= */ 0, + "newExternalDescription", + ) + commandQueueCallbacks.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()) @@ -296,6 +382,14 @@ class StatusBarIconControllerImplTest : SysuiTestCase() { assertThat(iconList.slots[0].name).isEqualTo("myslot$EXTERNAL_SLOT_SUFFIX") } + @Test + fun externalSlot_fromCommandQueue_alreadyEndsWithSuffix_suffixNotAddedTwice() { + commandQueueCallbacks.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",