-
Notifications
You must be signed in to change notification settings - Fork 521
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #5451 : java.lang.IllegalStateException - Media player has not been previously initialized #5475
Fix #5451 : java.lang.IllegalStateException - Media player has not been previously initialized #5475
Conversation
Develop (fork) Branch Update
Hi @subhajitxyz, could you please ensure that you have added tests for the changes? We want to make sure the issue does not recurr. @theMr17, @Vishwajith-Shettigar, could you please keep an eye out incase @subhajitxyz has questions about testing? |
Sure, @subhajitxyz please reach out if you have any trouble or doubts while writing tests! |
@theMr17 Actually i didn't clearly understand the question of adhiamboperes. |
If mediaplayer is not initialized and releaseMediaPlayer() is called, for this test was added already. |
Yes, you are right! For every change we make, we should add tests to verify that the behavior is handled correctly. Please add a test for the case you mentioned. This will help us verify that the |
Ok , soon I will add required tests. |
Hi @adhiamboperes, I have added a test for the changes. The test assures that consecutive calls of releaseMediaPlayer() do not throw exceptions. |
Please take a look at the test naming convention described here: https://github.com/oppia/oppia-android/wiki/Oppia-Android-Testing#naming-convention. Go ahead with a name which you think should be right per this convention. The name could be verified while reviewing the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @subhajitxyz! Please take a look at the comments. Do you also have explicit reprduction steps for this bug? It would be great if you could add those to the issue body for future reference.
domain/src/test/java/org/oppia/android/domain/audio/AudioPlayerControllerTest.kt
Outdated
Show resolved
Hide resolved
domain/src/test/java/org/oppia/android/domain/audio/AudioPlayerControllerTest.kt
Outdated
Show resolved
Hide resolved
…rControllerTest.kt changed test function name Co-authored-by: Adhiambo Peres <[email protected]>
…se' into fix-mediaplayer-initialize-release
@subhajitxyz, once your PR is ready for review, assign back to the reviewer. |
Hi @subhajitxyz, here is a guide to assign PRs to reviewers: https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section |
Thanks @theMr17 |
@adhiamboperes, PTAL. |
Hi @subhajitxyz, update your branch with base branch, before assigning back to reviewer and make sure all checks pass. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @subhajitxyz! This LGTM!
Could you please fill this form in once this PR has been merged?
Assigning @BenHenning for code owner reviews. Thanks! |
Thanks @adhiamboperes , I will fiil the form. |
Explanation
Fixes #5451
The error occurs if the release is attempted before the MediaPlayer is initialized or if releaseMediaPlayer() is called multiple times. To address this, I have modified releaseMediaPlayer() to only release the MediaPlayer if it has not already been released. These changes properly handle the situation when releaseMediaPlayer() is called before initialization.
Essential Checklist
For UI-specific PRs only
If your PR includes UI-related changes, then: