Fix #4606: Language selector feature implementation #4762
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
KevinGitonga
changed the title
…language_selector_feature # Conflicts: # model/src/main/proto/BUILD.bazel
|
Hi @KevinGitonga, 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. |
oppiabot
bot
added
the
stale
oppiabot
bot
removed
the
stale
…some failing tests.
…troller. This test will not pass due to use of PersistenceCacheStore.
…for TranslationController.kt.
|
Hi @KevinGitonga, 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. |
oppiabot
bot
added
the
stale
oppiabot
bot
removed
the
stale
6 tasks
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.
|
@KevinGitonga PTAL KevinGitonga#5. Please review the changes and then merge it into your branch. If you have any concerns or see the need for additional changes, please merge the PR and then make those changes directly on the branch. Once the PR is in and you're happy with this PR, please assign it back to me. I will take one more pass, but I hope and expect that I can merge it on the next review pass. |
Finalize language selector
Thanks @BenHenning i have reviewed again and i would say the changes are LGTM according to me, assigning back the PR to you for a follow up pass since all ci checks are passing and the changes look good after manual review which you had similarly done on your side too. Thanks alot. |
BenHenning
changed the title
BenHenning
approved these changes
Jun 1, 2023
There was a problem hiding this comment.
Thanks @KevinGitonga! Just found 2 small things that I can address in a follow-up PR. I've noted them, but will go ahead and resolve them now.
Approving this and enabling auto-merge.
app/src/main/java/org/oppia/android/app/home/recentlyplayed/PromotedStoryViewModel.kt
Show resolved
Hide resolved
app/src/main/java/org/oppia/android/app/options/OptionControlsViewModel.kt
Show resolved
Hide resolved
|
Unassigning @BenHenning since they have already approved the PR. |
|
Assigning @rt4914 for code owner reviews. Thanks! |
Thanks @BenHenning good to have this ready for QA testing. |
BenHenning
added a commit
that referenced
this pull request
Jun 2, 2023
This commit: - Enables the language picker (being completed in #4762). - Bumps version codes (x2 since an extra re-release of 0.10 was necessary, and the updated version codes for that release weren't checked in).
This was referenced Jun 3, 2023
BenHenning
added a commit
that referenced
this pull request
Jun 6, 2023
## Explanation Fixes #52 This PR finishes up a bunch of separate tasks relating to the 0.11 release of the Oppia Android app. ### Release plans Note that a lot of the tasks addressed were completed out-of-band (hence the lack of tracking issues). These are needed to prepare for the following release changes: - A new version of the alpha app will be launched to provide a basis of testing spotlights - A new version of the Kenya alpha app will be launched (since those users are still in transitionary state to the beta version of the app until later this year) - A new version of the beta version of the app to include dark mode, the new language selector, and more ### Overview #### Language support changes - Arabic and Nigerian Pidgin (Naija) languages were added to production builds (Naija was added generally as it wasn't supported previously). Both languages were also added as possible audio languages. - Some basic tests were added to cover Arabic and Naija now being available, though some tests ended up being omitted due to the inherent complexity of testing these particular languages in certain cases (Arabic because it's RTL and Naija because Android doesn't actually support it natively--see below explanation). - Also added Nigeria as a language region. We don't yet have a strong grasp on the regions in which Arabic will be used, so that's being kept as a broad language for now. We may refine this in the future. - The translations for Nigerian Pidgin are added here as a replacement to #5009 since the original translations had some formatting issues that were identified by @adhiamboperes and subsequently fixed by our translation volunteers. Since we can't wait for another push to #5009, this PR is just introducing the strings directly. - ``AndroidLocaleFactory`` was largely rebuilt to make better use of code sharing, but its actual functionality needed to change due to the Naija support problems mentioned below. #### New feature: spotlights (alpha-only) - A new alpha module was added to allow the team to stage features specific to alpha builds, and spotlights were added as an initial use case for this new capability. - As part of testing spotlights I noticed that the background overlay was too dark (it was fully occluding the background), so I changed it to something that's dark but still semi-translucent. See the UI part of the description below for a before-and-after. #### New feature: in-app language selector (all users) - This PR enables the new language selector introduced by #4762 by default as part of addressing #52. - Some small fast-follow nits from #4762 were addressed (see #4762 (comment) and #4762 (comment)). #### Event system changes - Introduced a new script for decoding the compressed base 64 event strings that can be generated during user studies (see explanation below for details). This has some basic fault tolerance built into it so that truncated event strings can still be partially decoded. - A debug event property was added which essentially amounts to an event count since app startup that's attached to each event. While this won't help us identify events outright missing, it will help us identify events missing between received events. This is aimed to help an ongoing investigation that has found the high probability that events are being lost between user action and Firebase's storage layer. #### Infrastructure changes - The minor version of the app is being bumped (since this is a major beta release of the app). - Version codes were bumped for 2 releases (since one re-release of the beta version of the app was needed a few weeks back due to it expiring). - This PR also quiets the output when creating AABs since currently ~5k lines are outputted during the final bundle assembly steps and this output is never useful to the user unless there's a build pipeline failure. - This PR fixes an error in the resource filtering output that is provided at the very end of the AAB creation process (previously it was outputting the number of configurations being filtered not the number of actual resources being removed). #### Improved support for future user studies - The EnableLearnerStudyAnalytics has been split into 3 flags (2 new ones): one that controls just whether to attach the sensitive profile IDs to events, one that controls the fast in-lesson language switcher (and corresponding admin-controlled profile setting for it), and one that controls access to the analytics screen + admins being able to mark lessons as completed for profiles (this is gated on the existing flag). - The analytics screen flag has been default enabled for regular alpha builds of the app so that this channel can be better utilized for user studies. The user ID and in-lesson language switcher aspects of the old learner study analytics feature are being kept off in alpha builds, only the analytics screen & related behavior is being made available. ### Nigerian Pidgin support issues The Android system does not formally support Nigerian Pidgin (Naija) despite the fact that it has a recognized ISO 639-3 language code. Naija is a creole language which means it's mostly a derivative of English with vernacular adaptations (such as recreated words and grammar alterations). I think that because it's mostly a derivative language and is mainly used for casual speaking (vs. formal communications), there isn't as much of a desire on the Android side to formally support it as a UI language. Fortunately, the Oppia Android app's localization system already supports forcing the system to rendering app strings that are not natively supported. However, this results in several considerations and has turned up one new issue that needed addressing: - Custom language overriding only works by matching _both_ language and region, so the language strings needed to be moved to ``values-pcm-rNG`` (for Nigeria locking). This doesn't stop the language from being used outside Nigeria, it's just a limitation in Android's resource qualifier system when working with custom languages. - It's not yet clear how the change in the values structure will affect Translatewiki--some additional work might be needed there (which will become clear once this PR is merged & Translatewiki attempts another branch sync). - Custom languages don't have the benefit of guaranteeing rendering support, so the language's script largely comes to mind. Fortunately, as a derivative of English Naija uses Latin-based script so there are no concerns with language rendering compatibility. - An issue was found in the localization system when handling fallbacks. Since Naija is English-based, this PR has configured its fallback language to be English. This led to the system actually prioritizing English app string translations rather than forcing the Naija strings. This required the changes that can be seen in ``AndroidLocaleFactory``. ### ``DecodeUserStudyEventString`` utility A new utility was introduced to decode the compressed Base64 string of events logs that can be exported from devices being used for user studies (i.e. whose admins have access to the learner analytics screen). These logs represent the entirety of both pending and uploaded logs since the introduction of the feature (but only for devices that have the learner study enabled). The utility reads an input file containing a single instance of the string (and automatically strips newline and horizontal space formatting since the output from the app includes line-wrapping) and outputs it to one of three indicated formats: textproto, JSON, or Yaml. The script can be run in a local terminal from this branch (or ``develop`` once it's checked in) at the repository root by running: ```sh bazel run //scripts:decode_user_study_event_string -- $(pwd)/input.log $(pwd)/output.json ``` (where 'input.log' exists in the local repository root, and 'output.json' will be written to the local repository root). For use outside of Bazel, one can use a pre-built deployment Jar file by running: ```sh java -jar decode_user_study_event_string_deploy.jar input.log output.json ``` (Note that the error output for this command will be assuming Bazel is being used, so some adjustment may be needed to interpret CLI errors when using the deploy Jar). ### Third-party dependency updates Some new dependencies were needed for the new ``DecodeUserStudyEventString`` utility to make the implementation much simpler: - Protobuf's Java util (https://cloud.google.com/java/docs/reference/protobuf/latest/com.google.protobuf.util) for conversion from proto to json. - A converter from JSON to Yaml (since there's no direct proto-to-yaml conversion): https://github.com/xlate/yaml-json. - The Jakarta JSON API (required by the converter): https://jakarta.ee/specifications/platform/9/apidocs/jakarta/json/json. - The Jakarta JSON Parsson implementation (runtime dependency required by the converter): https://github.com/eclipse-ee4j/parsson. - Snakeyaml implementation (runtime dependency required by the converter): https://github.com/snakeyaml/snakeyaml. This required an update to maven_install.json, but no changes are needed in tracked Maven dependency licenses since these are not dependencies that are deployed with the app to end users. The above do result in an incidental update to Gson 2.8.6 (from the current 2.8.5 used). I don't expect that this actually will affect the release much since Gson isn't used broadly, and this is a minor update to Gson. ### Exemptions & test notes Two new files were exempted for tests: - ``PlatformParameterAlphaModule``: while we can test modules, I'm not actually keen on testing the platform parameter modules at this time because staging is generally messy at the moment with a lot of duplication across multiple modules, so the test complexity isn't small. A lot of the platform parameter system needs to be streamlined, but that's largely dependent on #1720 being finished. - ``DecodeUserStudyEventString`` while not particularly difficult to test, this is a special local analytics tool that will rarely be used and is very unlikely to break. Since I'm strapped for time on this PR, I'm forgoing adding tests for this utility. Regarding broad testing coverage, everything has had corresponding testing additions or changes where applicable. This PR might not have quite as much testing thoroughness as other PRs, but it does largely cover the core scenarios. ## 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 Spotlights changes before/after: | Old spotlights background | New spotlights background | |--------|--------| | https://github.com/oppia/oppia-android/assets/12983742/a8df7807-1bb6-45be-b53a-6d057f4a5931 | https://github.com/oppia/oppia-android/assets/12983742/e499f4b6-4609-4e58-871f-a09ebd09a80d | Video demonstrating the new functionality in the app, including alpha access to the learner analytics screen and support for both Nigerian Pidgin and Arabic (and via the in-app language selector): https://github.com/oppia/oppia-android/assets/12983742/c0a92393-a8a3-4634-af5b-2e7fc35ecd97 --------- Co-authored-by: translatewiki.net <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation
Fixes part of #52
Fixes #4606
Feature implementation for #4606
proto enum object.
representation.
See also KevinGitonga#5 for an additional explanation of more specific changes contained within this PR.
Essential Checklist
For UI-specific PRs only
If your PR includes UI-related changes, then:
Video demo of changes
language_selection_demo.mp4