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
diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaControlPanel.java b/packages/SystemUI/src/com/android/systemui/media/MediaControlPanel.java
index b8c1842..f039fc2 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.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 @@
* @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 3557b04..9b9a6b4 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.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 @@
@Singleton
class MediaViewManager @Inject constructor(
private val context: Context,
- @Background private val backgroundExecutor: DelayableExecutor,
+ private val mediaControlPanelFactory: Provider<MediaControlPanel>,
private val visualStabilityManager: VisualStabilityManager,
- private val activityStarter: ActivityStarter,
private val mediaHostStatesManager: MediaHostStatesManager,
mediaManager: MediaDataCombineLatest
) {
@@ -265,8 +262,7 @@
}
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 @@
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 75ad069..efc476d 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.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 @@
}
/** 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 @@
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 @@
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 @@
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 @@
fun clearController() = bgExecutor.execute {
controller = null
playbackState = null
+ cancel?.run()
+ cancel = null
_data = _data.copy(enabled = false)
}
@@ -158,26 +162,34 @@
* 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 @@
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 1ba36e1..737ced6 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.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.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.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 @@
@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<SeekBarViewModel.Progress>
private lateinit var appIcon: ImageView
private lateinit var appName: TextView
private lateinit var albumView: ImageView
@@ -94,18 +99,17 @@
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 19e15b3..24e9bd8 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 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 @@
@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 0000000..477f615
--- /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);
+ }
+}