Fix #4519, #4522: Add concept card link support to hints and concept cards #4529
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
This adds support for opening concept cards from hints, solutions, and other concept cards.
|
Remaining tasks before force-merge:
|
|
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. |
oppiabot
bot
added
the
stale
|
Replaced by #4846. |
6 tasks
BenHenning
added a commit
that referenced
this pull request
Mar 10, 2023
, #4856: Assorted alpha MR6 fixes (#4846) ## Explanation Fixes #4833 Fixes #4834 Fixes #4835 Fixes #4838 Fixes #1050 Fixes #4519 Fixes #4522 Fixes #4837 Fixes #4836 Fixes #4855 Fixes #4856 This PR introduces a variety of fixes and user quality-of-life improvements in preparation for the start of the upcoming multi-week user research project in Kenya. This PR covers 9 different issues in one shot to keep the PR management simpler due to the time sensitive nature of these changes, so it's on the larger side. Please note that: - These changes have been released to a user study specific release track since the research project is currently ongoing. - Many of these changes will impact users outside of the study, as detailed below. The specifics of the PR will be split across the different issues being addressed, in categories below. ### Improvements to solutions #4833 needed to be addressed because the previous terminology ("reveal") has been found to be confusing to learners who might not yet have a strong grasp on English. This string update will, unfortunately, not be translated for the upcoming study (though the study predominantly uses English). This change affects all users of the app. #1050 is a long-standing issue wherein solution answers would only show up correctly if they were either text or a fraction. This PR fixes the issue by introducing generic answer support, expanding it to include: numeric input, numeric expression, algebraic expression, algebraic equation, and ratio expression. The only missing interaction that web supports is drag & drop, but this has been found to not be needed for the current shipped list of topics (as now enforced by an update to the content import pipeline) and introducing support for it would nontrivially increase the complexity of that side of the PR. This is something that, like other interactions, will be added in the future once needed. This fix will affect all users of the app. ### Improvements to voiceover telemetry #4834 was addressed by introducing a new event to track explicit voiceover pauses (i.e. those that require clicking the 'pause' button and not the ones that are automatic, such as autoplay ending or the user navigating away from the lesson player). Because of the nature of the 'play voiceover' event (that it is logged both for clicking the 'play' button and when expanding the audio bar due to autoplay), it's possible for the play & pause event counts to not be equal. The intent behind these events is to track explicit user actions, not the occurrences of voiceovers playing/not playing. This event will be logged for all users, though sensitive identifiers will only be logged if the user study feature is enabled. #4835 was addressed by updating both the existing 'play voiceover' and new 'pause vocieover' events to include the lesson player-reported language code (that is, the language code originating from Oppia web). We may change this to be something more strongly ensured (since Oppia Android is a bit more careful in its language code management due to Android complexities), but the code should be generally usable as-is for data purposes as it's implemented. This change will affect telemetry for all users of the app. ### Improvements to concept card availability #4519 and #4522 were addressed by adding support for opening concept cards both in concept cards themselves, and from hints. As of this PR, just about all cases of lesson HTML rendering should now include support for linkifying and opening concept card references. Since both hints and concept cards are dialogs, the behavior is that the newly opened concept card dialog "stacks" on top of the existing, open dialog (i.e. closing the concept card will show the original dialog from which it was opened). This change affects all users of the app. ### Miscellaneous language improvements #4837 was addressed to clean up a variety of extraneous spaces & newlines that were unintentionally added by translators for Swahili strings. These have a direct user benefit but are otherwise innocuous, non-semantic changes. This fix technically affects all users of the app, but only those who use the Swahili translations will actually interact with the changed strings. ### Study-specific improvements #4838 was addressed by introducing a new link in the profile edit flow for administrators to access the previously developer-only flow for manually marking certain chapters as completed. This newly exposed mechanism allows study facilitators to set up profiles so that learners can begin at a specific point (which is particularly helpful if the data on the device ever needs to be reset, such as in the case where the administrator forgot their PIN). This change will only affect user study administrators as it's locked both behind the admin account and the learner study flag. The functionality *will* be available on production-optimized builds (that is, it won't be restricted to developer-only build flavors of the app). #4836 was addressed by introducing a floating button within the lesson player for lesson cards whose content have available Swahili translations to quickly switch the content language between English and Swahili. This doesn't change app strings, only content strings, and only temporarily for the lifetime of that specific play session (that is, if they exit the lesson & return immediately they will still be in the switched language until the app itself restarts). This button only appears if the study feature is enabled and has been enabled for that user by the administrator profile (via a new setting on the "Edit Profile" screen). This button also has its own new event to track its usage. A "share IDs" button was added to make it easier to share the device & learner IDs by collating the IDs into a single blob of text that can be shared to any app that accepts text intents. This is meant to help reduce potential error by study coordinators. ### Content display improvements #4855 was addressed by changing a lot of how custom LI/OL tags are handled. Specifically, Android will automatically shift the leading span of cascading bullets/numbers, but the previous implementation was using the wrong value for each nested span's parent's leading margin (which is needed to adjust what leading margin to use for the child span). This was fixed by introducing more thorough tracking (which also included a bit of robustness in ensuring larger numbers can be aligned correctly). This doesn't quite match HTML rendering, but it's much closer. Bullet lists were also updated to use squares in addition to filled and open circles (for parity with web HTML), and now uses sizes & spacing that are a bit cleaner to see. There's still more work to improve edge cases for bullet list rendering (see #4859). #4856 was addressed by introducing a separate SVG "render size" that, unlike the intrinsic size, is based on the image filename. Since the image filename is given in pixels, there needs to be a translation to dp to ensure consistent viewing across devices. This computation has been implemented with three calculation passes: 1. Convert the image's target pixel size to "Oppia independent pixels" by using @seanlip's monitor display density as the basis. 2. Convert "Oppia independent pixels" into Android density-independent pixels. 3. Adjust the final value using a scalar (since the physical size of an image on @seanlip's display is too large for a small mobile screen, so all images are consistently further scaled down). There's still more work to improve image rendering (see #4093). ### General technical details Design choices & other details of note: - The chapter completion flow was updated to support configuring a confirmation dialog (since the action is permanent). This confirmation only shows for the admin edit profile flow (the developer options menu version still does not show a confirmation). - The hints & solution data flow was completely redone in the app layer since much of what it was doing before isn't actually necessary with ``HelpIndex`` as it greatly reduces the complexity of managing UI-side hint state. Since solutions were being updated, anyway, this unrelated complexity has been reduced (and other issues like directly constructing an injectable view model were addressed). - Solution titles were previously set to their content ID (similar to hints before this was recently fixed), so this was addressed by using a new "Solution" string. - Displayed solution answers didn't correctly differentiate between exclusive & example answers. This has been corrected in text (along with how the app generally concatenates the text as the previous approach wasn't very RTL friendly). - Generalizing solutions was done by leveraging an InteractionObject for Solution's correct_answer field. No migration is being done here since assets are replaced across releases. - There are a lot of changes to both json and textproto assets to accommodate the new correct_answer structure, among other various changes. A few of these were done manually, but most were generated (including new Swahili translations to allow local testing of the in-lesson language switch button). - A bug was noticed & fixed wherein states with only a solution (and no hints) would never show the solution. - Due to the concatenation strategy for solution correct answer text (see a few points above), the horizontal textual alignment was a bit off. The new approach addresses this (though LaTeX is still a bit off, but it's generally off everywhere in the app). - To make the new in-lesson switch language button a bit more consistent and aesthetically pleasing, this PR repurposes the "Start over" button that currently shows on the checkpoint continue screen by making it a generic "secondary button" style (with corresponding colors) and using that both for the start over button and this new secondary button. Per the screenshots below, this seems to work well. The secondary button may also be used elsewhere in the app in the future. - Note that some of the work of this PR originates from #4529, so it can be considered a replacement for that PR. - Only the first test topic was updated to include Swahili translations to help decrease the size of the PR (plus, it's convenient to have non-Swahili lessons to verify cases when the switch button does **not** show up). - The PR fixes an incidental issue with the hints & solution dialog whereby clicking any part of an expanded hint/solution card would collapse it (rather than just the header or collapse icon). This would make clicking links more difficult, and is just generally a likely unexpected behavior from users. - The new event log & language code property have been manually verified as logging correctly using the developer Firebase project with debug view enabled. - A new event was added to track when users leave revision cards. - A bug was found & fixed that led to tab switches in the topic activity not having their corresponding events properly logged. - All events have been updated to now include app string, content string, and voiceover languages per-profile for that event (which led to a fairly large amount of complexity around providing access to those languages at a low level like ``AnalyticsController``). - A bug was found & fixed around session ID management: previously, the session ID was supposed to be reset upon a new lesson being started but it actually wasn't behaving this way. Per the new needs of analytics, this behavior was changed & fixed to reset upon profile login (so that behaviors for a single user session can be properly correlated). Noteworthy test & exemption changes: - ``StateRetrieverTest`` is largely not updated since all of the JSON changes in this PR only affect local development (all Bazel tests & production builds use textproto or binary proto versions of lessons). The tests locally passing plus some basic local ad hoc testing is considered sufficient for the JSON loading pipeline as it will, eventually, go away in favor of textproto (once Gradle is removed since textproto -> binary proto conversion is not easily implemented in the Gradle build environment). - Some of the question test suites weren't updated since questions aren't currently enabled, and much of the question pipeline is already covered under state fragment tests. It's likely that the testing matrix for questions will need to be largely refined once they're ready to be enabled, so some of this work was skipped in favor of expediency. - Some tests in StateFragmentTest & StateFragmentLocalTest are disabled and can't easily be verified because they would only pass on Espresso and Bazel (due to Robolectric having limitations in language resource handling & Gradle builds not supporting the app's language configurations). Since Espresso tests do not yet work in Bazel, these tests will be verified later (though they have been manually verified as part of development). - A bunch of KDoc validity exemptions were removed since those classes are now, as of this PR, fully documented. - The test_file_exemptions updates are due to HintsViewModel being split into two new view models, and the general convention on the team to not directly test view models. ## 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 ### Screenshots showing some of the new functionality | **Scenario** | LTR | LTR | LTR | LTR | RTL | RTL | RTL | RTL | |----------------------------------------------|----------|-----------|----------|-----------|----------|-----------|----------|-----------| | | Handset | Handset | Tablet | Tablet | Handset | Handset | Tablet | Tablet | | | Portrait | Landscape | Portrait | Landscape | Portrait | Landscape | Portrait | Landscape | | Fractions solution (exclusive) |  |  | Skipped until requested | Skipped until requested |  |  | Skipped until requested | Skipped until requested | | Numeric solution (exclusive) |  |  | Skipped until requested | Skipped until requested |  |  | Skipped until requested | Skipped until requested | | Ratio solution (not exclusive) |  |  | Skipped until requested | Skipped until requested |  |  | Skipped until requested | Skipped until requested | | Text solution (exclusive) |  |  | Skipped until requested | Skipped until requested |  |  | Skipped until requested | Skipped until requested | | Numeric expression solution (not exclusive) |  |  | Skipped until requested | Skipped until requested |  |  | Skipped until requested | Skipped until requested | | Algebraic expression solution (not exclusive) |  |  | Skipped until requested | Skipped until requested |  |  | Skipped until requested | Skipped until requested | | Math equation solution (not exclusive) |  |  | Skipped until requested | Skipped until requested |  |  | Skipped until requested | Skipped until requested | | Switch English to Swahili |  |  | Skipped until requested | Skipped until requested |  |  | Skipped until requested | Skipped until requested | | Switch Swahili to English |  |  | Skipped until requested | Skipped until requested |  |  | Skipped until requested | Skipped until requested | | Mark chapters as completed (in profile edit) |  |  |  |  |  |  | Skipped until requested | Skipped until requested | A couple things to note: - Switching from English to Swahili & back is a bit awkward in the RTL screenshots (partly because the RTL layout is maintained despite switching the content language). This isn't actually a realistic scenario to occur, but it's technically possible so demonstrating it above still makes sense. There are no plans to refine this flow. - The landscape screenshots for the profile edit screens clearly show a broken "Profile Deletion" button placement. This is unfortunately an issue already on develop, and #4852 has been filed to track this. ### Videos demonstrating different changed/new flows | Scenario | Video demonstration | |----------------------------------------------------|---------------------| | Switch language in-lesson | https://user-images.githubusercontent.com/12983742/213829720-075cbb64-cc5a-4a08-ab3b-f6d123650fac.mp4 | | Mark chapters completed | https://user-images.githubusercontent.com/12983742/213829850-e0e157c1-520b-4ff1-86f8-470eb8c37536.mp4 | | Open concept card from hints & other concept cards | https://user-images.githubusercontent.com/12983742/213829894-28228cf9-4c0a-4eca-a617-325676b8d538.mp4 | | Accessibility flow for solutions (fractions, numeric, ratios, text) | https://user-images.githubusercontent.com/12983742/213830378-a56235c2-bc78-4c1f-9248-9531ae0f024d.mp4 | | Accessibility flow for solutions (numeric expressions, algebraic expressions, math equations) | https://user-images.githubusercontent.com/12983742/213830403-b964485b-9b49-43b8-9dfd-35f904c0974e.mp4 | ### Espresso test results The following tests have been modified as a result of this PR: - MarkChaptersCompletedActivityTest - MarkChaptersCompletedFragmentTest - ExplorationActivityTest - StateFragmentTest - ProfileEditFragmentTest - StoryFragmentTest - NavigationDrawerActivityDebugTest - ConceptCardFragmentTest I have **not** run the Espresso tests for these suites. I ran into some issues getting the suites to kick off locally, and I won't have time to investigate this for a bit due to other competing priorities. Reviewers: please let me know if you would like these run and I will prioritize accordingly (I'm leaning toward not running them since many of the Espresso tests are already failing on develop, and we have no viable way to keep them passing until we can run them in CI).
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 #4519
Fixes #4522
This PR expands concept card link support to hints/solutions, as well as other concept cards. Since both cases are dialogs, the behavior is that the newly opened concept card dialog "stacks" on top of the existing, open dialog (i.e. closing the concept card will show the original dialog from which it was opened).
Note that one necessary behavior change here was making only the hint & solution headers clickable, rather than the whole card, when collapsing the hint/solution. This was done intentionally to avoid potential user confusion that could arise from the fact that the now-clickable description text stops that text view from being clickable (so part of the card, when tapped, wouldn't collapse).
Essential Checklist
For UI-specific PRs only
This PR introduces new UI flows for hints/solutions and concept cards, but the actual UI/UX hasn't changed for the app. To keep things simple, here's a video showing the new behavior:
TODO
The only directly affected Espresso test is ConceptCardFragmentTest. Here are its results:
TODO