From c5b1bc80bc04d8e1c51d7f7ae83f2481fa9fef85 Mon Sep 17 00:00:00 2001 From: Amin Shaikh Date: Wed, 21 Mar 2018 08:13:47 -0400 Subject: [PATCH] Ensure TileQueryHelper#mTiles is thread safe. - Ensure TileQueryHelper#mTiles is only modified on the background thread and a send a copy to listeners. - Do not call onTilesChanged until all stock tiles are loaded (as opposed to after each tile) to reduce tile spec recalculation. - Remove completion callback and instead maintain query state in TileQueryHelper Bug: 75415415 Test: manually Change-Id: I2a20bd916dae0ee9f4c422ec4e2ac3f2d47da3dc --- .../systemui/qs/customize/QSCustomizer.java | 21 +---- .../systemui/qs/customize/TileQueryHelper.java | 99 ++++++++++++---------- .../systemui/qs/external/TileServiceManager.java | 1 - .../systemui/qs/customize/TileQueryHelperTest.java | 88 +++++++++++-------- 4 files changed, 110 insertions(+), 99 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/qs/customize/QSCustomizer.java b/packages/SystemUI/src/com/android/systemui/qs/customize/QSCustomizer.java index 3847040271e2..a3d6c6cff283 100644 --- a/packages/SystemUI/src/com/android/systemui/qs/customize/QSCustomizer.java +++ b/packages/SystemUI/src/com/android/systemui/qs/customize/QSCustomizer.java @@ -18,14 +18,9 @@ package com.android.systemui.qs.customize; import android.animation.Animator; import android.animation.Animator.AnimatorListener; import android.animation.AnimatorListenerAdapter; -import android.app.AlertDialog; import android.content.Context; import android.content.res.Configuration; -import android.graphics.drawable.Drawable; -import android.graphics.drawable.TransitionDrawable; import android.os.Bundle; -import android.os.Handler; -import android.os.Looper; import android.support.v7.widget.DefaultItemAnimator; import android.support.v7.widget.GridLayoutManager; import android.support.v7.widget.RecyclerView; @@ -36,8 +31,6 @@ import android.view.LayoutInflater; import android.view.Menu; import android.view.MenuItem; import android.view.View; -import android.view.WindowManager; -import android.view.WindowManager.LayoutParams; import android.widget.LinearLayout; import android.widget.Toolbar; import android.widget.Toolbar.OnMenuItemClickListener; @@ -48,12 +41,10 @@ import com.android.systemui.Dependency; import com.android.systemui.R; import com.android.systemui.plugins.qs.QS; import com.android.systemui.plugins.qs.QSTile; -import com.android.systemui.qs.QSContainerImpl; import com.android.systemui.qs.QSDetailClipper; import com.android.systemui.qs.QSTileHost; import com.android.systemui.statusbar.phone.LightBarController; import com.android.systemui.statusbar.phone.NotificationsQuickSettingsContainer; -import com.android.systemui.statusbar.phone.SystemUIDialog; import com.android.systemui.statusbar.policy.KeyguardMonitor; import com.android.systemui.statusbar.policy.KeyguardMonitor.Callback; @@ -73,6 +64,7 @@ public class QSCustomizer extends LinearLayout implements OnMenuItemClickListene private final QSDetailClipper mClipper; private final LightBarController mLightBarController; + private final TileQueryHelper mTileQueryHelper; private boolean isShown; private QSTileHost mHost; @@ -82,7 +74,6 @@ public class QSCustomizer extends LinearLayout implements OnMenuItemClickListene private boolean mCustomizing; private NotificationsQuickSettingsContainer mNotifQsContainer; private QS mQs; - private boolean mFinishedFetchingTiles = false; private int mX; private int mY; private boolean mOpening; @@ -112,6 +103,7 @@ public class QSCustomizer extends LinearLayout implements OnMenuItemClickListene mRecyclerView = findViewById(android.R.id.list); mTileAdapter = new TileAdapter(getContext()); + mTileQueryHelper = new TileQueryHelper(context, mTileAdapter); mRecyclerView.setAdapter(mTileAdapter); mTileAdapter.getItemTouchHelper().attachToRecyclerView(mRecyclerView); GridLayoutManager layout = new GridLayoutManager(getContext(), 3); @@ -193,12 +185,7 @@ public class QSCustomizer extends LinearLayout implements OnMenuItemClickListene } private void queryTiles() { - mFinishedFetchingTiles = false; - Runnable tileQueryFetchCompletion = () -> { - Handler mainHandler = new Handler(Looper.getMainLooper()); - mainHandler.post(() -> mFinishedFetchingTiles = true); - }; - new TileQueryHelper(mContext, mHost, mTileAdapter, tileQueryFetchCompletion); + mTileQueryHelper.queryTiles(mHost); } public void hide(int x, int y) { @@ -259,7 +246,7 @@ public class QSCustomizer extends LinearLayout implements OnMenuItemClickListene } private void save() { - if (mFinishedFetchingTiles) { + if (mTileQueryHelper.isFinished()) { mTileAdapter.saveSpecs(mHost); } } diff --git a/packages/SystemUI/src/com/android/systemui/qs/customize/TileQueryHelper.java b/packages/SystemUI/src/com/android/systemui/qs/customize/TileQueryHelper.java index 8bf40962c911..9593b0fb9e9b 100644 --- a/packages/SystemUI/src/com/android/systemui/qs/customize/TileQueryHelper.java +++ b/packages/SystemUI/src/com/android/systemui/qs/customize/TileQueryHelper.java @@ -25,60 +25,63 @@ import android.content.pm.PackageManager; import android.content.pm.ResolveInfo; import android.graphics.drawable.Drawable; import android.os.Handler; -import android.os.Looper; import android.service.quicksettings.TileService; import android.text.TextUtils; +import android.util.ArraySet; import android.widget.Button; import com.android.systemui.Dependency; import com.android.systemui.R; import com.android.systemui.plugins.qs.QSTile; import com.android.systemui.plugins.qs.QSTile.State; -import com.android.systemui.qs.tileimpl.QSTileImpl.DrawableIcon; -import com.android.systemui.qs.external.CustomTile; import com.android.systemui.qs.QSTileHost; +import com.android.systemui.qs.external.CustomTile; +import com.android.systemui.qs.tileimpl.QSTileImpl.DrawableIcon; import java.util.ArrayList; import java.util.Collection; import java.util.List; public class TileQueryHelper { - private static final String TAG = "TileQueryHelper"; private final ArrayList mTiles = new ArrayList<>(); - private final ArrayList mSpecs = new ArrayList<>(); + private final ArraySet mSpecs = new ArraySet<>(); + private final Handler mBgHandler; + private final Handler mMainHandler; private final Context mContext; private final TileStateListener mListener; - private final QSTileHost mHost; - private final Runnable mCompletion; - public TileQueryHelper(Context context, QSTileHost host, - TileStateListener listener, Runnable completion) { + private boolean mFinished; + + public TileQueryHelper(Context context, TileStateListener listener) { mContext = context; mListener = listener; - mHost = host; - mCompletion = completion; - addSystemTiles(); - // TODO: Live? + mBgHandler = new Handler(Dependency.get(Dependency.BG_LOOPER)); + mMainHandler = Dependency.get(Dependency.MAIN_HANDLER); } - private void addSystemTiles() { + public void queryTiles(QSTileHost host) { + mTiles.clear(); + mSpecs.clear(); + mFinished = false; // Enqueue jobs to fetch every system tile and then ever package tile. - final Handler qsHandler = new Handler((Looper) Dependency.get(Dependency.BG_LOOPER)); - final Handler mainHandler = new Handler(Looper.getMainLooper()); - addStockTiles(mainHandler, qsHandler); - addPackageTiles(mainHandler, qsHandler); - // Then enqueue the completion. It should always be last - qsHandler.post(mCompletion); + addStockTiles(host); + addPackageTiles(host); + // TODO: Live? + } + + public boolean isFinished() { + return mFinished; } - private void addStockTiles(Handler mainHandler, Handler bgHandler) { + private void addStockTiles(QSTileHost host) { String possible = mContext.getString(R.string.quick_settings_tiles_stock); String[] possibleTiles = possible.split(","); + final ArrayList tilesToAdd = new ArrayList<>(); for (int i = 0; i < possibleTiles.length; i++) { final String spec = possibleTiles[i]; - final QSTile tile = mHost.createTile(spec); + final QSTile tile = host.createTile(spec); if (tile == null) { continue; } else if (!tile.isAvailable()) { @@ -89,28 +92,25 @@ public class TileQueryHelper { tile.clearState(); tile.refreshState(); tile.setListening(this, false); - bgHandler.post(new Runnable() { - @Override - public void run() { - final QSTile.State state = tile.getState().copy(); - // Ignore the current state and get the generic label instead. - state.label = tile.getTileLabel(); - tile.destroy(); - mainHandler.post(new Runnable() { - @Override - public void run() { - addTile(spec, null, state, true); - mListener.onTilesChanged(mTiles); - } - }); - } - }); + tile.setTileSpec(spec); + tilesToAdd.add(tile); } + + mBgHandler.post(() -> { + for (QSTile tile : tilesToAdd) { + final QSTile.State state = tile.getState().copy(); + // Ignore the current state and get the generic label instead. + state.label = tile.getTileLabel(); + tile.destroy(); + addTile(tile.getTileSpec(), null, state, true); + } + notifyTilesChanged(false); + }); } - private void addPackageTiles(Handler mainHandler, Handler bgHandler) { - bgHandler.post(() -> { - Collection params = mHost.getTiles(); + private void addPackageTiles(final QSTileHost host) { + mBgHandler.post(() -> { + Collection params = host.getTiles(); PackageManager pm = mContext.getPackageManager(); List services = pm.queryIntentServicesAsUser( new Intent(TileService.ACTION_QS_TILE), 0, ActivityManager.getCurrentUser()); @@ -145,9 +145,18 @@ public class TileQueryHelper { icon.mutate(); icon.setTint(mContext.getColor(android.R.color.white)); CharSequence label = info.serviceInfo.loadLabel(pm); - addTile(spec, icon, label != null ? label.toString() : "null", appLabel, mContext); + addTile(spec, icon, label != null ? label.toString() : "null", appLabel); } - mainHandler.post(() -> mListener.onTilesChanged(mTiles)); + + notifyTilesChanged(true); + }); + } + + private void notifyTilesChanged(final boolean finished) { + final ArrayList tilesToReturn = new ArrayList<>(mTiles); + mMainHandler.post(() -> { + mListener.onTilesChanged(tilesToReturn); + mFinished = finished; }); } @@ -177,8 +186,8 @@ public class TileQueryHelper { mSpecs.add(spec); } - private void addTile(String spec, Drawable drawable, CharSequence label, CharSequence appLabel, - Context context) { + private void addTile( + String spec, Drawable drawable, CharSequence label, CharSequence appLabel) { QSTile.State state = new QSTile.State(); state.label = label; state.contentDescription = label; diff --git a/packages/SystemUI/src/com/android/systemui/qs/external/TileServiceManager.java b/packages/SystemUI/src/com/android/systemui/qs/external/TileServiceManager.java index 7ad5a594565b..f5f8ffa7f641 100644 --- a/packages/SystemUI/src/com/android/systemui/qs/external/TileServiceManager.java +++ b/packages/SystemUI/src/com/android/systemui/qs/external/TileServiceManager.java @@ -33,7 +33,6 @@ import android.service.quicksettings.TileService; import android.support.annotation.VisibleForTesting; import android.util.Log; -import com.android.systemui.qs.customize.TileQueryHelper.TileStateListener; import com.android.systemui.qs.external.TileLifecycleManager.TileChangeListener; import java.util.List; diff --git a/packages/SystemUI/tests/src/com/android/systemui/qs/customize/TileQueryHelperTest.java b/packages/SystemUI/tests/src/com/android/systemui/qs/customize/TileQueryHelperTest.java index 5ae107a6a431..f63d2360d976 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/qs/customize/TileQueryHelperTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/qs/customize/TileQueryHelperTest.java @@ -15,71 +15,87 @@ package com.android.systemui.qs.customize; import static junit.framework.Assert.assertEquals; +import static junit.framework.Assert.assertTrue; + +import static org.junit.Assert.assertFalse; import static org.mockito.Mockito.any; -import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; -import com.android.systemui.Dependency; +import android.support.test.filters.SmallTest; import android.testing.AndroidTestingRunner; -import com.android.systemui.SysuiTestCase; -import com.android.systemui.plugins.qs.QSTile; -import com.android.systemui.plugins.qs.QSTile.State; -import com.android.systemui.qs.QSTileHost; import android.testing.TestableLooper; import android.testing.TestableLooper.RunWithLooper; +import com.android.systemui.Dependency; +import com.android.systemui.SysuiTestCase; +import com.android.systemui.qs.QSTileHost; + import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; -import android.os.Message; -import android.test.suitebuilder.annotation.SmallTest; +import java.util.ArrayList; @SmallTest @RunWith(AndroidTestingRunner.class) @RunWithLooper public class TileQueryHelperTest extends SysuiTestCase { + @Mock private TileQueryHelper.TileStateListener mListener; + @Mock private QSTileHost mQSTileHost; + private TestableLooper mBGLooper; - private Runnable mLastCallback; + + private TileQueryHelper mTileQueryHelper; @Before public void setup() { + MockitoAnnotations.initMocks(this); mBGLooper = TestableLooper.get(this); mDependency.injectTestDependency(Dependency.BG_LOOPER, mBGLooper.getLooper()); + mTileQueryHelper = new TileQueryHelper(mContext, mListener); + } + + @Test + public void testIsFinished_falseBeforeQuerying() { + assertFalse(mTileQueryHelper.isFinished()); + } + + @Test + public void testIsFinished_trueAfterQuerying() { + mTileQueryHelper.queryTiles(mQSTileHost); + + mBGLooper.processAllMessages(); + waitForIdleSync(Dependency.get(Dependency.MAIN_HANDLER)); + + assertTrue(mTileQueryHelper.isFinished()); } @Test - public void testCompletionCalled() { - QSTileHost mockHost = mock(QSTileHost.class); - TileAdapter mockAdapter = mock(TileAdapter.class); - Runnable mockCompletion = mock(Runnable.class); - new TileQueryHelper(mContext, mockHost, mockAdapter, mockCompletion); + public void testQueryTiles_callsListenerTwice() { + mTileQueryHelper.queryTiles(mQSTileHost); + mBGLooper.processAllMessages(); - verify(mockCompletion).run(); + waitForIdleSync(Dependency.get(Dependency.MAIN_HANDLER)); + + verify(mListener, times(2)).onTilesChanged(any()); } @Test - public void testCompletionCalledAfterTilesFetched() { - QSTile mockTile = mock(QSTile.class); - State mockState = mock(State.class); - when(mockState.copy()).thenReturn(mockState); - when(mockTile.getState()).thenReturn(mockState); - when(mockTile.isAvailable()).thenReturn(true); - - QSTileHost mockHost = mock(QSTileHost.class); - when(mockHost.createTile(any())).thenReturn(mockTile); - - mBGLooper.setMessageHandler((Message m) -> { - mLastCallback = m.getCallback(); - return true; - }); - TileAdapter mockAdapter = mock(TileAdapter.class); - Runnable mockCompletion = mock(Runnable.class); - new TileQueryHelper(mContext, mockHost, mockAdapter, mockCompletion); - - // Verify that the last thing in the queue was our callback + public void testQueryTiles_isFinishedFalseOnListenerCalls_thenTrueAfterCompletion() { + doAnswer(invocation -> { + assertFalse(mTileQueryHelper.isFinished()); + return null; + }).when(mListener).onTilesChanged(any()); + + mTileQueryHelper.queryTiles(mQSTileHost); + mBGLooper.processAllMessages(); - assertEquals(mockCompletion, mLastCallback); + waitForIdleSync(Dependency.get(Dependency.MAIN_HANDLER)); + + assertTrue(mTileQueryHelper.isFinished()); } } -- 2.11.0