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

Finalize language selector #5

Merged
merged 5 commits into from
Jun 1, 2023
Merged

Finalize language selector #5

merged 5 commits into from
Jun 1, 2023

Conversation

BenHenning
Copy link

@BenHenning BenHenning commented Jun 1, 2023 •
edited
Loading

Explanation

These changes intend to wrap up the remainder of work needed for oppia#4762 to be merged, including:

  1. Addressing all remaining KDoc and formatting concerns.
  2. Finalizing the activity & app mixin tests.
  3. Performing some manual testing to make sure everything is working correctly.
  4. Clean up some of the view model logic in OptionControlsViewModel.

Some additional changes ended up happening as part of the above:

  • Per (1), I went ahead and added a bunch of regex checks & tests to both have confidence that all styling issues were fixed, and that they can't be reintroduced by anyone else in the future. This required cleaning up a number of existing problems in KDocs elsewhere in the codebase.
  • Per (2), I needed to update ProfileTestHelper to have more robust lifecycle management which, in turn, required some small changes to downstream tests. AppLanguageWatcherMixinTest also needed reworking in order to set up its TestActivity at the correct time (after profile creation). TestActivity needed to be updated to disable its own AppLanguageWatcherMixin so that AppLanguageWatcherMixinTest could test its mixin without interference.
  • Per (3), I noticed there was a bug when using the language selector. It wouldn't actually save the selected language when back navigating using the toolbar, only when using the system back button (and, in fact, would always revert back to English). Upon digging, I realized this was because of an inconsistency in the activity result handling (the toolbar bit wasn't ever updated so a default result proto was the "result," and default moves the app back to using system language, or English in my case). Due to OptionControlsViewModel now depending on TranslationController directly, this result logic isn't even needed anymore so I went ahead and removed all of it. This is a cleanup that also results in back navigation working correctly for both the navigation bar and system back buttons.

I also noticed in a follow-up commit that tablet mode auto-selection for the reading text fragment stopped working. I think this incidentally worked before due to a very specific timing situation that occurred when fragment initialization raced against the dual LiveData observation that was happening. I removed this fragile mechanism which now breaks with the simpler single LiveData version of OptionControlsViewModel and replaced it with a more robust flow that happens during fragment creation. This could be further improved by accounting for the default selected fragment, but it's a good place to start.

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

N/A for time--any relevant changes should be included directly in oppia#4762.

BenHenning added 4 commits May 31, 2023 13:35
@BenHenning
Bunch of KDoc fixes.
e1993e4 
This addresses a few remaining areas after a follow-up review of oppia#4762.
It also systematically ensures a variety of KDocs issues can't be
introduced in the future by adding 8 new regex checks + tests (with a
bunch of style fixes throughout the codebase). While a lot of this is
unrelated to the PR at hand, it's nice to just get this done in a PR
that's about to get merged.

Some other style things were done in this commit, as well.
@BenHenning
Merge branch 'language_selector_feature' into finalize-language-selector
9b3d39c 
Conflicts:
	scripts/assets/file_content_validation_checks.textproto
@BenHenning
More fixes and finalization.
18643ea 
Specifically, this commit:
- Finalizes tests for the mixin classes. This includes re-adding app
  mixin tests that were previously removed, and fixing the cases where
  they weren't working as expected.
- As part of the above, some revision was done in ProfileTestHelper to
  facilitate more robust managing of profile creation and login state.
- Fixes a bug with back navigation not working in the language selector.
- Removes (now unnecessary) code in the language selector.
- Cleans up OptionControlsViewModel and its DataProvider/LiveData logic
  a bit.
@BenHenning
Re-disable language selection feature flag.
2641bad
@BenHenning
Copy link
Author

NB: Will assign the PR once CI is verified as passing (since I will otherwise make follow-up changes before assignment).

@BenHenning BenHenning marked this pull request as draft June 1, 2023 00:45
@BenHenning
Fix broken tests & issues.
a723e44 
Specifically:
- Default options screen selection in tablet mode stopped working due to
  what seems to be a timing order difference. This establishes a more
  robust mechanism to initialize the state.
- Some other small changes/cleanups.
@BenHenning
Copy link
Author

PTAL @KevinGitonga.

@BenHenning BenHenning mentioned this pull request Jun 1, 2023
Merged
6 tasks
KevinGitonga
KevinGitonga approved these changes Jun 1, 2023
View reviewed changes
Copy link
Owner

@KevinGitonga KevinGitonga left a comment

Choose a reason for hiding this comment

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

Thanks Ben approving this into language_selector_feature_branch.

@BenHenning BenHenning marked this pull request as ready for review June 1, 2023 09:29
@KevinGitonga KevinGitonga merged commit 27e0ee4 into KevinGitonga:language_selector_feature Jun 1, 2023
@BenHenning BenHenning deleted the finalize-language-selector branch June 1, 2023 21:18
BenHenning added a commit to oppia/oppia-android that referenced this pull request Jun 1, 2023
@KevinGitonga @BenHenning
Fix #4606: Language selector feature implementation (#4762)
81a95b7 
## Explanation

Fixes part of #52
Fixes #4606

 Feature implementation for #4606
- This PR introduces the ability to change App level language through
the Options setting. It has involved below changes.
1. Refactored the codebase to accomodate feature changes by changing
from use of string variable to OppiaLanguage
            proto enum object.
2. Add Changes to allow display of languages loaded from proto in a
human readable form similar to AudioLanguage
           representation. 
- When fully functional this PR should allow users to modify app
language in App settings regardless of the language setting of their
device. PersistentCache is used to locally persist language which has
been selected by a user.

See also KevinGitonga#5 for an
additional explanation of more specific changes contained within this
PR.

## 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))
- Add a screenshot demonstrating that you ran affected Espresso tests
locally & that they're passing

## Video demo of changes

https://user-images.githubusercontent.com/20886444/222683790-7b2d4fa8-7068-4774-8d1a-0494d4f9adf0.mp4

---------

Co-authored-by: Ben Henning <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KevinGitonga KevinGitonga KevinGitonga approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@BenHenning @KevinGitonga