From f4a4a47ef65ae8a6df58a258ce59d26068ea2317 Mon Sep 17 00:00:00 2001 From: Tanish Moral <134790673+TanishMoral11@users.noreply.github.com> Date: Wed, 11 Dec 2024 12:49:48 +0530 Subject: [PATCH] Fix #5455: Resolve crash in AudioViewModel by initializing state variables (#5561) **Explanation:** Fixes #5455: - This PR addresses issue #5455, which involves a crash caused by `kotlin.UninitializedPropertyAccessException` in `AudioViewModel` when the device is rotated during exploration. - The fix ensures proper initialization of state variables in the `loadAudio()` function, which prevents the app from crashing due to uninitialized properties. **Essential Checklist:** - [x] The PR title and explanation each start with "Fix #5455: ". - [ ] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation. - [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide). - [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)). - [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop". - [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)). **Before Changes Video :** https://github.com/user-attachments/assets/649c9869-3ce6-4e0c-bafa-9769ff6d655f **After Changes Video :** https://github.com/user-attachments/assets/6ded81c1-4846-4328-bd2e-b5c25420808b **Screenshot of Passing Tests on Espresso:** ![Screenshot 2024-12-10 185043](https://github.com/user-attachments/assets/730e972b-65ab-4831-823a-eca0f0893376) --------- Co-authored-by: Adhiambo Peres <59600948+adhiamboperes@users.noreply.github.com> --- .../app/player/audio/AudioViewModel.kt | 15 +- .../app/player/audio/AudioFragmentTest.kt | 185 +++++++++++++++--- 2 files changed, 164 insertions(+), 36 deletions(-) diff --git a/app/src/main/java/org/oppia/android/app/player/audio/AudioViewModel.kt b/app/src/main/java/org/oppia/android/app/player/audio/AudioViewModel.kt index 07037abd873..e04b17f162a 100644 --- a/app/src/main/java/org/oppia/android/app/player/audio/AudioViewModel.kt +++ b/app/src/main/java/org/oppia/android/app/player/audio/AudioViewModel.kt @@ -72,6 +72,7 @@ class AudioViewModel @Inject constructor( } fun loadMainContentAudio(allowAutoPlay: Boolean, reloadingContent: Boolean) { + setStateAndExplorationId(state, explorationId) hasFeedback = false loadAudio(contentId = null, allowAutoPlay, reloadingContent) } @@ -88,7 +89,12 @@ class AudioViewModel @Inject constructor( * @param allowAutoPlay If false, audio is guaranteed not to be autoPlayed. */ private fun loadAudio(contentId: String?, allowAutoPlay: Boolean, reloadingMainContent: Boolean) { - val targetContentId = contentId ?: state.content.contentId + val targetContentId = when { + !contentId.isNullOrEmpty() -> contentId + this::state.isInitialized -> state.content.contentId + else -> "" + } + val voiceoverMapping = state.recordedVoiceoversMap[targetContentId] ?: VoiceoverMapping.getDefaultInstance() @@ -106,10 +112,9 @@ class AudioViewModel @Inject constructor( selectedLanguageName.set(locale.getDisplayLanguage(locale)) when { - selectedLanguageCode.isEmpty() && languages.any { - it == defaultLanguage - } -> setAudioLanguageCode(defaultLanguage) - languages.any { it == selectedLanguageCode } -> setAudioLanguageCode(selectedLanguageCode) + selectedLanguageCode.isEmpty() && languages.contains(defaultLanguage) -> + setAudioLanguageCode(defaultLanguage) + languages.contains(selectedLanguageCode) -> setAudioLanguageCode(selectedLanguageCode) languages.isNotEmpty() -> { autoPlay = false this.reloadingMainContent = false diff --git a/app/src/sharedTest/java/org/oppia/android/app/player/audio/AudioFragmentTest.kt b/app/src/sharedTest/java/org/oppia/android/app/player/audio/AudioFragmentTest.kt index dec40d9a790..a5edb919920 100644 --- a/app/src/sharedTest/java/org/oppia/android/app/player/audio/AudioFragmentTest.kt +++ b/app/src/sharedTest/java/org/oppia/android/app/player/audio/AudioFragmentTest.kt @@ -11,8 +11,10 @@ import androidx.appcompat.app.AppCompatActivity import androidx.test.core.app.ActivityScenario.launch import androidx.test.core.app.ApplicationProvider import androidx.test.espresso.Espresso.onView +import androidx.test.espresso.PerformException import androidx.test.espresso.UiController import androidx.test.espresso.ViewAction +import androidx.test.espresso.ViewInteraction import androidx.test.espresso.action.ViewActions.click import androidx.test.espresso.assertion.ViewAssertions.matches import androidx.test.espresso.matcher.RootMatchers.isDialog @@ -22,9 +24,12 @@ import androidx.test.espresso.matcher.ViewMatchers.isRoot import androidx.test.espresso.matcher.ViewMatchers.withContentDescription import androidx.test.espresso.matcher.ViewMatchers.withId import androidx.test.espresso.matcher.ViewMatchers.withText +import androidx.test.espresso.util.HumanReadables +import androidx.test.espresso.util.TreeIterables import androidx.test.ext.junit.runners.AndroidJUnit4 import com.google.common.truth.Truth.assertThat import dagger.Component +import org.hamcrest.CoreMatchers.allOf import org.hamcrest.Description import org.hamcrest.Matcher import org.hamcrest.TypeSafeMatcher @@ -113,6 +118,7 @@ import org.oppia.android.util.parser.image.ImageParsingModule import org.oppia.android.util.profile.CurrentUserProfileIdIntentDecorator.extractCurrentUserProfileId import org.robolectric.annotation.Config import org.robolectric.annotation.LooperMode +import java.util.concurrent.TimeoutException import javax.inject.Inject import javax.inject.Singleton @@ -125,8 +131,7 @@ import javax.inject.Singleton @RunWith(AndroidJUnit4::class) @LooperMode(LooperMode.Mode.PAUSED) @Config( - application = AudioFragmentTest.TestApplication::class, - qualifiers = "port-xxhdpi" + application = AudioFragmentTest.TestApplication::class, qualifiers = "port-xxhdpi" ) class AudioFragmentTest { @get:Rule @@ -174,8 +179,7 @@ class AudioFragmentTest { private fun createAudioFragmentTestIntent(profileId: Int): Intent { return AudioFragmentTestActivity.createAudioFragmentTestActivity( - context, - profileId + context, profileId ) } @@ -188,14 +192,13 @@ class AudioFragmentTest { ) ).use { testCoroutineDispatchers.runCurrent() - onView(withId(R.id.audio_progress_seek_bar)) - .check( - matches( - withContentDescription( - context.getString(R.string.audio_player_seekbar_content_description) - ) + onView(withId(R.id.audio_progress_seek_bar)).check( + matches( + withContentDescription( + context.getString(R.string.audio_player_seekbar_content_description) ) ) + ) } } @@ -208,14 +211,47 @@ class AudioFragmentTest { ) ).use { testCoroutineDispatchers.runCurrent() - onView(withId(R.id.audio_language_icon)) - .check( - matches( - withContentDescription( - context.getString(R.string.audio_language_icon_content_description) + onView(withId(R.id.audio_language_icon)).check( + matches( + withContentDescription( + context.getString(R.string.audio_language_icon_content_description) + ) + ) + ) + } + } + + @Test + fun testAudioFragment_playAudio_configurationChange_checkAudioPaused() { + addMediaInfo() + launch( + createAudioFragmentTestIntent( + internalProfileId + ) + ).use { + testCoroutineDispatchers.runCurrent() + + waitForTheView( + allOf( + withId(R.id.play_pause_audio_icon), + WithNonZeroDimensionsMatcher() + ) + ).perform(click()) + testCoroutineDispatchers.runCurrent() + + onView(withId(R.id.audio_progress_seek_bar)).perform(setProgress(100)) + + onView(isRoot()).perform(orientationLandscape()) + testCoroutineDispatchers.runCurrent() + onView(withId(R.id.play_pause_audio_icon)).check( + matches( + withContentDescription( + context.getString( + R.string.audio_play_description ) ) ) + ) } } @@ -228,10 +264,16 @@ class AudioFragmentTest { ) ).use { testCoroutineDispatchers.runCurrent() - onView(withId(R.id.play_pause_audio_icon)) - .check(matches(isDisplayed())) - onView(withId(R.id.play_pause_audio_icon)) - .check(matches(withContentDescription(context.getString(R.string.audio_play_description)))) + onView(withId(R.id.play_pause_audio_icon)).check(matches(isDisplayed())) + onView(withId(R.id.play_pause_audio_icon)).check( + matches( + withContentDescription( + context.getString( + R.string.audio_play_description + ) + ) + ) + ) } } @@ -250,8 +292,15 @@ class AudioFragmentTest { onView(withId(R.id.play_pause_audio_icon)).perform(click()) testCoroutineDispatchers.runCurrent() - onView(withId(R.id.play_pause_audio_icon)) - .check(matches(withContentDescription(context.getString(R.string.audio_pause_description)))) + onView(withId(R.id.play_pause_audio_icon)).check( + matches( + withContentDescription( + context.getString( + R.string.audio_pause_description + ) + ) + ) + ) } } @@ -268,8 +317,15 @@ class AudioFragmentTest { onView(withId(R.id.audio_progress_seek_bar)).perform(setProgress(100)) testCoroutineDispatchers.runCurrent() - onView(withId(R.id.play_pause_audio_icon)) - .check(matches(withContentDescription(context.getString(R.string.audio_play_description)))) + onView(withId(R.id.play_pause_audio_icon)).check( + matches( + withContentDescription( + context.getString( + R.string.audio_play_description + ) + ) + ) + ) } } @@ -290,8 +346,15 @@ class AudioFragmentTest { onView(withId(R.id.audio_progress_seek_bar)).perform(setProgress(100)) testCoroutineDispatchers.runCurrent() - onView(withId(R.id.play_pause_audio_icon)) - .check(matches(withContentDescription(context.getString(R.string.audio_pause_description)))) + onView(withId(R.id.play_pause_audio_icon)).check( + matches( + withContentDescription( + context.getString( + R.string.audio_pause_description + ) + ) + ) + ) } } @@ -307,8 +370,15 @@ class AudioFragmentTest { onView(withId(R.id.play_pause_audio_icon)).perform(click()) onView(withId(R.id.audio_progress_seek_bar)).perform(setProgress(100)) onView(isRoot()).perform(orientationLandscape()) - onView(withId(R.id.play_pause_audio_icon)) - .check(matches(withContentDescription(context.getString(R.string.audio_pause_description)))) + onView(withId(R.id.play_pause_audio_icon)).check( + matches( + withContentDescription( + context.getString( + R.string.audio_pause_description + ) + ) + ) + ) } } @@ -330,16 +400,22 @@ class AudioFragmentTest { onView(withId(R.id.audio_language_icon)).perform(click()) testCoroutineDispatchers.runCurrent() - onView(withText(R.string.hinglish_localized_language_name)) - .inRoot(isDialog()) + onView(withText(R.string.hinglish_localized_language_name)).inRoot(isDialog()) .perform(click()) testCoroutineDispatchers.runCurrent() onView(withText("Ok")).inRoot(isDialog()).perform(click()) testCoroutineDispatchers.runCurrent() - onView(withId(R.id.play_pause_audio_icon)) - .check(matches(withContentDescription(context.getString(R.string.audio_play_description)))) + onView(withId(R.id.play_pause_audio_icon)).check( + matches( + withContentDescription( + context.getString( + R.string.audio_play_description + ) + ) + ) + ) onView(withId(R.id.audio_progress_seek_bar)).check(matches(withSeekBarPosition(0))) } } @@ -540,4 +616,51 @@ class AudioFragmentTest { override fun getApplicationInjector(): ApplicationInjector = component } + /** Returns a matcher that matches view based on non-zero width and height. */ + private class WithNonZeroDimensionsMatcher : TypeSafeMatcher() { + + override fun matchesSafely(target: View): Boolean { + val targetWidth = target.width + val targetHeight = target.height + return targetWidth > 0 && targetHeight > 0 + } + + override fun describeTo(description: Description) { + description.appendText("with non-zero width and height") + } + } + + private fun waitForTheView(viewMatcher: Matcher): ViewInteraction { + return onView(isRoot()).perform(waitForMatch(viewMatcher, 30000L)) + } + + private fun waitForMatch(viewMatcher: Matcher, millis: Long): ViewAction { + return object : ViewAction { + override fun getDescription(): String { + return "wait for a specific view with matcher <$viewMatcher> during $millis millis." + } + + override fun getConstraints(): Matcher { + return isRoot() + } + + override fun perform(uiController: UiController?, view: View?) { + checkNotNull(uiController) + uiController.loopMainThreadUntilIdle() + val startTime = System.currentTimeMillis() + val endTime = startTime + millis + + do { + if (TreeIterables.breadthFirstViewTraversal(view).any { viewMatcher.matches(it) }) { + return + } + uiController.loopMainThreadForAtLeast(50) + } while (System.currentTimeMillis() < endTime) + + // Couldn't match in time. + throw PerformException.Builder().withActionDescription(description) + .withViewDescription(HumanReadables.describe(view)).withCause(TimeoutException()).build() + } + } + } }