[Cleanup] Migrate all fragment arguments & activity parameters to proto values #4437
Assignees
Labels
enhancement
End user-perceivable enhancements.
Impact: Low
Low perceived user impact (e.g. edge cases).
Issue: Needs Break-down
Indicates that an issue is too large and should be broken into smaller chunks.
Issue: Needs Clarification
Indicates that an issue needs more detail in order to be able to be acted upon.
Work: Medium
The means to find the solution is clear, but it isn't at good-first-issue level yet.
Z-ibt
Temporary label for Ben to keep track of issues he's triaged.
Comments
Broppia
added
issue_type_infrastructure
Impact: Low
6 tasks
BenHenning
added a commit
that referenced
this issue
Sep 8, 2022
…ze support (#4411) ## Explanation Fixes #4285 Fixes #4394 Fixes part of #4437 This PR fixes two specific bugs related to in-lesson reading text support: 1. That changing the reading support requires leaving & reentering the lesson in order for it to apply. 2. That selecting a reading size when on a non-English language simply doesn't work if the reading text sizes have been translated. Issue (1) comes from the fact that, while ``FontScaleConfigurationUtil`` recomputes the scaled density for layouts, an actual recreation seems necessary for sp-related fields to actually recompute correctly. Simply adding a ``recreate()`` call in ``ExplorationFragmentPresenter`` seems to address the issue fully. Issue (2) comes from the fact that reading text sizes are managed by their user-readable text value. This inherently doesn't work for non-English languages since the string content will be different. The fix here is to introduce non-string, strong typing (in this case, a proto enum) to represent text sizes. However, since the text size needs to be passed through some fragment arguments and activity intents, various fragments and activities needed to be updated to use protos (fixing part of #4437). This required broader changes than solving issue (1) alone. Some technical specifics: - The proto changes also required introducing a new ``ParentScreen`` proto enum for exploration activity to replace the existing backflow integer mechanism. This enum is actually shared by other activities that interact with exploration activity (such as recently played) which creates a strong semantic connection than just passing around an integer. - ``ReadingTextSizeActivity`` is providing a bundle as its result so that the calling activity can receive which size was set. I believe this is the first time we've used such a pattern in the codebase, and it may be nice to use more of it when the extra hop to receive an asynchronous result from the domain layer changing isn't worth the wait (e.g. due to user-perceived jank or transient inconsistencies that may arise during that small gap of time). Notes on tests/exemptions: - A lot of the behaviors changed in this PR are verified through existing reading text size selection tests. - More thorough testing is needed not only for verifying the specific issues fixed in this PR, but also to ensure the general reading text size selection flow works exactly as needed. These tests have been intentionally left out of this PR as a time saving mechanism, but #4567 is tracking adding them in the longer term (after the PR has been merged). - A new ``hasProtoExtra`` Espresso matcher was added in a few of the tests that was mainly inspired by discussions in #4511. This matcher has some room for improvement when it comes to error reporting (though Espresso makes this generally difficult to do well), but it provides a way to verify proto payloads passed into intents. ## Essential Checklist - [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 This PR doesn't change existing UIs, but does change UX flows (mainly for sighted users, so the accessibility cases aren't particularly interesting). Since there are no new layout changes, RTL and dark mode considerations are also not relevant. See the following videos for the (now working) UX flows: Text size correctly applies when navigating back to the exploration from the options menu, from a checkpoint, and after logging back into the profile: https://user-images.githubusercontent.com/12983742/189110927-9dd1e943-148e-438a-8d8d-b85294dc812f.mp4 Text size correctly applies when using a non-English language (such as Brazilian Portuguese): https://user-images.githubusercontent.com/12983742/189111036-68e233a1-b11f-4a9a-8366-76a5ae6fbd57.mp4 Espresso tests haven't changed as part of this PR (so far as I know), so there are no confirmation screenshots to include. Commits: * Improve reading text size support. Specifically: - This commit fixes reading text size not changing when selected for non-English languages. - This commit fixes reading text changes not actually changing when navigating back to the exploration. * Fix broken tests & static checks. * Small style spacing fix. * Post-merge fix.
BenHenning
moved this to Needs Triage
in [Team] Developer Workflow & Infrastructure - Android
BenHenning
added
Issue: Needs Clarification
seanlip
added
enhancement
|
@BenHenning Could you please add a description for this issue? |
MohitGupta121
added
the
Work: Medium
MohitGupta121
moved this from Todo
to In Progress
in [Team] Developer Workflow & Infrastructure - Android
seanlip
changed the title
|
Thanks @Vishwajith-Shettigar. You're correct that #4986 addresses this issue plus more, so closing this in favor of that one. |
github-project-automation
bot
moved this from In Progress
to Done
in [Team] Developer Workflow & Infrastructure - Android
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
No description provided.
The text was updated successfully, but these errors were encountered: