-
-
Notifications
You must be signed in to change notification settings - Fork 489
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
* Add basic mocks to unit test PlaybackController Adds the various basic mocks required to unit test Playback Controller. At a high level the steps we perform are: - Prep mocks for deps injected by constructor - Prep mocks for Koin, then pass these as singletons - Patch up various calls out to Android SharedPrefs ...etc. This allows us to construct a PlaybackController in a JVM context now. These mechanisms will be useful for later in the patch series too.... * Add setPlayback impl and tests to controller/view Adds setPlaybackSpeed to the view and controller. The controller has associated tests which built on the earlier ones. Unfortunately the view also has logic in it and I've tried to (ab)use mocks to factor out the ExoPlayer factory. However because it's a static final the mock will always defer to the real impl. I could introduce some sort of interface and abstract out the calls to the underlying players using an adapter pattern so we can test this. But alas, the player rewrite will obselete this and I'm 8+ hours into getting tests to work... * Add VideoSpeedController and tests Adds a new controller for VideoSpeed, to help encapsulate items following SRP. This also adds a mechanism to track the users most recent speed selection, so switching from videos / to other media and back will restore the user's selection on the next video. * Switch to using Enums to representing speed This makes it much easier to compare for equality and has built in indexing making the menu much easier to work with. In comparison using doubles directly would have required us to add a comparison lambda in the view to correctly detect what was selected / determine what is selected. * Wire up view for Playback speed controls Wires up the view component for playback speed controls, based on the pattern used in various other action items. * Preserve playback speed between videos - Update the PlaybackController to internally track the last playback speed, but still treat it as a black box from all external callers - Add a test we don't throw a NPE trying to set it before init - There's no test for checking this value gets forwarded (see below) - Change the activity to create the speed controller whilst the ControlGlue is being created. This is unfortunately a bit of a hack. There isn't a signal for onPlaybackStarted or similar we can attach to from the playback controller so we have to mess with the internals. Something I've tried to avoid greatly. I've tried adding a unit test with mocks, which was surprisingly going well. Until we create LibVLC objects mid-way. These needs some sort of factory pattern to inject mocks else it will query Android for playback capabilities failing the test. I'm going to leave this particular test as a manual case as I extracting factories out with the pending rewrite is a futile effort. * Fix various nits / warnings from CI This includes: - Using magic values in our enum - Impliclt Locale issues - Nullability checks * Port drawable from Exoplayer into drawables Ports the playback speed drawable into the projects list of drawable items, following PR feedback. The license is preserved (Apache 2.0) and compatible with the GPL license of this project. * Apply various code improvements to PlaybackSpeedAction Includes the following from the PR review: - Whitespace tidy up - Implicitly return from lambda instead of explicitly naming it - Use apply to remove a significant amount of boilerplate * Add check that setPlaybackSpeed is sane This will ignore any values that are clearly incorrect i.e. <0.25. This means the playback controller can still have a sentinel value of -1.0 to ensure the model controlling speed is in-place correctly. Whilst not breaking user playback by default if something goes wrong. As this file directly interfaces with the video manager (which holds surfaces) we can't easily make a test for it, so leave this block untested * DI interface instead of concrete type Inject the interface for shared preferences rather than the concrete type. This avoids Mockk trying to grab the abstract class underneath, which would run the real impl instead. It also brings our DI in this file more in-line with the commonly used pattern * Address PR comments - Revert some noisy automatic formatting - Move speed steps out of companion object - Drop supuerfluous interface * Tidy up VideoSpeedController from PR review - Use Kotlin a kotlin setter directly - Make the variable public - (Small amount of rewiring to make everything work) * Drop PlaybackController tests and revert DI Drop PlaybackController test. We can't simply inject by interface as this isn't type safe with how it's currently implemented. Since it's likely PlaybackController is going to undergo a major rework with pending future work, let's simply drop the tests and revert the associated changes.
- Loading branch information
Showing
8 changed files
with
284 additions
and
14 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
40 changes: 40 additions & 0 deletions
40
app/src/main/java/org/jellyfin/androidtv/ui/playback/VideoSpeedController.kt
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| package org.jellyfin.androidtv.ui.playback | ||
|
|
||
| class VideoSpeedController( | ||
| private val parentController: PlaybackController | ||
| ) { | ||
| enum class SpeedSteps(val speed: Double) { | ||
| // Use named parameter so detekt knows these aren't magic values | ||
| SPEED_0_25(speed = 0.25), | ||
| SPEED_0_50(speed = 0.5), | ||
| SPEED_0_75(speed = 0.75), | ||
| SPEED_1_00(speed = 1.0), | ||
| SPEED_1_25(speed = 1.25), | ||
| SPEED_1_50(speed = 1.50), | ||
| SPEED_1_75(speed = 1.75), | ||
| SPEED_2_00(speed = 2.0), | ||
| } | ||
|
|
||
| companion object { | ||
| // Preserve the currently selected speed during the app lifetime, even if | ||
| // video playback closes | ||
| private var previousSpeedSelection = SpeedSteps.SPEED_1_00 | ||
| } | ||
|
|
||
| var currentSpeed = previousSpeedSelection | ||
| set(value) { | ||
| parentController.setPlaybackSpeed(value.speed) | ||
| previousSpeedSelection = value | ||
| field = value | ||
| } | ||
|
|
||
| init { | ||
| // We need to do this again in init, as Kotlin will not call the custom | ||
| // setter on initialization, so the PlaybackController is not informed | ||
| currentSpeed = previousSpeedSelection | ||
| } | ||
|
|
||
| fun resetSpeedToDefault() { | ||
| currentSpeed = SpeedSteps.SPEED_1_00 | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
58 changes: 58 additions & 0 deletions
58
app/src/main/java/org/jellyfin/androidtv/ui/playback/overlay/action/PlaybackSpeedAction.kt
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| package org.jellyfin.androidtv.ui.playback.overlay.action | ||
|
|
||
| import android.content.Context | ||
| import android.view.Gravity | ||
| import android.view.View | ||
| import android.widget.PopupMenu | ||
| import org.jellyfin.androidtv.R | ||
| import org.jellyfin.androidtv.ui.playback.PlaybackController | ||
| import org.jellyfin.androidtv.ui.playback.VideoSpeedController | ||
| import org.jellyfin.androidtv.ui.playback.overlay.CustomPlaybackTransportControlGlue | ||
| import org.jellyfin.androidtv.ui.playback.overlay.LeanbackOverlayFragment | ||
| import java.util.* | ||
|
|
||
| class PlaybackSpeedAction( | ||
| context: Context, | ||
| customPlaybackTransportControlGlue: CustomPlaybackTransportControlGlue, | ||
| playbackController: PlaybackController | ||
| ) : CustomAction(context, customPlaybackTransportControlGlue) { | ||
| private val speedController = VideoSpeedController(playbackController) | ||
| private val speeds = VideoSpeedController.SpeedSteps.values() | ||
|
|
||
| init { | ||
| initializeWithIcon(R.drawable.ic_playback_speed) | ||
| } | ||
|
|
||
| override fun handleClickAction( | ||
| playbackController: PlaybackController, | ||
| leanbackOverlayFragment: LeanbackOverlayFragment, | ||
| context: Context, view: View | ||
| ) { | ||
| val speedMenu = populateMenu(context, view, speedController) | ||
|
|
||
| speedMenu.setOnDismissListener { leanbackOverlayFragment.setFading(true) } | ||
|
|
||
| speedMenu.setOnMenuItemClickListener { menuItem -> | ||
| speedController.currentSpeed = speeds[menuItem.itemId] | ||
| speedMenu.dismiss() | ||
| true | ||
| } | ||
|
|
||
| speedMenu.show() | ||
| } | ||
|
|
||
| private fun populateMenu( | ||
| context: Context, | ||
| view: View, | ||
| speedController: VideoSpeedController | ||
| ) = PopupMenu(context, view, Gravity.END).apply { | ||
| speeds.forEachIndexed { i, selected -> | ||
| // Since this is purely numeric data, coerce to en_us to keep the linter happy | ||
| menu.add(0, i, i, String.format(Locale.US, "%.2fx", selected.speed)) | ||
| } | ||
|
|
||
| menu.setGroupCheckable(0, true, true) | ||
| menu.getItem(speeds.indexOf(speedController.currentSpeed)).isChecked = true | ||
| } | ||
|
|
||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| <?xml version="1.0" encoding="utf-8"?> | ||
| <!-- | ||
| Copyright 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. | ||
| --> | ||
| <vector xmlns:android="http://schemas.android.com/apk/res/android" | ||
| android:width="24dp" | ||
| android:height="24dp" | ||
| android:viewportWidth="24" | ||
| android:viewportHeight="24"> | ||
| <path | ||
| android:pathData="M13.05,9.79L10,7.5v9l3.05,-2.29L16,12zM13.05,9.79L10,7.5v9l3.05,-2.29L16,12zM13.05,9.79L10,7.5v9l3.05,-2.29L16,12zM11,4.07L11,2.05c-2.01,0.2 -3.84,1 -5.32,2.21L7.1,5.69c1.11,-0.86 2.44,-1.44 3.9,-1.62zM5.69,7.1L4.26,5.68C3.05,7.16 2.25,8.99 2.05,11h2.02c0.18,-1.46 0.76,-2.79 1.62,-3.9zM4.07,13L2.05,13c0.2,2.01 1,3.84 2.21,5.32l1.43,-1.43c-0.86,-1.1 -1.44,-2.43 -1.62,-3.89zM5.68,19.74C7.16,20.95 9,21.75 11,21.95v-2.02c-1.46,-0.18 -2.79,-0.76 -3.9,-1.62l-1.42,1.43zM22,12c0,5.16 -3.92,9.42 -8.95,9.95v-2.02C16.97,19.41 20,16.05 20,12s-3.03,-7.41 -6.95,-7.93L13.05,2.05C18.08,2.58 22,6.84 22,12z" | ||
| android:fillColor="#FFFFFF"/> | ||
| </vector> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.