From 7cec5425a2e01e03bd6fe3941be48587934b7931 Mon Sep 17 00:00:00 2001 From: Robert Snoeberger Date: Fri, 29 May 2020 17:09:14 -0400 Subject: [PATCH] Use RepeatableExecutor to poll playback position This helps prevents errors where multiple polling chains are accidentally started. Fixes: 154352658 Test: atest tests/src/com/android/systemui/media/SeekBarViewModelTest.kt Change-Id: I25c67aaa097dbf95ac94a2f3959be30fdebb3eb4 --- .../systemui/media/MediaControlPanel.java | 12 ++-- .../systemui/media/MediaViewManager.kt | 12 ++-- .../systemui/media/SeekBarViewModel.kt | 56 +++++++++++-------- .../systemui/media/MediaControlPanelTest.kt | 16 ++++-- .../systemui/media/SeekBarViewModelTest.kt | 3 +- .../concurrency/FakeRepeatableExecutor.java | 34 +++++++++++ 6 files changed, 92 insertions(+), 41 deletions(-) create mode 100644 packages/SystemUI/tests/src/com/android/systemui/util/concurrency/FakeRepeatableExecutor.java diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaControlPanel.java b/packages/SystemUI/src/com/android/systemui/media/MediaControlPanel.java index b8c1842a17808..f039fc2d84ae1 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaControlPanel.java +++ b/packages/SystemUI/src/com/android/systemui/media/MediaControlPanel.java @@ -49,15 +49,17 @@ import com.android.settingslib.Utils; import com.android.settingslib.media.MediaOutputSliceConstants; import com.android.settingslib.widget.AdaptiveIcon; import com.android.systemui.R; +import com.android.systemui.dagger.qualifiers.Background; import com.android.systemui.plugins.ActivityStarter; import com.android.systemui.util.animation.TransitionLayout; -import com.android.systemui.util.concurrency.DelayableExecutor; import org.jetbrains.annotations.NotNull; import java.util.List; import java.util.concurrent.Executor; +import javax.inject.Inject; + /** * A view controller used for Media Playback. */ @@ -93,12 +95,14 @@ public class MediaControlPanel { * @param backgroundExecutor background executor, used for processing artwork * @param activityStarter activity starter */ - public MediaControlPanel(Context context, DelayableExecutor backgroundExecutor, - ActivityStarter activityStarter, MediaHostStatesManager mediaHostStatesManager) { + @Inject + public MediaControlPanel(Context context, @Background Executor backgroundExecutor, + ActivityStarter activityStarter, MediaHostStatesManager mediaHostStatesManager, + SeekBarViewModel seekBarViewModel) { mContext = context; mBackgroundExecutor = backgroundExecutor; mActivityStarter = activityStarter; - mSeekBarViewModel = new SeekBarViewModel(backgroundExecutor); + mSeekBarViewModel = seekBarViewModel; mMediaViewController = new MediaViewController(context, mediaHostStatesManager); loadDimens(); } diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaViewManager.kt b/packages/SystemUI/src/com/android/systemui/media/MediaViewManager.kt index 3557b04a57bc0..9b9a6b4b13ab6 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaViewManager.kt +++ b/packages/SystemUI/src/com/android/systemui/media/MediaViewManager.kt @@ -11,14 +11,12 @@ import android.widget.HorizontalScrollView import android.widget.LinearLayout import androidx.core.view.GestureDetectorCompat import com.android.systemui.R -import com.android.systemui.dagger.qualifiers.Background -import com.android.systemui.plugins.ActivityStarter import com.android.systemui.qs.PageIndicator import com.android.systemui.statusbar.notification.VisualStabilityManager import com.android.systemui.util.animation.UniqueObjectHostView import com.android.systemui.util.animation.requiresRemeasuring -import com.android.systemui.util.concurrency.DelayableExecutor import javax.inject.Inject +import javax.inject.Provider import javax.inject.Singleton private const val FLING_SLOP = 1000000 @@ -30,9 +28,8 @@ private const val FLING_SLOP = 1000000 @Singleton class MediaViewManager @Inject constructor( private val context: Context, - @Background private val backgroundExecutor: DelayableExecutor, + private val mediaControlPanelFactory: Provider, private val visualStabilityManager: VisualStabilityManager, - private val activityStarter: ActivityStarter, private val mediaHostStatesManager: MediaHostStatesManager, mediaManager: MediaDataCombineLatest ) { @@ -265,8 +262,7 @@ class MediaViewManager @Inject constructor( } var existingPlayer = mediaPlayers[key] if (existingPlayer == null) { - existingPlayer = MediaControlPanel(context, backgroundExecutor, activityStarter, - mediaHostStatesManager) + existingPlayer = mediaControlPanelFactory.get() existingPlayer.attach(PlayerViewHolder.create(LayoutInflater.from(context), mediaContent)) mediaPlayers[key] = existingPlayer @@ -289,7 +285,7 @@ class MediaViewManager @Inject constructor( needsReordering = true } } - existingPlayer.bind(data) + existingPlayer?.bind(data) updateMediaPaddings() updatePageIndicator() } diff --git a/packages/SystemUI/src/com/android/systemui/media/SeekBarViewModel.kt b/packages/SystemUI/src/com/android/systemui/media/SeekBarViewModel.kt index 75ad06962afe4..efc476d0c86f4 100644 --- a/packages/SystemUI/src/com/android/systemui/media/SeekBarViewModel.kt +++ b/packages/SystemUI/src/com/android/systemui/media/SeekBarViewModel.kt @@ -27,8 +27,10 @@ import androidx.annotation.AnyThread import androidx.annotation.WorkerThread import androidx.lifecycle.MutableLiveData import androidx.lifecycle.LiveData - -import com.android.systemui.util.concurrency.DelayableExecutor +import com.android.systemui.dagger.qualifiers.Background +import com.android.systemui.util.concurrency.RepeatableExecutor +import java.util.concurrent.Executor +import javax.inject.Inject private const val POSITION_UPDATE_INTERVAL_MILLIS = 100L @@ -65,7 +67,7 @@ private fun PlaybackState.computePosition(duration: Long): Long { } /** ViewModel for seek bar in QS media player. */ -class SeekBarViewModel(val bgExecutor: DelayableExecutor) { +class SeekBarViewModel @Inject constructor(@Background private val bgExecutor: RepeatableExecutor) { private var _data = Progress(false, false, null, null) set(value) { @@ -89,10 +91,10 @@ class SeekBarViewModel(val bgExecutor: DelayableExecutor) { private var callback = object : MediaController.Callback() { override fun onPlaybackStateChanged(state: PlaybackState) { playbackState = state - if (shouldPollPlaybackPosition()) { - checkPlaybackPosition() - } else if (PlaybackState.STATE_NONE.equals(playbackState)) { + if (PlaybackState.STATE_NONE.equals(playbackState)) { clearController() + } else { + checkIfPollingNeeded() } } @@ -100,12 +102,14 @@ class SeekBarViewModel(val bgExecutor: DelayableExecutor) { clearController() } } + private var cancel: Runnable? = null /** Listening state (QS open or closed) is used to control polling of progress. */ var listening = true - set(value) { - if (value) { - checkPlaybackPosition() + set(value) = bgExecutor.execute { + if (field != value) { + field = value + checkIfPollingNeeded() } } @@ -137,9 +141,7 @@ class SeekBarViewModel(val bgExecutor: DelayableExecutor) { playbackState?.getState() == PlaybackState.STATE_NONE || (duration != null && duration <= 0)) false else true _data = Progress(enabled, seekAvailable, position, duration) - if (shouldPollPlaybackPosition()) { - checkPlaybackPosition() - } + checkIfPollingNeeded() } /** @@ -151,6 +153,8 @@ class SeekBarViewModel(val bgExecutor: DelayableExecutor) { fun clearController() = bgExecutor.execute { controller = null playbackState = null + cancel?.run() + cancel = null _data = _data.copy(enabled = false) } @@ -158,26 +162,34 @@ class SeekBarViewModel(val bgExecutor: DelayableExecutor) { * Call to clean up any resources. */ @AnyThread - fun onDestroy() { + fun onDestroy() = bgExecutor.execute { controller = null playbackState = null + cancel?.run() + cancel = null } - @AnyThread - private fun checkPlaybackPosition(): Runnable = bgExecutor.executeDelayed({ + @WorkerThread + private fun checkPlaybackPosition() { val duration = _data.duration ?: -1 val currentPosition = playbackState?.computePosition(duration.toLong())?.toInt() if (currentPosition != null && _data.elapsedTime != currentPosition) { _data = _data.copy(elapsedTime = currentPosition) } - if (shouldPollPlaybackPosition()) { - checkPlaybackPosition() - } - }, POSITION_UPDATE_INTERVAL_MILLIS) + } @WorkerThread - private fun shouldPollPlaybackPosition(): Boolean { - return listening && playbackState?.isInMotion() ?: false + private fun checkIfPollingNeeded() { + val needed = listening && playbackState?.isInMotion() ?: false + if (needed) { + if (cancel == null) { + cancel = bgExecutor.executeRepeatedly(this::checkPlaybackPosition, 0L, + POSITION_UPDATE_INTERVAL_MILLIS) + } + } else { + cancel?.run() + cancel = null + } } /** Gets a listener to attach to the seek bar to handle seeking. */ @@ -194,7 +206,7 @@ class SeekBarViewModel(val bgExecutor: DelayableExecutor) { private class SeekBarChangeListener( val viewModel: SeekBarViewModel, - val bgExecutor: DelayableExecutor + val bgExecutor: Executor ) : SeekBar.OnSeekBarChangeListener { override fun onProgressChanged(bar: SeekBar, progress: Int, fromUser: Boolean) { if (fromUser) { diff --git a/packages/SystemUI/tests/src/com/android/systemui/media/MediaControlPanelTest.kt b/packages/SystemUI/tests/src/com/android/systemui/media/MediaControlPanelTest.kt index 1ba36e19b404b..737ced63eed03 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/media/MediaControlPanelTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/media/MediaControlPanelTest.kt @@ -31,6 +31,7 @@ import android.widget.ImageButton import android.widget.ImageView import android.widget.SeekBar import android.widget.TextView +import androidx.lifecycle.LiveData import androidx.test.filters.SmallTest import com.android.systemui.R import com.android.systemui.SysuiTestCase @@ -41,6 +42,7 @@ import com.android.systemui.util.time.FakeSystemClock import com.google.common.truth.Truth.assertThat import org.junit.After import org.junit.Before +import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith import org.mockito.ArgumentCaptor @@ -48,6 +50,7 @@ import org.mockito.Mock import org.mockito.Mockito.mock import org.mockito.Mockito.verify import org.mockito.Mockito.`when` as whenever +import org.mockito.junit.MockitoJUnit private const val KEY = "TEST_KEY" private const val APP = "APP" @@ -73,6 +76,8 @@ public class MediaControlPanelTest : SysuiTestCase() { @Mock private lateinit var holder: PlayerViewHolder @Mock private lateinit var view: TransitionLayout @Mock private lateinit var mediaHostStatesManager: MediaHostStatesManager + @Mock private lateinit var seekBarViewModel: SeekBarViewModel + @Mock private lateinit var seekBarData: LiveData private lateinit var appIcon: ImageView private lateinit var appName: TextView private lateinit var albumView: ImageView @@ -94,18 +99,17 @@ public class MediaControlPanelTest : SysuiTestCase() { private val device = MediaDeviceData(true, null, DEVICE_NAME) private val disabledDevice = MediaDeviceData(false, null, null) + @JvmField @Rule val mockito = MockitoJUnit.rule() + @Before fun setUp() { bgExecutor = FakeExecutor(FakeSystemClock()) - activityStarter = mock(ActivityStarter::class.java) - mediaHostStatesManager = mock(MediaHostStatesManager::class.java) - - player = MediaControlPanel(context, bgExecutor, activityStarter, mediaHostStatesManager) + player = MediaControlPanel(context, bgExecutor, activityStarter, mediaHostStatesManager, + seekBarViewModel) + whenever(seekBarViewModel.progress).thenReturn(seekBarData) // Mock out a view holder for the player to attach to. - holder = mock(PlayerViewHolder::class.java) - view = mock(TransitionLayout::class.java) whenever(holder.player).thenReturn(view) appIcon = ImageView(context) whenever(holder.appIcon).thenReturn(appIcon) diff --git a/packages/SystemUI/tests/src/com/android/systemui/media/SeekBarViewModelTest.kt b/packages/SystemUI/tests/src/com/android/systemui/media/SeekBarViewModelTest.kt index 19e15b3c43072..24e9bd837d5d0 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/media/SeekBarViewModelTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/media/SeekBarViewModelTest.kt @@ -29,6 +29,7 @@ import androidx.test.filters.SmallTest import com.android.systemui.SysuiTestCase import com.android.systemui.util.concurrency.FakeExecutor +import com.android.systemui.util.concurrency.FakeRepeatableExecutor import com.android.systemui.util.time.FakeSystemClock import com.google.common.truth.Truth.assertThat @@ -71,7 +72,7 @@ public class SeekBarViewModelTest : SysuiTestCase() { @Before fun setUp() { fakeExecutor = FakeExecutor(FakeSystemClock()) - viewModel = SeekBarViewModel(fakeExecutor) + viewModel = SeekBarViewModel(FakeRepeatableExecutor(fakeExecutor)) mockController = mock(MediaController::class.java) whenever(mockController.sessionToken).thenReturn(token1) mockTransport = mock(MediaController.TransportControls::class.java) diff --git a/packages/SystemUI/tests/src/com/android/systemui/util/concurrency/FakeRepeatableExecutor.java b/packages/SystemUI/tests/src/com/android/systemui/util/concurrency/FakeRepeatableExecutor.java new file mode 100644 index 0000000000000..477f615faf2b0 --- /dev/null +++ b/packages/SystemUI/tests/src/com/android/systemui/util/concurrency/FakeRepeatableExecutor.java @@ -0,0 +1,34 @@ +/* + * Copyright (C) 2020 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.util.concurrency; + +/** + * A fake to use in tests. + */ +public class FakeRepeatableExecutor extends RepeatableExecutorImpl { + + /** + * Initializes a fake RepeatableExecutor from a fake executor. + * + * Use the fake executor to actually process tasks. + * + * @param executor fake executor. + */ + public FakeRepeatableExecutor(FakeExecutor executor) { + super(executor); + } +}