Fix #2667: Wrong Tab selected on configuration change in TopicFragment #2670

MaskedCarrot · 2021-02-09T13:24:20Z

Explanation

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.

gif

21-02-09-18-46-29.mp4

Espresso Tests

without fix	with fix

Tests after the changes made in this PR.

Espresso	Robolectric

anandwana001 · 2021-02-10T11:30:01Z

Few points:

Can we check why do we have this issue at first place? What's causing this issue?
If we are solving this, can we have test case related to it?

Currently, we do have a test to test this issue testTopicFragment_clickOnPracticeTab_configurationChange_showsSameTabAndItsContent and it's passing for me.

Could you please confirm!!.

FareesHussain · 2021-02-10T13:23:29Z

@anandwana001 the bug only occurs when navigated through Promoted stories. I guess we need to test it using HomeActivityTest

@MaskedCarrot Your implementation does fix this. But you need to track down what might be causing this issue before fixing this, to avoid further bugs related to this. This is also weird that it is only reproduced when navigated through Promoted stories.

MaskedCarrot · 2021-02-10T13:26:44Z

This issue is caused because in the handleOnCreate of TopicFragmentPresenter we are setting the viewpager to lessons/info fragment every time the activity is recreated.

rt4914

@MaskedCarrot Please add a test case for this specific case. The test should be added in TopicFragmentTest.

anandwana001 · 2021-02-11T01:02:31Z

@rt4914 before going for the test case, could you confirm if the approach looks good to you? Also, what is the root cause of this issue before fixing it? As we are landing on the same topic page but from two different places, what's the difference here?

rt4914

@MaskedCarrot PTAL

app/src/main/java/org/oppia/android/app/topic/TopicFragment.kt

app/src/main/java/org/oppia/android/app/topic/TopicFragmentPresenter.kt

rt4914 · 2021-02-11T16:09:33Z

@rt4914 before going for the test case, could you confirm if the approach looks good to you? Also, what is the root cause of this issue before fixing it? As we are landing on the same topic page but from two different places, what's the difference here?

@anandwana001 Yeah the approach is correct. Only the naming is just opposite.

Cause: The code is incorrect. It should be like this: if we are on same screen and do orientation change than nothing should change.

From promoted story we show Topic-Lessons in expanded form and from All topics section we should Topic Info.

…-change' into topic-tab-changes-on-orientation-change

anandwana001

Nit changes on the Test file.

app/src/sharedTest/java/org/oppia/android/app/topic/TopicFragmentTest.kt

FareesHussain

LGTM. Added a commet please check

FareesHussain · 2021-02-18T08:17:40Z

app/src/sharedTest/java/org/oppia/android/app/topic/TopicFragmentTest.kt

@@ -375,6 +375,7 @@ class TopicFragmentTest {
        targetViewId = R.id.master_skills_text_view,
        stringToMatch = "Master These Skills"
      )
+      testCoroutineDispatchers.runCurrent()
      onView(isRoot()).perform(orientationLandscape())
      testCoroutineDispatchers.runCurrent()


Should we remove this?

I think we can remove this because after clicking on the tab we have a runCurrent() which will provide the delay to switch the tab.

Check if Robolectric passing or not with/without this stabilizing.

The robolectric test are passing with or without testCoroutineDispatchers.runCurrent()

removed unnecessary testCoroutineDispatchers.runCurrent() and checked that the tests are still correctly passing on espresso (real device and emulator) and robolectric.

added screenshots for the same in description.

FareesHussain

LGTM,
Can you confirm that you are able to see the screen rotation while running the espresso tests.

MaskedCarrot · 2021-02-24T10:05:44Z

yes I can see the orientation change

anandwana001 · 2021-02-24T16:56:25Z

I will take a look at this tomorrow.

anandwana001

LGTM

BenHenning

Thanks @MaskedCarrot! I had a couple thoughts, but feel free to resolve those conversations. The PR LGTM. Please re-assign me if you have any follow-up questions that you'd like my thoughts on.

app/src/main/java/org/oppia/android/app/topic/TopicFragment.kt

app/src/main/java/org/oppia/android/app/topic/TopicFragmentPresenter.kt

rt4914

LGTM, thanks.

MaskedCarrot added 2 commits February 9, 2021 18:40

fixed tab

a63dd1a

fixed lint

75d9c54

MaskedCarrot requested a review from rt4914 as a code owner February 9, 2021 13:24

MaskedCarrot assigned rt4914 Feb 9, 2021

FareesHussain assigned MaskedCarrot and rt4914 and unassigned rt4914 Feb 10, 2021

rt4914 suggested changes Feb 10, 2021

View reviewed changes

rt4914 removed their assignment Feb 10, 2021

rt4914 self-assigned this Feb 11, 2021

rt4914 suggested changes Feb 11, 2021

View reviewed changes

app/src/main/java/org/oppia/android/app/topic/TopicFragment.kt Outdated Show resolved Hide resolved

app/src/main/java/org/oppia/android/app/topic/TopicFragmentPresenter.kt Outdated Show resolved Hide resolved

rt4914 removed their assignment Feb 11, 2021

MaskedCarrot and others added 5 commits February 14, 2021 16:01

added test and fixed namming

9bde7a7

Merge branch 'develop' into topic-tab-changes-on-orientation-change

e73b370

lint fix

33cbb13

Merge remote-tracking branch 'origin/topic-tab-changes-on-orientation…

de2dc41

…-change' into topic-tab-changes-on-orientation-change

fixed variable name

ee0e8cc

MaskedCarrot assigned rt4914 and unassigned MaskedCarrot Feb 14, 2021

MaskedCarrot requested a review from rt4914 February 14, 2021 14:06

anandwana001 suggested changes Feb 15, 2021

View reviewed changes

anandwana001 assigned MaskedCarrot Feb 15, 2021

nit fix

40ba8b3

MaskedCarrot requested a review from anandwana001 February 15, 2021 06:30

MaskedCarrot removed their assignment Feb 15, 2021

FareesHussain approved these changes Feb 18, 2021

View reviewed changes

FareesHussain assigned anandwana001 and unassigned MaskedCarrot Feb 18, 2021

anandwana001 assigned MaskedCarrot and unassigned anandwana001 Feb 18, 2021

BenHenning added the PR don't merge - NEEDS UPDATE Corresponds to PRs that need to be updated with the latest develop changes before they can be merged label Feb 24, 2021

Merge branch 'develop' into topic-tab-changes-on-orientation-change

e304a17

BenHenning removed the PR don't merge - NEEDS UPDATE Corresponds to PRs that need to be updated with the latest develop changes before they can be merged label Feb 24, 2021

MaskedCarrot and others added 2 commits February 24, 2021 12:57

Merge branch 'develop' into topic-tab-changes-on-orientation-change

893b32e

nit

6f09fad

MaskedCarrot assigned anandwana001 and FareesHussain and unassigned MaskedCarrot Feb 24, 2021

FareesHussain approved these changes Feb 24, 2021

View reviewed changes

FareesHussain removed their assignment Feb 24, 2021

anandwana001 approved these changes Feb 25, 2021

View reviewed changes

anandwana001 assigned MaskedCarrot and unassigned anandwana001 Feb 25, 2021

MaskedCarrot assigned rt4914 and BenHenning and unassigned MaskedCarrot Feb 25, 2021

BenHenning approved these changes Feb 26, 2021

View reviewed changes

app/src/main/java/org/oppia/android/app/topic/TopicFragment.kt Outdated Show resolved Hide resolved

app/src/main/java/org/oppia/android/app/topic/TopicFragmentPresenter.kt Outdated Show resolved Hide resolved

BenHenning assigned MaskedCarrot and unassigned BenHenning Feb 26, 2021

nit

eec0adc

MaskedCarrot removed their assignment Feb 26, 2021

rt4914 approved these changes Feb 28, 2021

View reviewed changes

rt4914 merged commit 1467141 into oppia:develop Feb 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #2667: Wrong Tab selected on configuration change in TopicFragment #2670

Fix #2667: Wrong Tab selected on configuration change in TopicFragment #2670

MaskedCarrot commented Feb 9, 2021 •

edited

Loading

anandwana001 commented Feb 10, 2021

FareesHussain commented Feb 10, 2021

MaskedCarrot commented Feb 10, 2021

rt4914 left a comment

anandwana001 commented Feb 11, 2021

rt4914 left a comment

rt4914 commented Feb 11, 2021

anandwana001 left a comment

FareesHussain left a comment

FareesHussain Feb 18, 2021

MaskedCarrot Feb 18, 2021

anandwana001 Feb 18, 2021

MaskedCarrot Feb 24, 2021

MaskedCarrot Feb 24, 2021

MaskedCarrot Feb 24, 2021

FareesHussain left a comment

MaskedCarrot commented Feb 24, 2021

anandwana001 commented Feb 24, 2021

anandwana001 left a comment

BenHenning left a comment

rt4914 left a comment

Fix #2667: Wrong Tab selected on configuration change in TopicFragment #2670

Fix #2667: Wrong Tab selected on configuration change in TopicFragment #2670

Conversation

MaskedCarrot commented Feb 9, 2021 • edited Loading

Explanation

Checklist

gif

Espresso Tests

Tests after the changes made in this PR.

anandwana001 commented Feb 10, 2021

FareesHussain commented Feb 10, 2021

MaskedCarrot commented Feb 10, 2021

rt4914 left a comment

Choose a reason for hiding this comment

anandwana001 commented Feb 11, 2021

rt4914 left a comment

Choose a reason for hiding this comment

rt4914 commented Feb 11, 2021

anandwana001 left a comment

Choose a reason for hiding this comment

FareesHussain left a comment

Choose a reason for hiding this comment

FareesHussain Feb 18, 2021

Choose a reason for hiding this comment

MaskedCarrot Feb 18, 2021

Choose a reason for hiding this comment

anandwana001 Feb 18, 2021

Choose a reason for hiding this comment

MaskedCarrot Feb 24, 2021

Choose a reason for hiding this comment

MaskedCarrot Feb 24, 2021

Choose a reason for hiding this comment

MaskedCarrot Feb 24, 2021

Choose a reason for hiding this comment

FareesHussain left a comment

Choose a reason for hiding this comment

MaskedCarrot commented Feb 24, 2021

anandwana001 commented Feb 24, 2021

anandwana001 left a comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

rt4914 left a comment

Choose a reason for hiding this comment

MaskedCarrot commented Feb 9, 2021 •

edited

Loading