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 #3821: Ensure Portuguese can be a default audio language #4575

Merged

Conversation

BenHenning
Copy link
Sponsor Member

@BenHenning BenHenning commented Sep 9, 2022

Explanation

Fixes #3821

This PR introduces the following end-user visible changes:

  • It adds Brazilian Portuguese as an option when selecting a default audio language.
  • It removes the 'No Audio' option from the list since it's confusing and presently defaults to 'English' when playing audio voiceovers, anyway.
  • It changes the display names of the default audio languages to be localized to their own locale to make the languages easier to read by native speakers.
  • It fixes a bug in the tablet version of OptionsActivity wherein switching between audio & reading text would result in fragment stacking (i.e. you could see the bottom of the longer fragment when on the shorter one). This entailed changing fragment 'adds' to 'replaces' for the options fragment transactions.

Technical details:

  • This PR migrates the audio language pattern from using a string over to using the AudioLanguage proto enum, similar to the recent change for ReadingTextSize. This was preferred since changing everywhere to support a localized name of each language seemed non-ideal.
  • A few models & listeners needed to be split between app & audio languages due to the previous approaching having a lot of codesharing between the two that's no longer applicable with strong types.

Exemptions:

  • AppLanguageResourceHandler is now allowed to use Locale directly so that it can produce localized display names for each audio language. In the future, this could be moved to MachineLocale maybe.
  • AppLanguageResourceHandlerTest is now allowed to use parameterized tests. The new parameterized tests that were added are technically violating the principle of now parameterizing the expected output of the test, but these are medium-term tests that will go away when the new language selector is added.
  • The test file exemptions aren't technically "new" in the sense that they're just preserving the existing exemptions except that the exemptions are now split in the same way as the new app/audio versions of the classes.

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 section will be filled in post-merge (#4567 is tracking ensuring that the description is finished later).

This also makes some infrastructural improvements, a small UX
improvement (removing the 'No Audio' option), and fixed a tablet bug in
the options menu.
@BenHenning
Copy link
Sponsor Member Author

I've self-reviewed the code and haven't found any major points of concern.

@BenHenning
Copy link
Sponsor 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 7ff4874 into develop Sep 9, 2022
@BenHenning BenHenning deleted the ensure-portuguese-can-be-a-default-audio-language branch September 9, 2022 13:30
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 voiceovers in English for Portuguese language
1 participant