[MUON-2017] Simplify BpkVideoPlayer by removing VideoHandler seam - #2757
Conversation
Generated by 🚫 Danger Kotlin against 3a178bc |
Antoni Castejón (antonicg)
added
the
minor
VideoPlayerHandle/PlayerFactory/FakeVideoPlayerHandle were only added to unit-test BpkVideoPlayerController without a real ExoPlayer. The controller now owns an ExoPlayer directly, and play/pause/loop/mute/ended/dispose coverage moved to instrumentation tests driving a real bundled local video instead of a fake. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
3a178bc to
2541a80
Compare
Generated by 🚫 Danger Kotlin against 2541a80 |
Generated by 🚫 Danger Kotlin against 18e1979 |
Generated by 🚫 Danger Kotlin against e4ca132 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR simplifies BpkVideoPlayerController by removing the VideoPlayerHandle/PlayerFactory seam and having the controller build and own an ExoPlayer directly, while migrating controller behavior tests from JVM fakes to end-to-end instrumentation tests using a bundled video resource.
Changes:
- Remove
VideoPlayerHandle/PlayerFactoryabstractions and the fake-based JVM unit tests. - Build and manage
ExoPlayerdirectly insideBpkVideoPlayerController. - Add/extend instrumentation tests (including default controls) and use a bundled
android.resource://test video.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| backpack-compose/src/main/kotlin/net/skyscanner/backpack/compose/videoplayer/BpkVideoPlayerController.kt | Removes the player seam and constructs/owns an ExoPlayer directly. |
| backpack-compose/src/main/kotlin/net/skyscanner/backpack/compose/videoplayer/internal/VideoPlayerHandle.kt | Deletes the internal handle abstraction. |
| backpack-compose/src/main/kotlin/net/skyscanner/backpack/compose/videoplayer/internal/PlayerFactory.kt | Deletes the factory previously responsible for building the handle/player. |
| backpack-compose/src/test/kotlin/net/skyscanner/backpack/compose/videoplayer/FakeVideoPlayerHandle.kt | Removes the JVM fake used to drive controller unit tests. |
| backpack-compose/src/test/kotlin/net/skyscanner/backpack/compose/videoplayer/BpkVideoPlayerControllerTest.kt | Removes fake-based controller unit tests (superseded by androidTests). |
| backpack-compose/src/androidTest/kotlin/net/skyscanner/backpack/compose/videoplayer/BpkVideoPlayerTest.kt | Adds end-to-end instrumentation coverage for playback + controller behaviors. |
| backpack-compose/src/androidTest/kotlin/net/skyscanner/backpack/compose/videoplayer/BpkVideoPlayerDefaultControlsTest.kt | Adds instrumentation coverage for default play/pause controls behavior. |
| private var originalAnimationScales: Pair<String?, String?>? = null | ||
|
|
||
| private fun disableReducedMotionSignal() { | ||
| originalAnimationScales = | ||
| getGlobalSetting(ANIMATOR_DURATION_SCALE_KEY) to getGlobalSetting(TRANSITION_ANIMATION_SCALE_KEY) | ||
| putGlobalSetting(ANIMATOR_DURATION_SCALE_KEY, "1") | ||
| putGlobalSetting(TRANSITION_ANIMATION_SCALE_KEY, "1") | ||
| } |
There was a problem hiding this comment.
| private fun getGlobalSetting(key: String): String? = | ||
| runShellCommand("settings get global $key") | ||
| .trim() | ||
| .takeUnless { it == "null" || it.isEmpty() } | ||
|
|
||
| private fun putGlobalSetting(key: String, value: String) { | ||
| runShellCommand("settings put global $key $value") | ||
| } | ||
|
|
||
| private fun runShellCommand(command: String): String = | ||
| AutoCloseInputStream(InstrumentationRegistry.getInstrumentation().uiAutomation.executeShellCommand(command)) | ||
| .use { it.readBytes().decodeToString() } |
There was a problem hiding this comment.
| fun givenPlayerComposed_whenRemovedFromComposition_thenUnderlyingPlayerIsReleased() { | ||
| // Given | ||
| lateinit var controller: BpkVideoPlayerController | ||
| var showPlayer by mutableStateOf(true) | ||
| composeTestRule.setContent { | ||
| BpkTheme { | ||
| if (showPlayer) { | ||
| controller = rememberBpkVideoPlayerController(playableConfig()) | ||
| BpkVideoPlayer(controller = controller) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // When | ||
| composeTestRule.runOnIdle { showPlayer = false } | ||
| composeTestRule.waitForIdle() | ||
|
|
||
| // Then | ||
| assertEquals(Player.STATE_IDLE, controller.player.playbackState) | ||
| } |
There was a problem hiding this comment.
- Extract shared reduced-motion helpers and shell commands into VideoPlayerTestRule (ExternalResource) to eliminate duplication between BpkVideoPlayerTest and BpkVideoPlayerDefaultControlsTest - Fix dispose test to wait for Playing state before removing the player from composition, making the STATE_IDLE assertion unambiguously prove release() was called Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Generated by 🚫 Danger Kotlin against 5db0de0 |
MUON-2017
Summary
VideoPlayerHandle/PlayerFactory/FakeVideoPlayerHandlewere added in a prior PR ([Android] BpkVideoPlayer: controller testability (MUON-1847) #2749) purely to letBpkVideoPlayerControllerbe unit-tested without a realExoPlayer. Removes that seam — the controller now builds and owns anExoPlayerdirectly.backpack-compose/src/androidTest/res/raw/bpk_video_player_test.mp4, played viaandroid.resource://.internalmembers (internal val player, theinternal constructor), sorememberBpkVideoPlayerController,play(),pause(),toggle(),setMuted(),resetToStart(),playbackState,isMuted,configall keep their exact signatures.Test plan
./gradlew :backpack-compose:testDebugUnitTest— reducer + playback-state unit tests still pass, controller test class removed (superseded by instrumentation tests)./gradlew :backpack-compose:connectedDebugAndroidTest—BpkVideoPlayerTestandBpkVideoPlayerDefaultControlsTestinstrumentation suites pass on device/emulator🤖 Generated with Claude Code