From 755737cf6e4e1d02339101761ac395ac9e33165b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabi=C3=A1n=20Kozynski?= Date: Wed, 17 May 2023 12:59:29 -0400 Subject: [PATCH] Remove removeCallbacks when processing tiles This is not needed and can lead to the following error: 1. QSPanel added a callback for tile A 2. CurrentTilesInteractor processes tiles and removes all callbacks from A. After that, the list of tiles didn't change. As it's backed by a StateFlow, consumers don't get notified of a change (they don't need to, the list is the same). 3. QSPanelControllerBase doesn't get a notified so it doesn't re-add the callback. The error is not usually observable, as it requires the tile setting to change but the list of actual tiles not to change, though certain scenarios could make it happen (like restoring from backup with tiles that are not available, or changing tiles through adb). Also, have QSPanelControllerBase only remove the associated callback (and not all of them) onViewDetached. Flag: QS_PIPELINE_NEW_HOST Fixes: 282978588 Test: atest com.android.systemui.qs Change-Id: I6451c80a7595bd3b3614de7b2f2870f6e97ba9d1 --- .../src/com/android/systemui/qs/QSPanel.java | 2 +- .../systemui/qs/QSPanelControllerBase.java | 2 +- .../interactor/CurrentTilesInteractor.kt | 2 -- .../qs/QSPanelControllerBaseTest.java | 26 +++++++++++++++++++ .../com/android/systemui/qs/QSPanelTest.kt | 14 ++++++++++ .../CurrentTilesInteractorImplTest.kt | 20 ++++++++++++++ .../pipeline/domain/interactor/FakeQSTile.kt | 13 +++++++--- 7 files changed, 72 insertions(+), 7 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/qs/QSPanel.java b/packages/SystemUI/src/com/android/systemui/qs/QSPanel.java index b476521f1975c..b936c41d9295a 100644 --- a/packages/SystemUI/src/com/android/systemui/qs/QSPanel.java +++ b/packages/SystemUI/src/com/android/systemui/qs/QSPanel.java @@ -522,7 +522,7 @@ public class QSPanel extends LinearLayout implements Tunable { return mExpanded; } - void addTile(QSPanelControllerBase.TileRecord tileRecord) { + final void addTile(QSPanelControllerBase.TileRecord tileRecord) { final QSTile.Callback callback = new QSTile.Callback() { @Override public void onStateChanged(QSTile.State state) { diff --git a/packages/SystemUI/src/com/android/systemui/qs/QSPanelControllerBase.java b/packages/SystemUI/src/com/android/systemui/qs/QSPanelControllerBase.java index fdab9b16c7a1f..20f035260ba4b 100644 --- a/packages/SystemUI/src/com/android/systemui/qs/QSPanelControllerBase.java +++ b/packages/SystemUI/src/com/android/systemui/qs/QSPanelControllerBase.java @@ -199,7 +199,7 @@ public abstract class QSPanelControllerBase extends ViewContr mMediaHost.removeVisibilityChangeListener(mMediaHostVisibilityListener); for (TileRecord record : mRecords) { - record.tile.removeCallbacks(); + record.tile.removeCallback(record.callback); } mRecords.clear(); mDumpManager.unregisterDumpable(mView.getDumpableTag()); diff --git a/packages/SystemUI/src/com/android/systemui/qs/pipeline/domain/interactor/CurrentTilesInteractor.kt b/packages/SystemUI/src/com/android/systemui/qs/pipeline/domain/interactor/CurrentTilesInteractor.kt index 91c6e8b5fcb6c..c579f5c3061cf 100644 --- a/packages/SystemUI/src/com/android/systemui/qs/pipeline/domain/interactor/CurrentTilesInteractor.kt +++ b/packages/SystemUI/src/com/android/systemui/qs/pipeline/domain/interactor/CurrentTilesInteractor.kt @@ -318,7 +318,6 @@ constructor( // We have a handful of different cases qsTile !is CustomTile -> { // The tile is not a custom tile. Make sure they are reset to the correct user - qsTile.removeCallbacks() if (userChanged) { qsTile.userSwitch(user) logger.logTileUserChanged(tileSpec, user) @@ -327,7 +326,6 @@ constructor( } qsTile.user == user -> { // The tile is a custom tile for the same user, just return it - qsTile.removeCallbacks() qsTile } else -> { diff --git a/packages/SystemUI/tests/src/com/android/systemui/qs/QSPanelControllerBaseTest.java b/packages/SystemUI/tests/src/com/android/systemui/qs/QSPanelControllerBaseTest.java index 3d55c5131b406..6720dae102300 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/qs/QSPanelControllerBaseTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/qs/QSPanelControllerBaseTest.java @@ -321,4 +321,30 @@ public class QSPanelControllerBaseTest extends SysuiTestCase { assertThat(mController.shouldUseHorizontalLayout()).isFalse(); verify(mHorizontalLayoutListener).run(); } + + @Test + public void changeTiles_callbackRemovedOnOldOnes() { + // Start with one tile + assertThat(mController.mRecords.size()).isEqualTo(1); + QSPanelControllerBase.TileRecord record = mController.mRecords.get(0); + + assertThat(record.tile).isEqualTo(mQSTile); + + // Change to a different tile + when(mQSHost.getTiles()).thenReturn(List.of(mOtherTile)); + mController.setTiles(); + + verify(mQSTile).removeCallback(record.callback); + verify(mOtherTile, never()).removeCallback(any()); + verify(mOtherTile, never()).removeCallbacks(); + } + + @Test + public void onViewDetached_removesJustTheAssociatedCallback() { + QSPanelControllerBase.TileRecord record = mController.mRecords.get(0); + + mController.onViewDetached(); + verify(mQSTile).removeCallback(record.callback); + verify(mQSTile, never()).removeCallbacks(); + } } diff --git a/packages/SystemUI/tests/src/com/android/systemui/qs/QSPanelTest.kt b/packages/SystemUI/tests/src/com/android/systemui/qs/QSPanelTest.kt index 93cebe2bcd169..a60dad4a14fe0 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/qs/QSPanelTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/qs/QSPanelTest.kt @@ -27,6 +27,8 @@ import androidx.test.filters.SmallTest import com.android.systemui.R import com.android.systemui.SysuiTestCase import com.android.systemui.plugins.qs.QSTile +import com.android.systemui.plugins.qs.QSTileView +import com.android.systemui.qs.QSPanelControllerBase.TileRecord import com.android.systemui.qs.logging.QSLogger import com.android.systemui.qs.tileimpl.QSIconViewImpl import com.android.systemui.qs.tileimpl.QSTileViewImpl @@ -192,6 +194,18 @@ class QSPanelTest : SysuiTestCase() { verify(accessibilityInfo, never()).addAction(actionCollapse) } + @Test + fun addTile_callbackAdded() { + val tile = mock(QSTile::class.java) + val tileView = mock(QSTileView::class.java) + + val record = TileRecord(tile, tileView) + + qsPanel.addTile(record) + + verify(tile).addCallback(record.callback) + } + private infix fun View.isLeftOf(other: View): Boolean { val rect = Rect() getBoundsOnScreen(rect) diff --git a/packages/SystemUI/tests/src/com/android/systemui/qs/pipeline/domain/interactor/CurrentTilesInteractorImplTest.kt b/packages/SystemUI/tests/src/com/android/systemui/qs/pipeline/domain/interactor/CurrentTilesInteractorImplTest.kt index 7ecb4dc9376a8..426ff670802fa 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/qs/pipeline/domain/interactor/CurrentTilesInteractorImplTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/qs/pipeline/domain/interactor/CurrentTilesInteractorImplTest.kt @@ -589,6 +589,26 @@ class CurrentTilesInteractorImplTest : SysuiTestCase() { .isTrue() } + @Test + fun retainedTiles_callbackNotRemoved() = + testScope.runTest(USER_INFO_0) { + val tiles by collectLastValue(underTest.currentTiles) + tileSpecRepository.setTiles(USER_INFO_0.id, listOf(TileSpec.create("a"))) + + val tileA = tiles!![0].tile + val callback = mock() + tileA.addCallback(callback) + + tileSpecRepository.setTiles( + USER_INFO_0.id, + listOf(TileSpec.create("a"), CUSTOM_TILE_SPEC) + ) + val newTileA = tiles!![0].tile + assertThat(tileA).isSameInstanceAs(newTileA) + + assertThat((tileA as FakeQSTile).callbacks).containsExactly(callback) + } + private fun QSTile.State.fillIn(state: Int, label: CharSequence, secondaryLabel: CharSequence) { this.state = state this.label = label diff --git a/packages/SystemUI/tests/src/com/android/systemui/qs/pipeline/domain/interactor/FakeQSTile.kt b/packages/SystemUI/tests/src/com/android/systemui/qs/pipeline/domain/interactor/FakeQSTile.kt index e50969641a715..013c925df51ed 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/qs/pipeline/domain/interactor/FakeQSTile.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/qs/pipeline/domain/interactor/FakeQSTile.kt @@ -29,6 +29,7 @@ class FakeQSTile( private var tileSpec: String? = null var destroyed = false private val state = QSTile.State() + val callbacks = mutableListOf() override fun getTileSpec(): String? { return tileSpec @@ -45,11 +46,17 @@ class FakeQSTile( override fun refreshState() {} - override fun addCallback(callback: QSTile.Callback?) {} + override fun addCallback(callback: QSTile.Callback) { + callbacks.add(callback) + } - override fun removeCallback(callback: QSTile.Callback?) {} + override fun removeCallback(callback: QSTile.Callback) { + callbacks.remove(callback) + } - override fun removeCallbacks() {} + override fun removeCallbacks() { + callbacks.clear() + } override fun createTileView(context: Context?): QSIconView? { return null