fix(android): resolve MediaPlayer lifecycle bugs and remove deprecated Android style usage#80
Open
MattyB95 wants to merge 2 commits intomoust:masterfrom
Open
fix(android): resolve MediaPlayer lifecycle bugs and remove deprecated Android style usage#80MattyB95 wants to merge 2 commits intomoust:masterfrom
MattyB95 wants to merge 2 commits intomoust:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the Android MediaPlayer lifecycle in the Cordova VideoPlayer plugin (preventing double-callbacks/leaks/crashes), removes deprecated fullscreen handling, and adds an Android Gradle-based unit test harness with regression coverage.
Changes:
- Fix Android playback lifecycle behavior (async prepare, error handling, safe release/dismiss, asset FD closure, API-safe fullscreen).
- Add Android unit tests and a standalone Gradle test project under
tests/android. - Update docs/metadata (Android-only support, permissions guidance, npm/just test commands, cleanup
.gitignore).
Reviewed changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/android/VideoPlayer.java |
Lifecycle fixes: async prepare, safer close/error/completion cleanup, API-safe fullscreen via reflection, shared sendError helper. |
www/videoplayer.js |
Replace custom merge with Object.assign-based options merge. |
tests/android/build.gradle |
Introduces Android Gradle test module compiling plugin source and running JUnit/Mockito tests. |
tests/android/gradlew |
Adds Gradle wrapper script for test project. |
tests/android/gradle/wrapper/* |
Adds wrapper jar/properties for pinned Gradle distribution. |
tests/android/src/test/java/* |
Adds regression tests for stripFileProtocol, applyFullscreen, and onError behavior. |
tests/android/src/test/resources/...MockMaker |
Configures Mockito mock maker for unit tests. |
tests/android/src/main/AndroidManifest.xml |
Minimal manifest for the test module. |
tests/android/settings.gradle |
Settings for the test Gradle project. |
package.json |
Updates metadata (Android-only) and adds npm test to run Android unit tests. |
README.md |
Updates docs (Android-only, permissions, troubleshooting). |
.gitignore |
Replaces legacy ignores with common OS/editor/Android/Node ignores. |
justfile |
Adds convenience tasks for running tests/lint/report/validation. |
Comments suppressed due to low confidence (1)
src/android/VideoPlayer.java:278
prepareAsync()makesonPrepared()asynchronous relative toclose()/surfaceDestroyed()which can release/null out the player. As written,mp.start()can throwIllegalStateExceptionif the player was released before the callback fires. Add a guard (e.g., return ifplayer == nullormp != player) and/or wrapstart()in a try/catch to avoid lifecycle race crashes.
@Override
public void onPrepared(MediaPlayer mp) {
mp.start();
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
66b4c5e to
782b3ff
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes several Android
MediaPlayerlifecycle issues that could cause crashes, duplicate callbacks, silent failures, and resource leaks. It also removes deprecated style usage and adds regression tests for the corrected error-handling paths.Changes
Theme_NoTitleBarwithTheme_Black_NoTitleBar_Fullscreenprepare()withprepareAsync()onError()returningfalse, which triggeredonCompletion()and caused doublerelease()/dismiss()onError()playeranddialogafter release to prevent duplicate callbacks and potential NPEsAssetFileDescriptorafter asset playbackreturnDefaultValues = trueto support Android framework stubbed paths in unit testsVideoPlayerOnErrorTestregression coverage for the fixedonError()behaviourFixes #71 #73 #75