Fixes #4470, #4471, #4472, #4473, #4474, #4708: Handle configuration change using on saved instance #4668

vrajdesai78 · 2022-10-19T17:13:01Z

Explanation

Fixes #4470: State of input interactions is retained on configuration change using onSavedInstance.
Fixes #4471: Retain state for Text based interactions
Fixes #4472: Retain state for ImageRegion
Fixes #4473: Retain state for DragAndDrop
Fixes #4474: Retain State for SelectionInteraction
Fixes #4708: Fixed issue of returning null view in onSubmit time error

Essential Checklist

The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
Any changes to scripts/assets files have their rationale included in the PR explanation.
The PR follows the style guide.
The PR does not contain any unnecessary code changes from Android Studio (reference).
The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
The PR is assigned to the appropriate reviewers (reference).

Demo Video

ScreenRotation.mp4

Passing esspresso tests

BenHenning · 2022-10-21T06:10:21Z

Since we discussed how to proceed here, I don't think I need to take any further review action at this time. Assigning back to you @vrajdesai78.

…into handle-configuration-change-using-onSavedInstance

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

BenHenning

Thanks @vrajdesai78. Took a mostly fully pass (didn't look at questions or tests yet since both are probably gonna change yet).

Please also make sure that all current interactions are correctly tested such that we can demonstrate that rotation logic does work upon configuration change.

app/src/main/java/org/oppia/android/app/player/state/StateFragment.kt

app/src/main/java/org/oppia/android/app/player/state/StateFragmentPresenter.kt

model/src/main/proto/exploration.proto

...org/oppia/android/app/player/state/itemviewmodel/ImageRegionSelectionInteractionViewModel.kt

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

rt4914 · 2022-11-13T07:57:14Z

Resolved all my open conversations.

vrajdesai78 · 2022-11-13T21:16:09Z

When testing this locally @vrajdesai78 I noticed two things:

For multiple choice & item selection interactions, rotating causes the bullet items/checkboxes to animate slightly (as if they were just clicked). This seems wrong since the UI should be retaining the exact state, not showing as though the items were recently clicked.

The image region selection interaction isn't correctly retaining the default selection (i.e. when selecting a region that isn't one of the known regions), and confusingly reverts back to the last selected region after 2 rotations. We should be also retaining the default region location.

Please address both of these, and include a test for the default region one.

@BenHenning I have tried to remove animation in selection interaction view by calling jumpDrawablesToCurrentState(), but the issue is with my current implementation the animation got removed even after the screen rotates means once previously selected item is selected then also if user selects any other item the animation is removed. Do you have any suggestion how can I fix this issue, currently I am calling jumpDrawablesToCurrentState() on setOnChangedListener, is there any other way to do this.

Also, to retain state for default image region, do you have any suggestions how can I retain it's state.

seanlip · 2022-11-13T21:19:04Z

@vrajdesai78 Just a note, this discussion will likely be more efficient if you can explain what approaches you have tried and have considered for e.g. image region.

For the animation it would help to see a debugging doc with your understanding of the problem and the current flows.

vrajdesai78 · 2022-11-13T21:44:12Z

You can find more info about problem I am facing to reset animation in selection interaction from this debugging doc, thanks.

BenHenning

Left some follow-up comments, and one on the doc @vrajdesai78.

For the checkbox, does setChecked() cause the animation to trigger? If so, we may need to look into using view attributes and/or a custom view to initialize the checkbox to off. That being said, if we can't find an obvious solution to this, it might be fine for us to live with (but we should file an issue to track it).

Regarding image region selection, what have you tried? You'll need to look into how the default region is represented and make sure that we're able to re-add the default view upon reinitializing the view model.

BenHenning · 2022-11-13T22:44:03Z

app/src/main/java/org/oppia/android/app/player/state/SelectionInteractionView.kt

+import kotlinx.android.synthetic.main.item_selection_interaction_items.view.*
+import kotlinx.android.synthetic.main.multiple_choice_interaction_items.view.*


Please avoid using synthetic imports--these are deprecated.

BenHenning · 2022-11-13T22:44:13Z

app/src/main/java/org/oppia/android/app/player/state/SelectionInteractionView.kt

-  @field:[Inject ExplorationHtmlParserEntityType] lateinit var entityType: String
-  @field:[Inject DefaultResourceBucketName] lateinit var resourceBucketName: String
+  @field:[Inject ExplorationHtmlParserEntityType]
+  lateinit var entityType: String
+  @field:[Inject DefaultResourceBucketName]
+  lateinit var resourceBucketName: String


Please revert style-only changes.

reverted back

BenHenning · 2022-11-13T22:50:26Z

.../main/java/org/oppia/android/app/player/state/itemviewmodel/SelectionInteractionViewModel.kt

@@ -77,6 +77,7 @@ class SelectionInteractionViewModel private constructor(
      rawUserAnswer.itemSelection.selectedIndexesList.forEach { index ->
        selectedItems += index
        updateIsAnswerAvailable()
+        choiceItems[index].disableAnimation = true


Is this going to keep the animation disabled with additional user interactions?

reverted back changes as this approach won't work

vrajdesai78 · 2022-11-14T04:32:50Z

Hey @BenHenning I have checked how we actually get the unlabeled region, so we use have onClickListener inside an init method of ClickableAreasImage.kt. To retain the state of default region I need to access it's exact coordinates inside ImageRegionSelectionViewModel because other than it's coordinates there is no other way we can access unlabelled region.

My query is how can we get this exact coordinates of selected region inside an ImageRegionSelectionViewModel, for labelled region we simply retrieve it's coordinates from selectableRegions, but for unlabelled region we can't retrieve it's coordinates from selectable regions as it doesn't have label.

For selection Interaction I have tried to use setChecked inside a custom view, for that I send selected indices from viewModel to custom view and inside that view I have tried to use setChecked on selected indices, but the view is returning null as I won't able to directly access particular checkbox to mark it as checked, also here I think that even if I am able check checkbox still it will show animation as by default checkboxes have animation.

BenHenning · 2022-11-14T04:45:20Z

@vrajdesai78 let’s skip the animation. For the default region, how do you propose we pipe the necessary information?

vrajdesai78 · 2022-11-14T04:46:24Z

@BenHenning can we remove the support for default region for normal users like we do for screen reader users as I don't think that it will create any problem in user experience as selecting unlabelled region won't help user to learn anything, just saying if we remove it then we don't need to handle the case to retain the state of unlabelled region. Also, mostly it is expected that user clicks on labelled region only as the interaction itself is to click on image regions.

vrajdesai78 · 2022-11-14T04:48:46Z

@vrajdesai78 let’s skip the animation. For the default region, how do you propose we pipe the necessary information?

We need to get the exact coordinates which user has clicked inside an ImageRegionSelectionViewModel from that we can pass it inside lastSelectedRegion. I want to know how we can actually get that coordinates or unlabelled selected region inside a viewModel.

BenHenning · 2022-11-14T04:58:49Z

@vrajdesai78 this is a problem for sighted users; config changes affect all users, not just screenreader users.

For the coordinates, the default region is known by the clickable image utility. Can we derive the coordinates from there?

vrajdesai78 · 2022-11-14T05:02:39Z

Inside clickable for default region we won't have any parameters like for labelled region we have label and content description

BenHenning · 2022-11-14T05:13:09Z

oppia-android/app/src/main/java/org/oppia/android/app/player/state/itemviewmodel/ImageRegionSelectionInteractionViewModel.kt

Line 60 in 5e64fae

override fun onClickableAreaTouched(region: RegionClickedEvent) {

couldn’t we just pass along the coordinates? I understand that we can’t pipe the necessary data today, but we need to change it so that we can.

vrajdesai78 · 2022-11-14T06:36:15Z

@BenHenning added a parameter to store coordinates according to your suggestion. Can you PTAL.

BenHenning

I think this is on the right track @vrajdesai78 but it’s not a complete solution yet. You need to reconstitute the default region from the coordinates and make sure the coordinates are included in the raw user answer.

BenHenning · 2022-11-14T06:54:04Z

app/src/main/java/org/oppia/android/app/utility/OnClickableAreaClickedListener.kt

@@ -8,6 +10,10 @@ interface OnClickableAreaClickedListener {
   * For an specified region it will be called with [NamedRegionClickedEvent] with region name.
   * For an unspecified region it will be called with [DefaultRegionClickedEvent].
   *
+   * @param coordinates the coordinates of unlabelled region


Wouldn’t this be the coordinates of the region? Why is it specifically the unlabeled region?

oppiabot · 2022-11-26T05:54:42Z

Hi @vrajdesai78, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

vrajdesai78 added 3 commits October 19, 2022 22:36

RawUserAnswer added

9a7da9a

Imports optimized

20280f1

Optimized import

3625a63

BenHenning self-assigned this Oct 19, 2022

BenHenning assigned vrajdesai78 and unassigned BenHenning Oct 21, 2022

vrajdesai78 added 5 commits October 21, 2022 17:08

rawUserAnswer added to constructor of StateItemViewModel

6f64dcc

Merge branch 'develop' of https://github.com/vrajdesai78/oppia-android …

002b500

…into handle-configuration-change-using-onSavedInstance

Merge branch 'develop' of https://github.com/vrajdesai78/oppia-android …

f4c93f6

…into handle-configuration-change-using-onSavedInstance

Platform parameter added

c4293c5

Optimized code

e8863a6

vrajdesai78 assigned BenHenning and rt4914 Oct 21, 2022

vrajdesai78 marked this pull request as ready for review October 21, 2022 18:12

vrajdesai78 requested review from BenHenning and rt4914 as code owners October 21, 2022 18:12

vrajdesai78 commented Oct 21, 2022

View reviewed changes

app/src/sharedTest/java/org/oppia/android/app/player/state/StateFragmentTest.kt Outdated Show resolved Hide resolved

vrajdesai78 removed their assignment Oct 21, 2022

BenHenning assigned vrajdesai78 and unassigned BenHenning Oct 22, 2022

vrajdesai78 assigned BenHenning Oct 22, 2022

BenHenning removed their assignment Oct 22, 2022

vrajdesai78 assigned BenHenning Oct 22, 2022

vrajdesai78 added 3 commits October 24, 2022 12:08

Kdoc added

cf4f093

added platformparameter in bazel

1f0d530

Updated bazel

4de5907

BenHenning requested changes Oct 25, 2022

View reviewed changes

BenHenning removed their assignment Oct 25, 2022

Improved data pipeline and nit changes

d9a79bf

rt4914 removed their assignment Nov 13, 2022

vrajdesai78 added 2 commits November 14, 2022 02:39

Removed animation

650fdab

Optimized code

444406c

vrajdesai78 assigned BenHenning Nov 13, 2022

BenHenning reviewed Nov 13, 2022

View reviewed changes

BenHenning removed their assignment Nov 13, 2022

Reverted back changes for selection interaction

992c479

vrajdesai78 assigned BenHenning Nov 14, 2022

vrajdesai78 added 2 commits November 14, 2022 12:04

added coordinates param

37f59fd

Updated Kdoc

ab533b6

BenHenning reviewed Nov 14, 2022

View reviewed changes

BenHenning removed the HACKTOBERFEST-ACCEPTED label Nov 18, 2022

BenHenning removed their assignment Nov 19, 2022

oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Nov 26, 2022

oppiabot bot closed this Dec 3, 2022

BenHenning added the TODO(Ben): Revive label Dec 3, 2022

adhiamboperes mentioned this pull request Jun 9, 2023

Fix part of #4470: Save state of pending answer on configuration change in StateFragment #5008

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #4470, #4471, #4472, #4473, #4474, #4708: Handle configuration change using on saved instance #4668

Fixes #4470, #4471, #4472, #4473, #4474, #4708: Handle configuration change using on saved instance #4668

vrajdesai78 commented Oct 19, 2022 •

edited

Loading

BenHenning commented Oct 21, 2022

BenHenning left a comment

rt4914 commented Nov 13, 2022

vrajdesai78 commented Nov 13, 2022

seanlip commented Nov 13, 2022

vrajdesai78 commented Nov 13, 2022

BenHenning left a comment

BenHenning Nov 13, 2022

BenHenning Nov 13, 2022

vrajdesai78 Nov 14, 2022

BenHenning Nov 13, 2022

vrajdesai78 Nov 14, 2022

vrajdesai78 commented Nov 14, 2022

BenHenning commented Nov 14, 2022

vrajdesai78 commented Nov 14, 2022 •

edited

Loading

vrajdesai78 commented Nov 14, 2022 •

edited

Loading

BenHenning commented Nov 14, 2022

vrajdesai78 commented Nov 14, 2022

BenHenning commented Nov 14, 2022

vrajdesai78 commented Nov 14, 2022

BenHenning left a comment

BenHenning Nov 14, 2022

oppiabot bot commented Nov 26, 2022

		import kotlinx.android.synthetic.main.item_selection_interaction_items.view.*
		import kotlinx.android.synthetic.main.multiple_choice_interaction_items.view.*

Fixes #4470, #4471, #4472, #4473, #4474, #4708: Handle configuration change using on saved instance #4668

Fixes #4470, #4471, #4472, #4473, #4474, #4708: Handle configuration change using on saved instance #4668

Conversation

vrajdesai78 commented Oct 19, 2022 • edited Loading

Explanation

Essential Checklist

Demo Video

Passing esspresso tests

BenHenning commented Oct 21, 2022

BenHenning left a comment

Choose a reason for hiding this comment

rt4914 commented Nov 13, 2022

vrajdesai78 commented Nov 13, 2022

seanlip commented Nov 13, 2022

vrajdesai78 commented Nov 13, 2022

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning Nov 13, 2022

Choose a reason for hiding this comment

BenHenning Nov 13, 2022

Choose a reason for hiding this comment

vrajdesai78 Nov 14, 2022

Choose a reason for hiding this comment

BenHenning Nov 13, 2022

Choose a reason for hiding this comment

vrajdesai78 Nov 14, 2022

Choose a reason for hiding this comment

vrajdesai78 commented Nov 14, 2022

BenHenning commented Nov 14, 2022

vrajdesai78 commented Nov 14, 2022 • edited Loading

vrajdesai78 commented Nov 14, 2022 • edited Loading

BenHenning commented Nov 14, 2022

vrajdesai78 commented Nov 14, 2022

BenHenning commented Nov 14, 2022

vrajdesai78 commented Nov 14, 2022

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning Nov 14, 2022

Choose a reason for hiding this comment

oppiabot bot commented Nov 26, 2022

vrajdesai78 commented Oct 19, 2022 •

edited

Loading

vrajdesai78 commented Nov 14, 2022 •

edited

Loading

vrajdesai78 commented Nov 14, 2022 •

edited

Loading