Fast-follow items from Beta MR1 (mainly reviews and PR descriptions) #4567
Assignees
Labels
bug
End user-perceivable behaviors which are not desirable.
Impact: Low
Low perceived user impact (e.g. edge cases).
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.
Milestone
Comments
This was referenced Sep 7, 2022
BenHenning
added a commit
that referenced
this issue
Sep 7, 2022
…able to reset the admin pin (#4418) ## Explanation Fixes #4366 Fixes #3611 Fixes #4323 This PR introduces a data reset flow for cases when the user fails to remember their administrator pin. Since there's no remote profile support currently, the only option is to clear all profiles on the device. This is done using a two-step dialog since the action is permanent (at least for now--in the future we could consider making a local backup that could be restored, but that could become pretty complicated). While this isn't a great solution, it's better than not having an option at all since users can currently get permanently stuck (see #3611). While this doesn't directly fix #3611, it mitigates it by providing a proper in-app means of resetting app data. #4323 seems unrelated, but resetting the app required routing back to SplashActivity which means this PR needed to introduce a robustness mechanism to not set the app locale unless it wasn't already initialized (which bypasses the bug mentioned by that issue). Mechanically, the deletion process involves: - Deleting all profiles on the device - Updating the in-memory cache (which causes a few UI glitches) - "Restarting" the app by ending the activity and routing back to SplashActivity This replaces the existing "forgot PIN" flow for admins, and it technically does not reset non-profile data (such as the device installation ID, onboarding status, analytics/crash events, and some device-wide preferences). ## 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 is using the app's existing alert dialog theme, so it's not particularly interesting to show landscape/tablet screenshots as the UI isn't fundamentally different. The same is the case for special internationalization or accessibility concerns. In terms of a demonstration of the new flow, see the following video: https://user-images.githubusercontent.com/12983742/186572278-d0238e62-d013-419a-9c49-afe97cd10a5d.mp4 #4567 is tracking introducing tests, and the PR addressing it for these changes will include demonstrations of changed Espresso tests. Commits: * Add app data reset flow for resetting admin pin. * Fix broken test. * Update text & fix data reset issue.
This was referenced Sep 8, 2022
Merged
BenHenning
added a commit
that referenced
this issue
Sep 8, 2022
…rt to titles & descriptions (#4555) ## Explanation Fixes #4224 Fixes #4306 Fixes part of #1051 This PR introduces expanded support for translating content strings. In particular, previous support for written content translations only extended as far as exploration state content and interactions. Now, all topics, stories, chapters, exploration, and revision cards titles and descriptions now also support translations. When considered in conjunction with app string translations, this now means that nearly the entire app is translatable. ### Technical approach At a high-level, this PR expands the existing pattern for content translations for titles and descriptions for structures: explorations, topics, revision cards (subtopics), stories, and chapters (story nodes). This required changing existing structures (and corresponding test assets) to use ``SubtitledHtml`` instead of raw strings for their titles and descriptions, and to now include written translation tables. New ephemeral structures have been introduced to compute the translations that can be consumed by the UI to ensure the correct string is displayed. Note that the test JSON assets have not been updated since it was a bit easier to just translate the title/description strings into ``SubtitledHtml``s during parsing time (which seems like a reasonable stopgap since the JSON loading pipeline is temporary and only developer-facing). Due to new ephemeral structures needing to be computed in a bunch of places, profile ID now needs to be passed in more places. This has led to some minor refactoring to push more UI components over to passing around the ``ProfileId`` structure rather than the internal integer ID. This PR also fixes part of #1051 by migrating more fragment presenters over to injecting their view models rather than retrieving them from Jetpack's view model provider (which means each of these changed presenters no longer expose potential memory leaks in the corresponding view model). ### Caveats This PR doesn't technically directly address #4224 and #4306, but it does indirectly. The new structures are linked to the assets download script which has been subsequently updated to cross-reference the translations for topics, stories, chapters, and subtopics from Oppia web's repository (which has temporary hardcoded translations for many of the titles and descriptions). This means that: - Not everything will be translated despite the app now supporting it (example: revision card content isn't available to cross-reference). - Many translation languages are missing (the team is currently focusing on Brazilian Portuguese ahead of the upcoming Beta MR1 app launch). - Longer term, Oppia web will provide the translations directly with the structures like it does for explorations (this is an ongoing backend project). Another caveat is that this PR further reveals the specific need to be able to compute a ``DataProvider`` based on the result of another. This is a limitation that the current ``DataProviders`` utility methods cannot handle, and the workaround is to call ``retrieveData()`` on the computed ``DataProvider`` directly which has updating inconsistency issues. Some of the providers modified as part of this PR should be updated to leverage a new dynamic transform method (see #4564 for the tracking issue). This was unfortunately dropped from the project due to limited time, but #4484 is the WIP PR. ### Test changes (or lack thereof) Due to this PR being especially high priority, tests have been omitted from this PR in favor of merging it faster. Instead, tests will be added in a follow-up PR (#4567 is tracking to make sure that this happens). The changes have been manually tested, and will be tested more prior to many users interacting with the new functionality. ## 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 While this PR is highly end user-facing, for the sake of time screenshots and video demonstrations have been omitted from the PR. They will be added at a later date (after the PR is merged), and #4567 is tracking the work item to make sure that this happens. Commits: * Create dedicated alpha application component. This simplifies application component management significantly and allows individual build flavors to have their own unique module lists. * Add beta & GA update notices. This also introduces dedicated beta & GA build flavors which is a necessary prerequisite. It also introduces an extra beta, alpha, and dev mode labels for the splash screen (the latter 2 were extra) with 2 second minimum wait timers for beta and alpha to ensure they are seen. A 5-second safety timer was added to ensure the splash screen can always be passed even if something goes wrong at the domain level (since there are now quite a few moving pieces to determine the user's current onboarding state). * Add build tests for the new beta & GA flavors. * Initial support for dynamic data provider xforms. * Stabilization improvements. To ensure that the new dynamic DataProvider can't result in a soft lock if in-memory cache-backed providers (such as PersistentCacheStore) circularly reference each other (since they depend on a single thread), InMemoryBlockingCache was updated to use the standard background in conjunction with an actor channel. This ensures the lockless synchronization mechanism can continue without cross-cache contention occurring due to using a shared test. This commit also subsequently updates all blocking caches to use the same actor model, instead, and outright removes the blocking dispatcher to ensure these situations can never occur in the future. * Update codebase to Kotlin 1.6. * Add support for build-specific asset handling. * Fix build & Kotlin warnings (that are now errors). One test needed to be ignored due to the Kotlin coroutines update, so follow-up work will be needed to address this. Note that this commit also restricts access to the Kotlin reflection library (since it shouldn't be used broadly). This commit also updates Gradle to use Kotlin 1.6 (though only the app build has been verified). Proguard building support hasn't yet been verified. * Fix broken test per earlier changes. * Fix general broken tests & builds. Tests broken due to changes to the app startup experience haven't yet been fixed. * Lint fixes. * Introduce coordinated executors for tests. This commit replaces coroutine dispatchers with lower-level executors that perform the actual state management and coordination for all multi-threaded task handling in the app. This is actually a major simplification as it allows for: - Proper threading resource sharing between the app and its libraries (such as OkHttp) - Proper thread synchronization between executors and coroutines in tests without leveraging the extremely complicated CoroutineExecutorService - Less custom coroutine infrastructure which can be difficult to maintain * Fix broken tests & lint issues. * Post-merge & static check fixes. Does not include new tests that need to be added, or lint fixes. * Lots of test fixes. This also includes fixing some core issues in AsyncDataSubscriptionManager and InMemoryBlockingCache that went previously unnoticed. More tests still need to be added yet. * First part of new tests. (This is an in-progress commit that's needed so that I can transfer changes to a different machine). * First part of adding tests for GA notices. There's a bunch left to do here, this is mainly needed so that I can transfer changes to a different machine. * Update TransformAndroidManifestTest.kt Correct typos. * Fix tests & static checks. This also removes temporary debug code and TODOs, and finishes the tests for SplashActivity. * Post-merge fixes. * Test fixes. * Fix Gradle test. * Initial commit for expanded xlations support. This commit introduces the base changes needed to introduce translations support for all other content strings in the app outside core lessons. * Undo executors, Kotlin upgrade, and dynamic xform. This undoes the work of PRs #4484 and others not yet created in order to isolate the expanded translations support directly off of the develop branch to expedite getting it finished for the upcoming beta release. * Part 1 of post-"rebase" fixes. * Some post-merge fixes. * Undo changes from #4436. Isolating domain assets shouldn't be necessary for the expanded translations support, and it will be easier to manage this PR with fewer extra things being added to it. * Fix various tests & update test assets. Also, remove temporary TODOs. * Static fixes. * Add some string resource checks/tools. Also, fixes major performance issue with all file-based CI checks. * Ensure newline consistency in translated strings. Also, fix reporting in new validation check script. * Add tests & fix static checks. * Fix broken tests. * Update version codes & pt-BR strings. The strings were manually pulled Translatewiki. * Follow-up adjustments after self-review. * Undo changes from #4565. This commit allows the PR to be "rebased" back onto develop so that it can be prepped for merging.
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.
This was referenced Sep 9, 2022
BenHenning
added a commit
that referenced
this issue
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 - [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 section will be filled in post-merge (#4567 is tracking ensuring that the description is finished later). Commits: * Add pt-Br as option for default voiceover audio. This also makes some infrastructural improvements, a small UX improvement (removing the 'No Audio' option), and fixed a tablet bug in the options menu. * Update app/src/main/java/org/oppia/android/app/translation/AppLanguageResourceHandler.kt
6 tasks
BenHenning
added a commit
that referenced
this issue
Sep 9, 2022
## Explanation Fixes #4413 This PR updates the placeholder launcher icon with something that better fits the adaptive icon expectations. Note that this PR is removing the rounded icon per the advice of the adaptive icon support page (that it should be preferred not to be used unless necessary), and it's removing the play store icon since it shouldn't be needed. It's also inverting the colors of the icon since that seems to be less visually jarring for the circle-in-circle icon cases. Note that this PR is introducing more binary files to the history, but they're small. Longer term we can look into rendering these PNGs during build time, but it's not an option right now. ## 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 section will be filled in post-merge (#4567 is tracking ensuring that the description is finished later).
6 tasks
BenHenning
added
issue_type_infrastructure
Impact: Low
BenHenning
moved this to Needs Triage
in [Team] Developer Workflow & Infrastructure - Android
seanlip
added
enhancement
MohitGupta121
moved this from Todo
to In Progress
in [Team] Developer Workflow & Infrastructure - Android
MohitGupta121
added
the
Work: Medium
seanlip
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
PRs that need to be post-merge reviewed:
PRs that have additional work items to complete:
Note: Some follow-up tests also need to be written. These are detailed in #5035.
The text was updated successfully, but these errors were encountered: