Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix #1930: "Tick icon" disappears on configuration change in QuestionPlayer #2318

Closed
wants to merge 9 commits into from
Closed

Fix #1930: "Tick icon" disappears on configuration change in QuestionPlayer #2318

wants to merge 9 commits into from

Conversation

MaskedCarrot
Copy link
Contributor

@MaskedCarrot MaskedCarrot commented Dec 25, 2020 •
edited
Loading

Explanation

Fixes #1930

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.
fixed.disappearing.Tick.icon.mp4

Tests

Roboelectric Espresso
robo espresso

@MaskedCarrot MaskedCarrot requested a review from rt4914 as a code owner December 25, 2020 08:24
@MaskedCarrot MaskedCarrot self-assigned this Dec 25, 2020
@chrk2205
Copy link
Contributor

@MaskedCarrot
is this PR ready for review??

@anandwana001
Copy link
Contributor

Can we have a test cases checking for this case which fail without this pr implementation

@MaskedCarrot
Copy link
Contributor Author

@chrk2205 yes this pr is ready for review.

@MaskedCarrot
Copy link
Contributor Author

Can we have a test cases checking for this case which fail without this pr implementation

@anandwana001 should I file a different issues to implement the test case or write the test in this PR?

@anandwana001
Copy link
Contributor

In this pr only as we are fixing issue here, so test case will justify the correctness of implementation

@anandwana001 anandwana001 removed their assignment Jan 3, 2021
FareesHussain
FareesHussain suggested changes Jan 4, 2021
View reviewed changes
Copy link
Contributor

@FareesHussain FareesHussain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MaskedCarrot NIT changes

app/src/main/java/org/oppia/android/app/topic/questionplayer/QuestionPlayerViewModel.kt Outdated Show resolved Hide resolved
@FareesHussain FareesHussain assigned rt4914 and unassigned MaskedCarrot Jan 4, 2021
anandwana001
anandwana001 approved these changes Jan 6, 2021
View reviewed changes
Copy link
Contributor

@anandwana001 anandwana001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @MaskedCarrot
Test LGTM

...src/sharedTest/java/org/oppia/android/app/topic/questionplayer/QuestionPlayerActivityTest.kt Show resolved Hide resolved
rt4914
rt4914 suggested changes Jan 7, 2021
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also add : after issue number in title.

Fix #XXX :

app/src/main/java/org/oppia/android/app/topic/questionplayer/QuestionPlayerViewModel.kt Outdated Show resolved Hide resolved
@rt4914 rt4914 assigned MaskedCarrot and unassigned rt4914 Jan 7, 2021
@MaskedCarrot MaskedCarrot changed the title Fix #1930 "Tick icon" disappears on configuration change in QuestionPlayer Fix #1930: "Tick icon" disappears on configuration change in QuestionPlayer Jan 7, 2021
@MaskedCarrot
Copy link
Contributor Author

Also add : after issue number in title.

Fix #XXX :

done

@MaskedCarrot MaskedCarrot requested a review from rt4914 January 7, 2021 16:49
@rt4914 rt4914 removed their assignment Jan 8, 2021
@MaskedCarrot MaskedCarrot assigned rt4914 and unassigned MaskedCarrot Jan 8, 2021
rt4914
rt4914 approved these changes Jan 8, 2021
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks.

@rt4914 rt4914 assigned BenHenning and unassigned rt4914 Jan 8, 2021
BenHenning
BenHenning requested changes Jan 9, 2021
View reviewed changes
Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @MaskedCarrot. Left one high-level comment on the approach. I don't think we want to solve it in this way since it's changing the design pattern of the assembler's builder, and I also think there might be a simpler approach overall. PTAL.

app/src/main/java/org/oppia/android/app/player/state/StatePlayerRecyclerViewAssembler.kt
private val isCorrectAnswer: ObservableField<Boolean> = ObservableField(false)

/** @param isCorrectAnswer is restored to its updated value on configuration change */
fun isAnswerCorrect(isCorrectAnswer: Boolean): Builder {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't the correct way to be doing this--the builder is designed to set up high-level UI features, not change super specific state.

The underlying issue that we're trying to solve is retaining state across configuration changes. We probably shouldn't be using the view model below, either, since that's set up incorrectly today (view models shouldn't be retrieved using ViewModelProvider since they can leak fragments/activities). Sorry--this probably wasn't clear.

It seems like perhaps the clearest way to solve this is to have the assembler itself ensure isCorrectAnswer is properly set in compute() based on whether we're in a completed state & the answer submitted is correct. I think if you did that approach it might also simplify the overall solution.

@BenHenning BenHenning assigned MaskedCarrot and unassigned BenHenning Jan 9, 2021
@MaskedCarrot
Copy link
Contributor Author

MaskedCarrot commented Jan 16, 2021 •
edited
Loading

closing this PR because I was not able to push a commit without force push. I will be continuing this work on #2493.

@MaskedCarrot MaskedCarrot deleted the TickIconDisappears branch February 1, 2021 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenHenning BenHenning BenHenning requested changes

@FareesHussain FareesHussain FareesHussain approved these changes

@ShridharGoel ShridharGoel ShridharGoel approved these changes

@anandwana001 anandwana001 anandwana001 approved these changes

@rt4914 rt4914 rt4914 approved these changes

Assignees

@MaskedCarrot MaskedCarrot

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

"Tick icon" disappears on configuration change in QuestionPlayer
7 participants
@MaskedCarrot @chrk2205 @anandwana001 @rt4914 @BenHenning @ShridharGoel @FareesHussain