Fix #2476: Fixed Revealing Second hint due to orientation change [Blocked on #2572]

[FareesHussain]

Explanation

Fixes #2476

Checklist

• The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
• The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
• The PR follows the style guide.
• The PR does not contain any unnecessary auto-generated code from Android Studio.
• The PR is made from a branch that's not called "develop".
• The PR is made from a branch that is up-to-date with "develop".
• The PR's branch is based on "develop" and not on any other branch.
• The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

[FareesHussain]

@BenHenning I'm facing an issue with the tests that are running in the StateFragmentTestActivity
for the states/questions in which the hints exist, the Hint is revealed without any interaction due to which my tests are failing

[BenHenning]

@FareesHussain I suggest using StateFragmentLocalTest for these tests, instead, since that's where we do most of the hints/solutions testing (due to them being inherently difficult to test on Espresso due to being heavily time-based). I suspect you'll only need to add the configuration test, too, since the other scenarios you're testing are already well tested.

[FareesHussain]

@BenHenning as you've suggested we need to make HintHandler Lifecycle-Aware can you please provide a bit more specific solution for this, also add some reference for this

[BenHenning]

@FareesHussain thought about this, and I think to solve it we will need to create some lifecycle awareness in the state recyclerview assembler. Specifically, I'm thinking you could add two new methods to the assembler:

• restoreState(Bundle) <-- called in onCreate() after creating the assembler and iff Bundle isn't null
• saveState(Bundle) <-- called in onSavedInstanceState()

With those two hooks, you'll then be able to save/restore HintHandler's internal state so that it properly survives configuration changes. That alone may be sufficient to fix the underlying issue here, and sets nice precedent for future cases when the assembler has special UI-only state to save/restore.

Ideally, we should also have a way to force the caller to properly hook up the saved checks, but I can't think yet of a way to do that. Unfortunately, LifecycleObserver doesn't support onSavedInstanceState.

[Luffy18346]

@FareesHussain PTAL

[BenHenning]

Thanks @FareesHussain! Left a reply on the earlier thread. Also, to clarify my comment above: we need to add tests for both state & question player in this PR, which may in turn require fixing some of the peripheral breakages in these tests to support orientation changes. I think this may also be blocked on #2572 but I wanted you to confirm that for me first. Per our discussion, I think you'll either need to start with question player or fix the media source issue in StateFragmentLocalTest.

[FareesHussain]

@BenHenning I've tried to look deeper into this (Don't know what is causing the audio Bug yet).

What I think is before fixing the audio bug we need to test if #2572 will actually work in this scenario.

testCoroutineDispatchers.runCurrent() is where we get the Audio bug while running the test This might be something related to ActivityRotator.kt or Due to configuration change (not sure). So I've tried to coment the following lines to avoid the bug.

oppia-android/app/src/main/java/org/oppia/android/app/player/audio/AudioViewModel.kt

Line 113 in e15c6a7

audioPlayerController.changeDataSource(voiceOverToUri(voiceoverMap[languageCode]))

And by running the test

  @Test
  fun testStateFragment_nextState_viewHint_rotateLandscape_noNewHintIsAvailable() {
    launchForExploration(FRACTIONS_EXPLORATION_ID_1).use {
      startPlayingExploration()
      playThroughState1()
      produceAndViewFirstHint()

//      testCoroutineDispatchers.advanceTimeBy(TimeUnit.SECONDS.toMillis(30))
      it.rotateToLandscape()

      onView(withId(R.id.dot_hint)).check(matches(isDisplayed()))
    }
  }

It was passing for isDisplayed() and fails for not(isDisplayed())

And as you've mentioned to add tests for QuestionPlayer it is the same as StateFragment (i.e, HintsAndSolutionConfigFastShowTestModule for Espresso).

So this PR is blocked on #2572 and Also the Audio Bug (Will have to file a separate issue for this once we know the cause).

Assigning this to you to have a look.

[BenHenning]

Thanks for confirming @FareesHussain. I'll prioritize the landscape work, though I might not get to it until later in the week. I still suspect that the audio issue is due to an arrangement problem in the test (e.g. we may be playing new audio that isn't being properly faked in setUp()--we've seen this in other tests in the past), but it makes sense that this is blocked on #2572 otherwise.

[anandwana001]

@FareesHussain Can we unblock this from #2572 as we can write a test and confirm its result over espresso, only thing left to be done is to confirm over robolectric, but later after merging #2572 we do require a big walkthrough over all test cases to confirm if they are correct over landscape change or not.

[FareesHussain]

@anandwana001 Actually here we can't write tests for Hints and Solutions in Espresso as we use the annotation HintsAndSolutionConfigFastShowTestModule which shows the hints even without any interaction.

oppia-android/app/src/sharedTest/java/org/oppia/android/app/player/state/StateFragmentTest.kt

Line 1410 in 0027ded

HintsAndSolutionConfigFastShowTestModule::class, WorkManagerConfigurationModule::class,

[BenHenning]

Closing this per #2572 (comment). We should revive it once time frees up to finish the rotation utility.

[BenHenning]

Note that this was replaced by #3659 which moves the hint handler to the domain layer so that the hint state machine isn't entirely dependent on UI lifecycles which allows it to work across configuration changes.

Thanks again for all the work you did on this PR @FareesHussain. It was a really big help in framing other work around how to retain UI state across configuration changes, and I think it'll be a key part in solving #1737.