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

Fix #4285 and #4394, part of #4437: Improve in-lesson reading text size support #4411

Merged
merged 8 commits into from
Sep 8, 2022

Conversation

BenHenning
Copy link
Member

@BenHenning BenHenning commented Jul 1, 2022

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 Fast-follow items from Beta MR1 (mainly reviews and PR descriptions) #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 Fixes #3095, #2824: Use protos with intent extras in RecentlyPlayedActivity #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

  • 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).

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.

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.
@oppiabot
Copy link

oppiabot bot commented Jul 8, 2022

Hi @BenHenning, 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!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Jul 8, 2022
@oppiabot oppiabot bot closed this Jul 15, 2022
@BenHenning BenHenning reopened this Jul 16, 2022
@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Jul 16, 2022
@oppiabot
Copy link

oppiabot bot commented Jul 23, 2022

Hi @BenHenning, 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!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Jul 23, 2022
@oppiabot oppiabot bot closed this Jul 30, 2022
@BenHenning BenHenning reopened this Aug 2, 2022
@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Aug 2, 2022
@oppiabot
Copy link

oppiabot bot commented Aug 9, 2022

Hi @BenHenning, 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!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Aug 9, 2022
@oppiabot oppiabot bot closed this Aug 16, 2022
@BenHenning BenHenning reopened this Aug 19, 2022
@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Aug 19, 2022
@oppiabot
Copy link

oppiabot bot commented Aug 26, 2022

Hi @BenHenning, 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!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Aug 26, 2022
@oppiabot oppiabot bot closed this Sep 2, 2022
Conflicts:
	model/src/main/proto/arguments.proto
@BenHenning
Copy link
Member Author

BenHenning commented Sep 5, 2022

Remaining TODOs before merge:

  • Finish description
  • Verify everything still works correctly
  • Review code
  • Ensure CI checks are passing

@BenHenning BenHenning reopened this Sep 5, 2022
@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Sep 5, 2022
Conflicts:
	app/src/main/java/org/oppia/android/app/player/exploration/ExplorationActivityPresenter.kt
	app/src/main/java/org/oppia/android/app/resumelesson/ResumeLessonFragmentPresenter.kt
	app/src/main/java/org/oppia/android/app/story/storyitemviewmodel/StoryChapterSummaryViewModel.kt
@BenHenning BenHenning self-assigned this Sep 8, 2022
@BenHenning BenHenning changed the title Fix #4285 and #4394: Improve in-lesson reading text size support Fix #4285 and #4394, part of #4437: Improve in-lesson reading text size support Sep 8, 2022
@BenHenning BenHenning marked this pull request as ready for review September 8, 2022 11:29
@BenHenning BenHenning requested a review from rt4914 as a code owner September 8, 2022 11:29
@BenHenning
Copy link
Member Author

Since this PR is part of broader urgent work for the Beta MR1 release, I'm force-merging this without review. I've self-reviewed it and found no major issues. Furthermore, #4567 is tracking ensuring that this does get reviewed by someone else later after it's been merged.

@BenHenning BenHenning merged commit a9e132a into develop Sep 8, 2022
@BenHenning BenHenning deleted the fix-reading-text-resizing branch September 8, 2022 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Text doesn't change when using non-English languages Text size does not change in-exploration
1 participant