Fix part of #4044: Add support for expression evaluation & conversion to LaTeX #4054
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
Much of this is copied from #2173.
Also adds testing libraries for both + fractions & reals (new structure). Most of this is copied from #2173.
…tor' into add-support-for-math-expressions-pt3-math-expression-protos
…otos' into add-support-for-math-expressions-pt4-commutative-comparisons-protos
…isons-protos' into add-support-for-math-expressions-pt5-polynomial-protos
This is mostly copied from #2173.
This includes full error detection, and specific test suites for each parsing case. Much revisement is needed in the tests, and some additional issues may yet need to be fixed in the parser and/or error-detection logic. This is copied from #2173 with revisement & reduction since it's part of a multi-PR split.
This is mainly copied from #2173.
…rser' into add-support-for-math-expressions-pt8-latex-conversion-and-eval
…tor' into add-support-for-math-expressions-pt3-math-expression-protos
…otos' into add-support-for-math-expressions-pt4-commutative-comparisons-protos
…isons-protos' into add-support-for-math-expressions-pt5-polynomial-protos
… into add-support-for-math-expressions-pt6-tokenizer
…d-support-for-math-expressions-pt7-math-expression-parser
…rser' into add-support-for-math-expressions-pt8-latex-conversion-and-eval
|
Hi. As of today, some main reviewers have taken time off for the next few weeks, so it may take a little while before we can look at this PR. We appreciate your patience while some of our team members recharge. We'll be fully returning on 4 January 2021. |
|
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
6 tasks
oppiabot
bot
removed
the
stale
|
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
… into add-support-for-math-expressions-pt2-math-utility-refactor
oppiabot
bot
removed
the
stale
…-utility-refactor
…tor' into add-support-for-math-expressions-pt3-math-expression-protos
…-expression-protos
…otos' into add-support-for-math-expressions-pt4-commutative-comparisons-protos
…utative-comparisons-protos
…isons-protos' into add-support-for-math-expressions-pt5-polynomial-protos
… into add-support-for-math-expressions-pt6-tokenizer
…d-support-for-math-expressions-pt7-math-expression-parser
Base automatically changed from
add-support-for-math-expressions-pt7-math-expression-parser
to
develop
March 26, 2022 05:23
…-expression-parser
…rser' into add-support-for-math-expressions-pt8-latex-conversion-and-eval
BenHenning
changed the title
BenHenning
deleted the
add-support-for-math-expressions-pt8-latex-conversion-and-eval
branch
BenHenning
added a commit
that referenced
this pull request
Mar 27, 2022
…4068) ## Explanation Fix part of #4044 This PR introduces support for rendering raw LaTeX in the app using the KotliTeX (https://github.com/karino2/kotlitex) library which is necessary since users may input arbitrary math expressions that need to be pretty-rendered (using the math expression -> LaTeX generation support added in #4054). This implementation is heavily based on #3194 and leverages a new fork (https://github.com/oppia/kotlitex) of KotliTeX based on @anandwana001's fork. In order to keep the implementation simpler, this hangs the functionality onto the existing custom Oppia noninteractive math tag except it only renders LaTeX if there's no SVG filename to load (which will be the case for dynamically generated LaTeX). This approach has the advantage of the app always being able to fall back to LaTeX rendering in the unlikely scenario where content has a math tag without an SVG URL being present (or where the SVG fails to load which could allow potential fault tolerance when downloads support is fully implemented). These changes required several changes to KotliTeX itself which are outlined in more detail below. Furthermore, it was observed that the rendering was noticeably slow on a Nexus 5X for repeated renders (think navigation back and forth between views with rendered LaTeX, or expanding/collapsing multiple answers). This didn't seem acceptable, so this PR also introduces a platform parameter for utilizing Glide to pre-render the LaTeX into a PNG bitmap so that it can cache it. This is not only more performant for repeated exposure and faster to render, but it also offloads the expensive rendering step to a background thread rather than blocking the main thread (see the videos in the UI section below for a before-and-after comparison). ### Details on changes to KotliTeX A custom fork of KotliTeX (https://github.com/oppia/kotlitex) was needed to include the following changes: - Support to be built on Bazel - Lower the min sdk required by the library to match the app's - A slight hack to ensure KotliTeX doesn't auto-fail for larger square roots (the library actually doesn't support them, so this workaround just stretches the smaller square root rather than leading to error text) - A hack to expose the outer bounds of drawing requires by KotliTeX for its span (see caching details below for why) Note that KotliTeX only supports a subset of LaTeX, but it seems to support everything the app needs for now (and it should be extensible by porting other portions of KaTeX as needed). ### Details on the rendering caching & positioning challenges At a high-level, the approach to caching is to render the KotliTeX drawable to a bitmap that can be saved on-disk in a format that Glide can easily load on its own (PNG). While it's not ideal to perform the extra PNG conversion step, it's much more disk-friendly (and in the future, we could look into offloading just this part to another thread if needed). Each individual load is keyed on the exact raw LaTeX, the line height (up to 2 digits), and whether it's inline or block rendered (see later in this section for the distinction). If all three of these properties match, there should be a Glide cache hit and no need to re-render the LaTeX. A couple of issues arose when trying to implement this: - Despite us being able to expose the internal KotliTeX drawable (which wasn't by default), I was having a lot of difficulty getting the text alignment correct. Instead, it was much easier to rely on Android's internal text rendering by just rendering the span in a similar way to what TextView does. - While the above worked, it led to another problem: figuring out how large the bitmap should be. The actual drawable bounds that KotliTeX computes is an underestimate (its ascent and descent go outside of these bounds), so an additional estimation is needed. - Because we're now drawing the LaTeX as a bitmap in an ImageSpan, the text aligns the image either with its bottom line or its baseline. This introduces a vertical center alignment issue as shown below (the image with the red text shows one attempt at fixing this by eliminating unnecessary upper space by using a combination of vertical translation and negative bounds). Due to this quirk, we can't actually have whitespace below the text without modifying the text's font metrics (specifically the bottom/top/ascent/descent properties). #4170 is tracking this work. Note also that this is really only an issue for inline rendering which we're unlikely to use in the medium-term since all user-submitted answers will be rendered in block style which seems to work well. Caching is demonstrably much more performant for large amounts of repetition, and it doesn't result in UI lag like direct rendering does since all rendering is offloaded to a background thread (see the videos below for the comparison). ### Details on testing The tests are mainly focused around changes to the tag handler since the Glide portions can't be tested, and neither can the rendering routines (hence the exemptions). The fake image loader was updated to include support for the math-based rendering load requests. An aside change was made to make the Robolectric library test-only (it's something that was noticed in passing during development, and isn't directly needed by this PR; it's just a nice-to-have). The new interfaces are exempted from tests since they have no logic to verify (their implementations are generated by Dagger and verified at compile time). Furthermore, I didn't add any tests for the changes to the compute affected tests script since it's a bit difficult to test, and already quite an edge case. Please let me know if you have any concerns with this. ### Details on the race condition fix KotliTeX seems to initialized shared state when its parsing the LaTeX, and this with multiple Glide threads can cause a race condition where the LaTeX fails to render. This has been fixed by leveraging coroutines (where we still try to maximize efficiency and parallelization), but it required exposing new injector pathways for the coroutine dispatchers and ``ConsoleLogger``. ### Details on the ComputeAffectedTests fix This PR exposes an issue with the ComputeAffectedTests script wherein it can sometimes exceed the system's maximum argument length. This is being worked around by partitioning values for longer Bazel queries into multiple calls and concatenating the results. I'm fairly certain this is functionally equivalent to the current implementation, and for the vast majority of future PRs only one partition will ever be used. There doesn't seem to be an alternative possible since this is a platform-specific Kernel restriction. ## 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 For the most part, this PR doesn't actually affect users yet since this functionality isn't exposed (all LaTeX currently rendered in the app will utilize the existing SVG pipeline). The user-facing UI changes will be thoroughly documented in the PR that's planned for after this one since it'll be integrating all prior PRs along with the changes introduced here. That being said, the "What is a Fraction?" test revision card has been updated to include rendered LaTeX text in order to demonstrate the functionality more closely. The following is how rendering looks (with bitmap caching):  Note that the above, when compared with non-cached rendering, demonstrates where some of the spacing issues mentioned above come in:  Here's the image showing the bounds when trying to force the drawable downward:  This video shows how long a bunch of LaTeX expressions takes to render with direct rendering: https://user-images.githubusercontent.com/12983742/153363983-4ff99db5-52da-4360-9630-cc98397dc07b.mp4 Versus the performance of caching for the same experience (note this occurred with a completely clean local app cache): https://user-images.githubusercontent.com/12983742/153364178-3066d6ce-2a85-45c8-83fe-c0378a786023.mp4 ### Notes on rendering approach Note that rendering during cache time involves a three step process: 1. Approximate the outer bounding volume by tracking all draw calls needed for a given LaTeX expression (these bounds are used to create the bitmap that will store the cacheable render) 2. Render the LaTeX to a canvas with dimensions larger than the approximated bounding (since it's possible for the rendering to exceed these estimates--see notes below) 3. Crop the bitmap to the smallest bounds needed to encapsulate all of the pixels (1) is needed because KotliTeX actually incorrect computes bounds (it sometimes over and underestimates). This isn't as apparent when directly rendering because Android will handle text expanding past the space KotliTeX expects since it's rendered in-line. I spent some time trying to debug this within KotliTeX but I couldn't make much headway. It may be worth looking into this in the future to see if we can reliably calculate the bounds within KotliTeX (since it will likely be much more performant than the solution used here). Seeing KotliTeX's debug lines makes this clearer (where it thinks the bounds are relative to glyphs; note that this screenshot is using rendering without caching):  (2) is needed because the methodology used for (1) is inexact. It's actually really difficult to estimate exactly where the final glyph pixels will be rendered for a given glyph since KotliTeX is directly handling positioning (rather than allowing Android to compute x/y coordinates based on nearby characters to account for tracking, kerning, etc.). The dimension approximation for (1) is much closer to correct than KotliTeX's internal bounds calculations, however it tends to slightly underapproximate the bounds. To counteract this, the app uses a bitmap that's 2x what's actually estimated to be needed and offsets rendering into that bitmap such that 50% of the estimated size is a margin around the drawing area. This provides a large buffer to account for incorrect bounds calculations. The app then crops this to the edges of filled pixels so that the smallest bitmap is used to represent the final rendered equation. While this uses a lot more memory than should actually be required for rendering, it guarantees perfect results. See: Before (notice the over-sized LaTeX, and that some expressions are partially cut-off from the right and above, or have extra space below): |  |  | |------|------| After (with size estimation & croppingl this also includes a fix in UrlImageParser to ensure that LaTeX images are never auto-resized when rendering in block mode since it distorts them, and that they are properly centered): |  |  | |------|------| Commit history: * Add KDocs & test exemptions. * Lint fixes. * Remove temporary TODOs. * Add tests. * Split StringToFractionParser. This is a temporary change that will be finished upstream (since there's an earlier PR that's a better fit for this change). * Address reviewer comments + other stuff. This also fixes a typo and incorrectly ordered exemptions list I noticed during development of downstream PRs. * Move StringExtensions & fraction parsing. This splits fraction parsing between UI & utility components. * Address reviewer comments. * Alphabetize test exemptions. * Fix typo & add regex check. The new regex check makes it so that all parameterized testing can be more easily tracked by the Android TL. * Add missing KDocs. * Post-merge cleanups. Also, fix text file exemption ordering. * Add new test for negation with math symbol. * Post-merge fixes. * Add KDocs. Also, add new regex exemption for new parameterized tests in this branch. * Refactor & simplify real ext impl. Also, fix/clarify some KDocs. * Lint fixes. * Simplify operation list converter a lot. This inlines three recursive operations to be done during the actual computation to simplify the overall converter complexity (and to make determining the test matrix easier). * Prepare for new tests. * Remove the ComparableOperationList wrapper. * Change parameterized method delimiter. * Use utility directly in test. * Post-merge fixes. This adjusts for the removal of ComparableOperationList (i.e. no wrapper proto). * Add first round of tests. This includes fixes to the converter itself as it wasn't distributing both product inversions and negation correctly in several cases. Tests should now be covering these cases. * Finish initial test suite. Still needs to be cleaned up, but after converter refactoring attempts. * Simplify operation sorting comparators. * Remove old tests. * Add remaining missing tests. * KDocs & test exemption. * Renames & lint fixes. * Post-merge fixes. * Add tests. * KDocs + exemptions. Also, clean up polynomial sorting. * Lint fixes. * Post-merge fixes. Also, mark methods/classes that need tests. * Add extension tests. * Add classifier tests. * Use more intentional epsilons for float comparing. * Treat en-dash as a subtraction symbol. * Add explicit platform selection for paramerized. This adds explicit platform selection support rather than it being automatic based on deps. While less flexible for shared tests, this offers better control for tests that don't want to to use Robolectric for local tests. This also adds a JUnit-only test runner, and updates MathTokenizerTest to use it (which led to an almost 40x decrease in runtime). * Exemption fixes. Also, fix name for the AndroidJUnit4 runner. * Remove failing test. * Fix unary expression precedence. Also, use ParameterizedJunitTestRunner for MathExpressionParserTest. * Fixes & add more test cases. * Post-merge fixes & test changes. Also, update RealExtensionsTest to use the faster JUnit runner. * Use utility directly in LaTeX tests. * Post-merge fixes. Also, update ExpressionToComparableOperationConverterTest to use the fast JUnit-only runner. * Post-merge fixes. Also, update PolynomialExtensionsTest to use fast JUnit-only runner. * Post-merge fixes. Also, update float interval per new tests. * Lint & other check fixes. * Replace deprecated term. * Post-merge fixes. * Add full test suites for alg exp classifiers. * Lint & static check fixes. * Fix test on Gradle. * Fix test for Gradle. * Add tests for math equations. And, post-merge fixes. * Static check & lint fixes. * Post-merge fixes. Verified CI checks & all unit tests are passing. * Split up tests. Also, adds dedicated BUILD.bazel file for new test. * Add missing test in Bazel, and fix it. * Correct order for genrule. * Add full test suite. * Clean up + KDocs + exemption. * Lint fixes. * Post-merge fix. * Cache KotliTeX renders. Directly rendering LaTeX through KotliTeX is way too slow, so this introduces a custom flow through Glide that computes a PNG for the LaTeX on a background thread and then caches it as part of Glide's cache to speed up re-renders of the LaTeX. We may need to manage/prune the cache over time, but for now we'll just rely on Glide's internal behaviors. This also documents some new tests that should be added, but it's not comprehensive. * Add tests, docs, and exemptions. * Update to fixed version of KotliTeX. The newer version correctly computes the bounds for rendered LaTeX. * Lint fixes. * Add new dependency licenses. This isn't done yet (some of the licenses still need to be fixed). * Fix license links. Note that the kxml one was tricky since its Maven entry says it's licensed under BSD and CC0 1.0, and its SourceForge link says the same plus LGPL 2.0. However, the source code and GitHub version of the project license it under MIT, and this seems to predate the others so it seems like the most correct license to use in this case and the one that we're using to represent the dependency. * Fix Gradle build. This uses a version of KotliTeX that builds correctly on Jitpack for Gradle, and fixes the StaticLayout creation to use an alignment constant that builds on Gradle (I'm not sure why there's a difference here between Gradle & Bazel, but the previous constant isn't part of the enum per official Android docs). * Create the math drawable synchronously. This requires exposing new injectors broadly in the app since the math model loader doesn't have access to the dependency injection graph directly. * Remove new deps from Maven list. They were incorrectly pulled in by KotliTeX. * Add argument partitioning. This fixes cases where argument calls may be very large and fail to execute due to exceeding system limitations. * Make allowance for empty cases to fix tests. These tests correspond to real scenarios. * Lint fixes. * Address reviewer comment. Clarifies the documentation in the test runner around parameter injection. * Fix broken build. * Fix broken build post-merge. * Fix broken post-merge classifier. * Address reviewer comment. * Post-merge build fixes. * Post-merge build fixes for new classifiers. * Post-merge build fixes. * Correct reference document link. * Ensure LaTeX isn't stretched or cut-off. The comments in-code have much more specifics on the approach. * Add and fix missing test (was broken on Gradle). * Post-merge fix. * More post-merge fixes. * Fix TODO comment. * Post-merge fix. * Update KotliTeX version. This version doesn't have debug drawing enabled.
BenHenning
added a commit
that referenced
this pull request
Mar 27, 2022
…ns (#2173) ## Explanation Fixes #4044 This PR introduces the UI interaction views for numeric expressions, algebraic expressions, and algebraic equations, finishing the initial implementation of the broader math expressions project. This work unblocks adding support for 4 additional topics in the app: Multiplication (support was lost after Alpha MR2 due to a change that added numeric expression questions), Addition & Subtraction, Division, and Expressions & Equations. This PR finishes a long chain of PRs that were needed to provide the domain functionality to support these new interactions, as a significant amount of mathematics functionality needed to be added including: - Support for representing generic math expressions/equations, polynomials, and reals - Support for parsing math expressions - Support for converting between math expressions and: - LaTeX (for rendering) - Human-readable sentences (for accessibility) - Polynomials and real-value evaluation (for equivalence checking) - Comparable structures to verify that two expressions/equations match irrespective of associativity or commutativity order - Robust error detection to support nearly 2 dozen special cased errors with potential for countless more if needed to ensure users receive excellent feedback when inputting expressions All of the above also required thorough testing to ensure correctness. See the previous PRs corresponding to #4044 for the full context, and thorough PR descriptions explaining past changes. From a UI perspective, this PR is introducing: - The new interaction views and relevant view models, including the logic for constructing and displaying errors for invalid answers - Support for playing a custom content description for math expression answers so that screenreader users can better understand the expression they've entered - Support for rendering the LaTeX representation of entered expressions - A new developer options menu to provide support to input arbitrary expressions and equations and convert them to different outputs to test the underlying math engine that's been built (though it can't yet test the classifiers, but it provides enough information to verify the more complex bits behind the classifiers) From a functional perspective, this PR is introducing: - Support for loading JSON explorations that include math expressions - A new test exploration that's been added to test topic 0 to demonstrate all three interactions and each of those interaction's classifiers (9 total states have been added) The above, and more, are explained in more detail below. ### Background on UI implementation for new interactions The UI implementation leverages a single new interaction view & model for all three new interaction types since they are very similar to one another, including handling accessibility and LaTeX rendering cases. The customization arguments, however, don't exactly match so there are some inconsistencies there. ### Background on parsing approach The new interactions rely on custom math infrastructure for parsing math expressions since no suitable libraries were found that were both license compatible and didn't leverage native code. While this significantly increases the scope of code that needs to be maintained in the project, it has the added benefit of leveraging extremely specific functionality to reduce inconsistencies with Oppia web and ensure a very high quality learner experience when it comes to answer classification and error detection. Expressions are constructed using an LL(1) recursive-descent parser with early-exit error detection (see the [technical specification](https://docs.google.com/document/d/1JMpbjqRqdEpye67HvDoqBo_rtScY9oEaB7SwKBBspss/edit)). Parsing first goes through an LL(1) lazy tokenization process before being parsed lazily into proto-defined math expression/equation structures. Classifier needs are satisified through custom infrastructure that can evaluate numeric expressions to a single real value, compare math expressions/equations with floating point error correction, compare math expressions/equations irrespective of operation associativity and commutativity by transforming it to an intermediary representation, and can compare expressions/equations for equivalence by first converting it to a polynomial using a custom nearly fully feature complete polynomial system. Finally, UI-specific needs are satisified through generators for both LaTeX and human-readable accessibility strings. The preceding PRs in this project contain the implementations for parsing, conversion, and comparsion and include PR descriptions that explain these systems & other peripheral changes in great detail. ### PR history This PR contains **all** of the PRs corresponding to both the previous attempt at implementing math expressions, and the latest (since all commits end up being duplicated in PR chains). There's also some early work that preceded the original work in this PR that's included in its history (it goes back quite a bit). For reference, here's the entire chain of PRs in order that precede this one for implementing math expressions support: 1. #4045 2. #4046 3. #4047 4. #4049 5. #4050 6. #4051 7. #4052 8. #4054 9. #4055 10. #4056 11. #4057 12. #4058 13. #4059 14. #4061 15. #4063 16. #4068 17. #4237 18. #4239 ### Refactors, miscellaneous changes, and future work All interaction view models were refactored to use a factory pattern rather than a function to improve readability, and to make that view model construction consistent with other similar situations (i.e. cases where new instances need to be created with Dagger dependencies). Split screen supported interaction IDs have been refactored to be a Dagger set to avoid a direct dependency on InteractionViewModelModule to access the IDs (the new solution is more Dagger 2 idiomatic). This PR doesn't finish all aspects of the project as there are a number of improvements that have been proposed toward the end of development. These have been cataloged in #4254 and will be implemented in a follow-up PR (but will only be started after this PR & its predecessors have been merged). None of those work items block the overall project, and are intentionally being delayed to a future work item to ensure that math expressions can land more quickly. ### Test specifics & exemptions Changes were needed in ``EditTextInputAction`` to support inputting Unicode text (which is necessary for some UI tests that verify using math symbols such as for times and divide). Espresso itself doesn't support inputting these characters (presumably since it can't easily find them on the keyboard), so the action now exposes an option to directly set the text. Exemption explanations: - ``MathExpressionInteractionsViewTest``: this has been enabled to allow parameterized tests to ensure it can verify a broad set of questions and answers. This might actually be a nice pattern to follow for other interaction tests in the future. - All test exemptions are either can't be obviously tested (listeners & annotations), are tested through other suites (view models & presenters), is especially difficult to test (``ParameterizedAutoAndroidTestRunner``), or is deemed not important enough to add tests for due to a trivial implementation and in the effort to finish this project sooner (``SplitScreenInteractionModule``). Note that the defaulting cases for accessibility strings & LaTeX (i.e. the scenarios where generation to either fails) can't easily be tested since the scenarios in which generation fails arise from invalid answers that can't be submitted since they trigger submit-time errors. However, the failure cases for both of these utilities have been thoroughly checked in their respective test suites. This PR also introduces a new addition to ``OppiaParameterizedTestRunner``: ``ParameterizedAutoAndroidTestRunner``. This runner acts like ``AndroidJUnit4`` wherein it will automatically select a Robolectric or Espresso runner depending on the current runtime environment. This ought to only be used by shared tests that are supposed to run on both platforms, otherwise Robolectric or Espresso specific runners should be used (or JUnit for non-Robolectric tests). ## 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 I've verified the interaction with the accessibility scanner locally and it didn't mention any new suggestions (there was one suggestion on the label for the input box when inputting something, and another for the congratulations banner contrast--both of which are existing issues that affect other interactions). ### Screenshots of new UIs | Device type | Language | Orientation | Input box | Error | Inputted answer | |-------------|----------|-------------|-----------|-----------------|-------| | Handset | English | Portrait |  |  |  | | Handset | English | Landscape |  |  |  | | Handset | Arabic | Portrait |  |  |  | | Handset | Arabic | Landscape |  |  |  | | Tablet | English | Portrait |  |  |  | | Tablet | English | Landscape |  |  |  | | Tablet | Arabic | Portrait |  |  |  | | Tablet | Arabic | Landscape |  |  |  | Note that the interaction's errors haven't yet been translated. Also, some work may need to be done in the future to ensure directionality makes sense both for input and rendering (asking folks yielded that input is typically right-to-left but math expressions are still represented left-to-right, so it's not clear if math input should also be left-to-right). In the case above, the text view actually switches the '+2' to '2+1' once it has another number since the context is more complete. It's possible RTL users are already used to quirks like this. It's also likely the custom math keyboard would be a good means of solving this issue long-term. Screenshot of the new developer options menu for testing math expressions/equations:  Note that I didn't bother to screenshot other configurations for the developer options menu since it's only visible to developers, so it's less important to make sure it meets the accessibility, language, device, and orientation requirements that the other screens of the UI are held to. ### Videos | Flow being demonstrated | Video recording | |------------------------------------------------------------|-----------------| | Submitting numeric expressions answers | https://user-images.githubusercontent.com/12983742/158933258-74992080-a9ec-46da-b752-c00b406ab3ec.mp4 | | Submitting algebraic expressions answers | https://user-images.githubusercontent.com/12983742/158933296-0ea70704-9e3b-4e0d-9150-680a47f9ece2.mp4 | | Submitting answers that cause errors | https://user-images.githubusercontent.com/12983742/158933391-602356c3-ccad-4199-8c0b-47d42abd7896.mp4 | | Rendering fractions vs. divisions | https://user-images.githubusercontent.com/12983742/158933425-9abc640b-289e-484f-8f30-dab38789c99e.mp4 | | Submitting numeric expressions with a screenreader enabled | https://user-images.githubusercontent.com/12983742/158933446-21afbcd6-0ea8-4558-8c68-9fa0157a7e3d.mp4 | | Submitted math equations with a screenreader enabled | https://user-images.githubusercontent.com/12983742/158933460-ddf39ba3-1fd8-4bfc-82e9-cc1845846646.mp4 | ### Espresso test results |  |  | |------|------| |  |  | |  |  | |  |  | Note that in the case of ``MathExpressionInteractionsViewTest`` this is the first PR that demonstrates using parameterized tests with Espresso. Commit history: * Treat en-dash as a subtraction symbol. * Add explicit platform selection for paramerized. This adds explicit platform selection support rather than it being automatic based on deps. While less flexible for shared tests, this offers better control for tests that don't want to to use Robolectric for local tests. This also adds a JUnit-only test runner, and updates MathTokenizerTest to use it (which led to an almost 40x decrease in runtime). * Exemption fixes. Also, fix name for the AndroidJUnit4 runner. * Remove failing test. * Fix unary expression precedence. Also, use ParameterizedJunitTestRunner for MathExpressionParserTest. * Fixes & add more test cases. * Post-merge fixes & test changes. Also, update RealExtensionsTest to use the faster JUnit runner. * Use utility directly in LaTeX tests. * Post-merge fixes. Also, update ExpressionToComparableOperationConverterTest to use the fast JUnit-only runner. * Post-merge fixes. Also, update PolynomialExtensionsTest to use fast JUnit-only runner. * Post-merge fixes. Also, update float interval per new tests. * Lint & other check fixes. * Replace deprecated term. * Post-merge fixes. * Add full test suites for alg exp classifiers. * Lint & static check fixes. * Fix test on Gradle. * Fix test for Gradle. * Add tests for math equations. And, post-merge fixes. * Static check & lint fixes. * Post-merge fixes. Verified CI checks & all unit tests are passing. * Split up tests. Also, adds dedicated BUILD.bazel file for new test. * Add missing test in Bazel, and fix it. * Correct order for genrule. * Add full test suite. * Clean up + KDocs + exemption. * Lint fixes. * Post-merge fix. * Cache KotliTeX renders. Directly rendering LaTeX through KotliTeX is way too slow, so this introduces a custom flow through Glide that computes a PNG for the LaTeX on a background thread and then caches it as part of Glide's cache to speed up re-renders of the LaTeX. We may need to manage/prune the cache over time, but for now we'll just rely on Glide's internal behaviors. This also documents some new tests that should be added, but it's not comprehensive. * Add tests, docs, and exemptions. * Update to fixed version of KotliTeX. The newer version correctly computes the bounds for rendered LaTeX. * Lint fixes. * Add new dependency licenses. This isn't done yet (some of the licenses still need to be fixed). * Fix license links. Note that the kxml one was tricky since its Maven entry says it's licensed under BSD and CC0 1.0, and its SourceForge link says the same plus LGPL 2.0. However, the source code and GitHub version of the project license it under MIT, and this seems to predate the others so it seems like the most correct license to use in this case and the one that we're using to represent the dependency. * Fix Gradle build. This uses a version of KotliTeX that builds correctly on Jitpack for Gradle, and fixes the StaticLayout creation to use an alignment constant that builds on Gradle (I'm not sure why there's a difference here between Gradle & Bazel, but the previous constant isn't part of the enum per official Android docs). * Create the math drawable synchronously. This requires exposing new injectors broadly in the app since the math model loader doesn't have access to the dependency injection graph directly. * Remove new deps from Maven list. They were incorrectly pulled in by KotliTeX. * Add argument partitioning. This fixes cases where argument calls may be very large and fail to execute due to exceeding system limitations. * Post-merge fixes. Note that only the following tests are failing (some of which may be due to the previous branch): - MarkChaptersCompletedFragmentTest - MarkStoriesCompletedFragmentTest - StoryProgressTestHelperTest - ModifyLessonProgressControllerTest - TopicListControllerTest - ComputeAffectedTestsTest - MarkTopicsCompletedFragmentTest - HomeActivityTest * Make allowance for empty cases to fix tests. These tests correspond to real scenarios. * Lint fixes. * Fix broken tests and add reasonable exemptions. * First pass at adding tests. Some debugging left, and more tests to add. This also adds auto-switching support for parameterized tests. * Fix DeveloperOptionsActivityTest. Also, fix all other test builds (for Gradle). This will probably require some reformatting. * Add & fix remaining tests. This also fixes a bug where the correct answer a11y label was being incorrectly assumed to be the correct answer even for incorrect cases. * Add docs & clean up remaining non-lint parts. * Lint & small breakage fixes. * Fix broken tests. * Address reviewer comment. Clarifies the documentation in the test runner around parameter injection. * Fix broken build. * Fix broken build post-merge. * Fix broken post-merge classifier. * Address reviewer comment. * Post-merge build fixes. * Post-merge build fixes for new classifiers. * Post-merge build fixes. * Correct reference document link. * Ensure LaTeX isn't stretched or cut-off. The comments in-code have much more specifics on the approach. * Add and fix missing test (was broken on Gradle). * Fix Gradle tests. This commit has verified the following tests now pass on Espresso: - MathExpressionInteractionsViewTest - HtmlParserTest - MathExpressionParserActivityTest - MathExpressionParserFragmentTest - DeveloperOptionsActivityTest - DeveloperOptionsFragmentTest StateFragmentTest is having issues (it ends up hanging), so further investigation is needed. * Update to newer version of Kotlin coroutines. This version includes StateFlow which will be a really useful mechanism for helping to avoid using critical sections. * First attempt to fix deadlock. This uses StateFlows and an actor to avoid the deadlock. This is missing necessary hint changes that are coming in a later commit. Tests currently fail, and questions haven't yet been migrated (and won't until the fixes are verified). * Attempt to make hint handler multi-threadable. This is a midway solution that's being committed for posterity, but will be reverted in favor of a solution that forces hint handler to be unsafe across multiple threads (since it's much simpler, and works given that all users of it now synchronize state management using an actor). * Finish fixing state player. This includes a fix for accessibility string handling (since the new flow triggers one of these to fail). It also adds a Bazel file for StateFragmentTest (I spent some time trying to get Espresso tests working with Bazel but ran into a compatibility issue). StateFragmentTest has been verified to pass on Robolectric and Espresso with this change. This sets up the project for fixing questions in the next commit. * First pass on migrating question controller. This also includes a migration for exploration & question domain tests to use the test monitor utility. The question tests currently fail since there's a bug in AsyncResult where it won't support null values during transformations. * Refactor AsyncResult into a sealed class. This also introduces an AsyncResultSubject, and more or less fully fixes issue #3813 in all tests. * Refactor AsyncResult into a sealed class. This also introduces an AsyncResultSubject, and more or less fully fixes issue #3813 in all tests. This is a cherry-pick from the fix-progress-controller-deadlock branch since it ended up being quite large (it made more sense to split it into a pre-requisite PR). Conflicts: app/src/main/java/org/oppia/android/app/player/state/testing/StateFragmentTestActivityPresenter.kt domain/src/main/java/org/oppia/android/domain/exploration/ExplorationProgressController.kt domain/src/main/java/org/oppia/android/domain/question/QuestionAssessmentProgressController.kt domain/src/test/java/org/oppia/android/domain/exploration/lightweightcheckpointing/BUILD.bazel testing/src/main/java/org/oppia/android/testing/data/DataProviderTestMonitor.kt utility/src/main/java/org/oppia/android/util/data/DataProviders.kt * Post-merge fixes and updates for consistency. * Post-merge fixes. * TODO has been addressed. * Fix documentation & add tests. * Lint fixes. * Lint & post-merge fixes. Questions-related tests still fail and need to be fixed now that AsyncResult has been refactored to support null values. * Post-merge test fixes. The core affected UI/domain tests have now been verified as working on Robolectric. The full test suite is next. * Add documentation & tests. Also, fix a bug in the automatic StateFlow DataProviders wherein they wouldn't notify correctly on changes. This led to some simplifications in the exploration & question progress controllers. * Lint fixes. * Fix gradle tests. I've verified in this commit that all Gradle tests build & run locally (at least on Robolectric). * Fix Proguard build. This required bringing kotlinx-coroutines-android up-to-date with kotlinx-coroutines-core (since that was updated on this branch). New but reasonable Proguard warning exemptions also needed to be added. * Post-merge fixes. * Gradle fixes. This also fixes an issue wherein answers couldn't be resubmitted for math expression interactions after an error occurred. * Fix parameterized runners for Espresso. This fixes a typo when loading the AndroidJUnit4 runner during parameterization runer selection, and ensures that test names don't contain illegal characters that would break the runner. * Post-merge fix. * More post-merge fixes. * Fix TODO comment. * Post-merge lint fixes. * Post-merge fix. * Fix exploration routing issue. The underlying problem was that the PR inadvertently changed the behavior of comparing two results wherein results of different times would be considered different and be re-delivered (which happened to break exploration routing, and likely a variety of other places). This introduces a new method for checking equivelance rather than confusingly assuming that users of AsyncResult don't care about the result's time during comparison checking. New tests have been added to verify the functionality works as expected (and fails when expected), and I manually verified that the exploration routing issue was fixed. More details on the specific user-facing issue will be added to the PR as a comment. * Post-merge fixes. * Post-merge fixes. The local repo has been verified to pass nearly all static analysis and lint tests, as well as all Robolectric tests. The developer and alpha versions of the app build. * Update KotliTeX version. This version doesn't have debug drawing enabled. * Fix lifecycle breakage. I noticed downstream that quickly navigating back and forth to enter & exit an exploration can cause the progress controller to get into a bad state that either leads to a broken experience (stacked explorations or a blank state), or a crash. The issue was coming from DataProviders being shared across sessions even though the underlying state was different. This change ensures that providers stay completely independent, similar to the core session state. * Address reviewer comment. * Update play session controllers. This ensures that the command queue itself is fully isolated to avoid delayed events potentially leaking across sessions (now that the session ID barrier has been removed).
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
Fix part of #4044
Originally copied from #2173 when it was in proof-of-concept form
This PR introduces support for:
Realvalue)The LaTeX support is fairly barebones, but it covers all cases that can be represented by the math expression and equation protos. Note that it has a special mode for treating divisions as fractions as this is a requirement for the upcoming math interactions that are being added (as a creator-specified configuration argument).
Some specifics regarding evaluation:
Some notes on testing:
MathEquationSubjectandMathExpressionSubjectwere updated to include convenience functions for evaluation & LaTeX. These aren't directly tested since they're expected to be tested as part of the future test suites for each of these subjects.RealSubject.assertThatwas updated to accept a null value (since this can come up with evaluation test cases).Realoperation has a matrix of different combinations with different outcomes. This test coverage is important since these are the fundamental operational building blocks of polynomial calculations and evaluation.Essential Checklist
For UI-specific PRs only
N/A -- This change is introducing test-only functionality (numeric expression evaluation) and support for something that will later be shown on the UI (so it doesn't yet affect any UI screens or flows).