Fix #1572: Story Activity orientation change bug

[anandwana001]

Explanation

Fix #1572

Test passes with fix

Test fail without fix

androidx.test.espresso.base.DefaultFailureHandler$AssertionFailedWithCauseError: Wanted to match 1 intents. Actually matched 0 intents.

IntentMatcher: has component: has component with: class name: is "org.oppia.app.player.exploration.ExplorationActivity" package name: an instance of java.lang.String short class name: an instance of java.lang.String

Matched intents:[]

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.

[rt4914]

This does work. But the main issue is that why do we need this setRetainInstance in this fragment and not in other fragments.
Exploration can be started from three places:

1. RecentlyPlayed -> In this we don't need setRetainInstance
2. TopicLessons -> In this we don't need setRetainInstance
3. StoryFragment -> In this if you remove this setRetainInstance and investigate you will notice that click is working but the domain layer of exploration is not able to send back the result correctly to app layer.
   So, although you solution works but it does not solve the issues which is in domain layer.

[anandwana001]

This does work. But the main issue is that why do we need this setRetainInstance in this fragment and not in other fragments.
Exploration can be started from three places:

1. RecentlyPlayed -> In this we don't need setRetainInstance
2. TopicLessons -> In this we don't need setRetainInstance
3. StoryFragment -> In this if you remove this setRetainInstance and investigate you will notice that click is working but the domain layer of exploration is not able to send back the result correctly to app layer.
   So, although you solution works but it does not solve the issues which is in domain layer.

What i found here is the data which is coming from controller, we are observing it, and for this we are using fragment instance.
Also, why we need in this only is, this is the only fragment presenter where we are doing using .registerViewBinder()

[BenHenning]

@anandwana001 this PR is lacking detail on what you've tried, and what is specifically broken. If there's an issue with bindable adapter, what is that issue? We should make sure that we fully understand the causes behind the underlying bug, and fix those. setRetainInstance feels more like hiding the problem than fixing the underlying issue which could cause other problems that we haven't yet noticed. Can you add more information on this & change the solution to fix the underlying problems, instead?

[anandwana001]

@anandwana001 this PR is lacking detail on what you've tried, and what is specifically broken. If there's an issue with bindable adapter, what is that issue? We should make sure that we fully understand the causes behind the underlying bug, and fix those. setRetainInstance feels more like hiding the problem than fixing the underlying issue which could cause other problems that we haven't yet noticed. Can you add more information on this & change the solution to fix the underlying problems, instead?

@BenHenning Firstly, What I remember is a ViewModel should not have any android related dependency. Here, in our case, If you check this line of code where we use a fragment inside a ViewModel which I see here as an issue.
After the screen rotation, this ViewModel lost this fragment and after clicking on an item no observe happens.
Also,

We are observing the data inside a ViewModel, which I think should happen in a fragment presenter, as it is happening for all other data observing our codebase.

Without the fix, I had taken log of the fragment:

Log.d("fragment", fragment.toString()) 

print Internal unique name for this fragment - format <FragmentName{identityHashCode} (UUID) id=0x>
mFragmentId only prints if not zero. After rotation our mFragmentId is zero.

// before rotation
StoryFragment{99c4ef4} (f3cd78b3-96e9-4ac2-bc30-a91806c7c86f) id=0x7f090277}

// after rotation
StoryFragment{99c4ef4} (296ea370-5b4a-41d1-86e6-b6e2cb3da14c)}

Logging the context of fragment

Log.d("fragment", fragment.context.toString())

// before rotation
org.oppia.app.story.StoryActivity@4066dae

// after rotation
null

If we see for TopicLessonFragment

// before rotation
TopicLessonsFragment{9f0c8c7} (a9cdc6b2-1395-4709-96ae-46d042c4ff70) id=0x7f0902d0}

//after rotation
TopicLessonsFragment{583ca50} (a9cdc6b2-1395-4709-96ae-46d042c4ff70) id=0x7f0902d0}

[anandwana001]

Similar issue - #1676

[BenHenning]

Thanks @anandwana001. As discussed during the meeting, we have two different approaches that we can take:

1. Properly managing the view model to ensure it doesn't leak references to activity/fragment/view. Your PR is on the right track, but it still keeps a reference to fragment. Fixing this broadly is probably going to be tricky.

2. Removing the AndroidX ViewModel uses in this case and just directly inject the fragment-scoped object. In cases when we don't actually need to retain state across configuration changes, this is probably cleaner. It's valid for view models that are fragment scoped to depend on its corresponding fragment and that fragment's activity (hence why it can be injected).

I suggest trying approach (2) for these two issues to see if it resolves the problem. Hopefully that's a bit simpler.

Also, please make sure to add tests for this case as well. I'd expect it to be reproable with a test that triggers a configuration change.

[anandwana001]

Thanks @anandwana001. As discussed during the meeting, we have two different approaches that we can take:

1. Properly managing the view model to ensure it doesn't leak references to activity/fragment/view. Your PR is on the right track, but it still keeps a reference to fragment. Fixing this broadly is probably going to be tricky.
2. Removing the AndroidX ViewModel uses in this case and just directly inject the fragment-scoped object. In cases when we don't actually need to retain state across configuration changes, this is probably cleaner. It's valid for view models that are fragment scoped to depend on its corresponding fragment and that fragment's activity (hence why it can be injected).

I suggest trying approach (2) for these two issues to see if it resolves the problem. Hopefully that's a bit simpler.

Also, please make sure to add tests for this case as well. I'd expect it to be reproable with a test that triggers a configuration change.

Sure, I will definitely keep in mind about the test cases point from now. Thanks.

[rt4914]

Same as other PR. This does work but will need to defer to @BenHenning about the approach.

[anandwana001]

Same as other PR. This does work but will need to defer to @BenHenning about the approach.

Thanks for having a look at this, might my save time to complete this asap.

[BenHenning]

@anandwana001 can you include the details of the failure that the test catches (e.g. what exactly is failing) in the case where the fix isn't present? That's helpful to ensure the test is catching the same issue.

Otherwise, this LGTM!