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.

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

Handle configuration change using onSavedInstance #4470

Closed
vrajdesai78 opened this issue Jul 27, 2022 · 3 comments · Fixed by #5478 or subhajitxyz/oppia-android#3
Closed

Handle configuration change using onSavedInstance #4470

vrajdesai78 opened this issue Jul 27, 2022 · 3 comments · Fixed by #5478 or subhajitxyz/oppia-android#3
Assignees
Labels
bug End user-perceivable behaviors which are not desirable. Impact: Low Low perceived user impact (e.g. edge cases). Work: High It's not clear what the solution is. Z-ibt Temporary label for Ben to keep track of issues he's triaged.

Comments

@vrajdesai78
Copy link
Contributor

vrajdesai78 commented Jul 27, 2022

Problem
For all interactions in exploration player and question player when we rotate our device, selected or entered answer doesn't persist.

Solution
To solve this problem of configuration change we use onSavedInstance method to store the last entered user answer.

Alternatives

  • Singleton Class Approach: Complextiy of code will be increased if we use this approach as we have to write additional singleton class. Also, we need to handle issue of memory leak separately as it is not life-cycle aware.
  • ViewModel Approach: It is difficult to handle interactions when we have more than two interactions on one screen as all interactions are tightly coupled.

Note: Closing issue #1737 as the approach for configuration change is changed to onSavedInstance from Singleton class approach.

@vrajdesai78 vrajdesai78 self-assigned this Jul 27, 2022
@Broppia Broppia added issue_type_infrastructure Impact: Low Low perceived user impact (e.g. edge cases). labels Jul 29, 2022
@BenHenning BenHenning added Z-ibt Temporary label for Ben to keep track of issues he's triaged. issue_user_learner labels Sep 15, 2022
@seanlip seanlip added bug End user-perceivable behaviors which are not desirable. and removed issue_type_infrastructure labels Mar 28, 2023
@adhiamboperes adhiamboperes added the Work: Low Solution is clear and broken into good-first-issue-sized chunks. label Apr 18, 2023
@adhiamboperes adhiamboperes added Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. and removed Work: Low Solution is clear and broken into good-first-issue-sized chunks. labels May 16, 2023
@masclot
Copy link
Collaborator

masclot commented Jun 1, 2023

The ui/presenter is subscribed to get EphemeraState updates here. The subscription is not removed after first data comes, so theoretically new data can come at any time that recreates the ui. In this case, the user input is also lost. Should I handle this case as well or in practice it won't happen?

If it can happen and I need to handle the case, what changes can I expect? could the interaction id have changed? Even if the interaction id is the same, can it belong to a different question, in which case the saved data must be discarded?

Potential scenario I'm imagining:

  • The student opens Oppia both in smartphone and pc/web
  • Student opens the same exploration in both
  • Student types something in both, but submits only one

Another potential case:

  • The student opens Oppia android
  • Student navigates to an exploration and starts typing and answer
  • There is a change in the exploration configuration received over the net (Is this actually possible?)
  • The new exploration configuration changes the interaction id from a text input to multiple choice
  • The Ui/presenter receives the new exploration configuration and recreates the view
  • The student input is lost

@BenHenning
@adhiamboperes

@BenHenning
Copy link
Member

@masclot the app is currently local only, so fortunately none of the network scenarios you've outlined can occur. Regarding stability, the actual source of truth is whatever is provided via EphemeralState (or question), and yes it can change from underneath you since that's what happens during lesson navigation. No assumptions can be made about its state during a configuration change, though we could theoretically eliminate that particular bit of uncertainty by saving/restoring the latest ephemeral state while the new fragment ways for its DataProvider subscription to callback (I hadn't thought of doing something like this before--it might actually lead to smoother configuration changes).

Anything that's transient UI state and not part of the ephemeral data structures will be lost on configuration change. User input/interaction selections are the most obvious that are affected by this. FWIW if there's an easy way represent partial interaction answers in the ephemeral structures, that might actually be an easier way to go than trying to make saved instance state work. It also potentially leads to a cleaner long-term solution of saving these protos on disk for low-memory death scenarios, as well (but that's not part of the scope of this issue so it doesn't specifically need to be considered here).

@adhiamboperes adhiamboperes changed the title Handle configuration change using onSavedInstance Handle configuration change using onSavedInstance [Blocked] Jul 8, 2023
@adhiamboperes
Copy link
Collaborator

Unassigning @masclot because I feel this is blocked on technical direction. See #5008 (comment)

@adhiamboperes adhiamboperes removed the Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. label Jul 8, 2023
@adhiamboperes adhiamboperes added the Work: High It's not clear what the solution is. label Jul 8, 2023
adhiamboperes pushed a commit that referenced this issue Aug 2, 2024
…sing onSavedInstance. (#5458)

<!-- READ ME FIRST: Please fill in the explanation section below and
check off every point from the Essential Checklist! -->
## Explanation
Fix part of #4470. 
Fixes #4471.
Fixes #4474

This PR enables the retention of input when the device configuration
changes using onSavedInstance.

List of interactions covered in this PR:
1. FractionInputInteraction
2. NumericInputInteraction
3. TextInputInteraction
4. MathExpressionInteraction
5. RatioExpressionInteraction


List of interactions not covered in this PR:
1. Image Region selection interaction.
2. Drag and Drop interaction.

<!--
- Explain what your PR does. If this PR fixes an existing bug, please
include
- "Fixes #bugnum:" in the explanation so that GitHub can auto-close the
issue
  - when this PR is merged.
  -->

## Essential Checklist
<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [x] 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: ...".)
- [x] Any changes to
[scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets)
files have their rationale included in the PR explanation.
- [x] The PR follows the [style
guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
- [x] The PR does not contain any unnecessary code changes from Android
Studio
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
- [x] The PR is made from a branch that's **not** called "develop" and
is up-to-date with "develop".
- [x] The PR is **assigned** to the appropriate reviewers
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).

## For UI-specific PRs only
<!-- Delete these section if this PR does not include UI-related
changes. -->
If your PR includes UI-related changes, then:
- Add screenshots for portrait/landscape for both a tablet & phone of
the before & after UI changes
- For the screenshots above, include both English and pseudo-localized
(RTL) screenshots (see [RTL
guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines))
- Add a video showing the full UX flow with a screen reader enabled (see
[accessibility
guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide))
- For PRs introducing new UI elements or color changes, both light and
dark mode screenshots must be included
- Add a screenshot demonstrating that you ran affected Espresso tests
locally & that they're passing
Vishwajith-Shettigar added a commit to Vishwajith-Shettigar/oppia-android that referenced this issue Aug 2, 2024
…iguration change u…"

This reverts commit 7568bd3.
@adhiamboperes adhiamboperes changed the title Handle configuration change using onSavedInstance [Blocked] Handle configuration change using onSavedInstance Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug End user-perceivable behaviors which are not desirable. Impact: Low Low perceived user impact (e.g. edge cases). Work: High It's not clear what the solution is. Z-ibt Temporary label for Ben to keep track of issues he's triaged.
7 participants