Fix #4044: Introduce UI support for math expressions & new interactions #2173
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
utility/src/main/java/org/oppia/android/util/math/MathTokenizer.kt
Outdated
Show resolved
Hide resolved
BenHenning
added
the
PR don't merge - NEEDS UPDATE
|
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 3 days, it will be automatically closed so that others can take up the issue. |
oppiabot
bot
added
the
stale
oppiabot
bot
removed
the
stale
BenHenning
changed the base branch from
develop
to
build-all-targets-in-bazel-ci
BenHenning
removed
PR don't merge - NEEDS UPDATE
|
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
oppiabot
bot
removed
the
stale
|
Remaining work (for proof-of-concept):
Work for finishing up the project:
|
|
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
oppiabot
bot
removed
the
stale
BenHenning
added a commit
that referenced
this pull request
Dec 14, 2021
6 tasks
BenHenning
changed the base branch from
develop
to
add-support-for-math-expressions-pt1-fix-proto-imports
…ccessibility-string' into add-support-for-math-expressions-pt16-latex-rendering
…port-for-math-expressions-pt16-latex-rendering
…into refactor-async-result
BenHenning
added a commit
that referenced
this pull request
Mar 27, 2022
…migrating progress controllers over to a command-queue structure (#4239) ## Explanation Fix #3622 Fix #4238 Fix #3861 Fix part of #4044 (see below for how this relates to the broader math expressions project) This PR fixes a deadlock that's possible in both ``QuestionAssessmentProgressController`` and ``ExplorationProgressController`` wherein the internal locks used to synchronize session state can conflate with the locks of the controllers' ``HintHandler``s (which calls back into the controller within the lock). While this seems really difficult to hit when playing a training session or exploration, ``StateFragmentTest`` seems to very consistently hit the deadlock after only 10-15 tests (so it essentially can't be run without the fixes introduced here). While I could've tried rearranging the locks to be more cooperative, it's actually difficult to guarantee that deadlocks won't happen without rebuilding the internal locking mechanism for controller data. I chose the latter by introducing a command queue-esque pattern using a Kotlin actor with ``StateFlow``s for pushing state back to callers (see https://kotlinlang.org/docs/shared-mutable-state-and-concurrency.html; actors are a powerful way to leverage the lightweight nature of coroutines to manage cross-thread state machines). To simplify the implementation, this PR also introduces conversion methods from ``StateFlow``s to ``DataProvider``s (I could see us leveraging these flows more in the future, maybe as a long-term replacement to ``DataProvider``s with some refinement). One side effect of using a command queue to manage what is effectively a session state is that state can now fairly easily leak between sessions. Efforts have been taken to minimize the chance of this happening (mainly ignoring events that arrive from a previously ended session), but a better long-term solution would be to create new session controllers for each session (which should be something possible with custom scopes once we integrate Dagger Hilt in #1720). Due to a general push toward using ``DataProvider``s much more heavily in these controllers rather than ``LiveData``, ``AsyncResult`` needed to be refactored in order for some of the internal transformed chains in the controllers to work correctly (which is where #4237 comes in). This also permanently fixes #3861 and other potential errors that could happen by ensuring that the provider state can continue even if internal errors are encountered rather than entering a permanently broken state (#4230 mitigates the immediate repro steps from the bug by fixing the underlying issue that caused the exception, whereas this PR focuses on preventing those scenarios from causing the bug in question). ### Some explanations for the threading approach I was originally going to make hint handler also a command queue, but it became a bit complicated. To keep things simpler, I removed the threading restricting on it so that it can only be accessed on a single thread (similarly to the internal progress objects used by both controllers) which helps avoid needing a lock without worrying about data races. Note that this command queue style implementation of ``HintHandler`` is available to view in the past commits in this PR if it's interesting for future development (since it's a simpler command queue than the ones used by the controllers due to having an inherently simpler internal state machine). This PR demonstrates a general tendency to move the app toward a 'notify-and-pull' model (e.g. ``DataProvider``s) vs. a push-subscribe pattern. The former is easier to synchronize, parallelize, ensure lifecycle safety, and is often more performant since operations only need to execute when their results are needed. Note that both controllers' internal computation behaviors are completely different with this new approach. Previously, the internal progress state machine would only be fully processed when the current ephemeral state/question was retrieved from the UI layer (i.e. lazily), but now it's computed shortly after the change (i.e. eagerly) since that works a bit better with the ``Flow``s being used for internal cross-thread communication. This technically means the controllers are slightly less efficient since execution may happen in cases when the state/question is no longer being viewed by the user, but it should realistically make a small difference since we have to thread hop for the command queue, anyway, and computing the ephemeral state/question should be cheap. This has the benefit of a potentially snappier load experience in the frontend, though, since the ephemeral state/question will be available sooner. All operations in the controllers now execute their operations regardless of whether they're observed (which should better match callers' expectations). Note that the internal state machine complexity can probably be drastically simplified now that the command queue can also act as a state machine, but I didn't want to risk regressions by removing it (I was trying to keep the logical flow as similar as possible to the existing behavior). ### Updates to dependencies The coroutines core dependency needed to be updated since the older version didn't include ``StateFlow`` which was necessary for providing a sync-back mechanism for the controller command queues. ### Stability checks I verified that both ``QuestionPlayerActivity`` and ``StateFragmentTest`` pass on Espresso (where the latter deadlocked prior to this PR); see the screenshots below. Further, I tested ``ExplorationProgressControllerTest`` and ``QuestionAssessmentProgressControllerTest`` 100x times to verify both are not flaky (since some flakes were discovered during development). Both test suites passed all 100 runs. ### Bazel app module tests This PR introduces support for dedicated app module tests in Bazel (rather than bundling them as part of the broader list of app module tests). This required a few changes in .bzl and .bazel files, but the change is otherwise straightforward and extremely beneficial for team members that rely on the Android Studio with Bazel plugin for regular development. ### Miscellaneous I tried introducing a Bazel emulator test while developing this (as part of #59) but it ran into a desugar issue with Mockito and Robolectric (due to them both being pulled into the test binary that's being built to send to the device) which AGP apparently works around. See bazelbuild/bazel#13684 for a Bazel issue tracking the same issue. This fixes #3622 since it eliminates the lock entirely in favor of using actors for safe cross-thread communication. The ``MediatorLiveData`` specific part of this issue's TODO comment isn't relevant anymore, either, since we replaced ``MediatorLiveData`` with a custom notifying ``LiveData`` within ``DataProviders`` a while back. This fixes #4238 fully by ensuring any exceptions caused within the state flow of an exploration or questions session are captured and logged, but don't actually break the internal state machine of the session. This issue was discovered during the development of #2173 due to an exception being thrown in the classifier. While that issue was fixed, a test couldn't actually be added for the #4238 fix since it's difficult to intentionally trigger exceptions during answer classification. ### A note on checkpoints in ``ExplorationProgressController`` The checkpointing behavior in ``ExplorationProgressController`` became more complex as a result of these changes. In particular, checkpointing behaves as follows: - Something changes in the play session - A checkpoint recomputation is requested (which results in a new checkpoint being computed and then saved to disk) - The checkpoint is 'processed' by updating the play state of the exploration (i.e. in ``StoryProgressController``) - The ephemeral state is recomputed since it contains the checkpoint and the checkpoint has changed Each of these more or less require a command queue "hop" which means other operations can occur between them. As a result, #3467 can still occur since an exploration can be finished before the process happens (though the behavior has changed: the progress simply won't be processed which means an exploration might not be indicated as played, but the checkpointing should almost always be saved before the exploration session end is processed). Fixing this isn't straightforward since the exploration's play state can't be changed until the checkpoint is confirmed as saved which either requires updating ``StoryProgressController`` to also account for the presence of checkpoints, or to use more flows internally to combine the save-and-process operation into one in a way that doesn't lock up ``ExplorationProgressController`` (I'm currently not exactly sure how one would do this, but I haven't thought about it in depth yet). ## 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 does not intentionally change and user-perceivable behaviors in the questions or state players except potentially fixing deadlocks/ANRs that could occur while playing. There could be regressions in the players, and they may potentially be more performant after this change, but neither can easily be demonstrated via videos or screenshots. There are no accessibility behavior changes to demonstrate, either. However, two key tests have been run and verified as passing on Espresso to demonstrate that the deadlock has been fixed. Specifically: | QuestionPlayerActivityTest | StateFragmentTest | |------|------| |  |  | Commit history: * 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). * 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 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. * 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. * 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).
…gress-controller-deadlock
…aic-expression-system
This was referenced Mar 27, 2022
|
(Note that I plan to bypass @vinitamurthi's and @rt4914's approvals per discussions with them in order to merge this to ensure it's included in an upcoming time-sensitive alpha release. #4265 and #4266 are respectively tracking follow-up reviews from both to make sure that this PR is checked for correctness by all owners). |
BenHenning
changed the title
This was referenced Mar 28, 2022
Merged
Closed
6 tasks
BenHenning
added a commit
that referenced
this pull request
May 5, 2022
) ## Explanation <!-- - Explain what your PR does. If this PR fixes an existing bug, please include - "Fixes #bugnum:" in the explanation so that GitHub can auto-close the issue - when this PR is merged. --> Fix #4249 Fix part of #4064 Introduces the domain utilities necessary for logging learner analytics, but doesn't make them available for actual usage yet (that is being done in #4269 to keep this PR smaller & more focused). Some notes on the history of this PR: - This PR is a rebase of #4253 to remove dependencies on #2173 (which has since been merged into developer) for a much cleaner history. - This PR is pulling out elements from #4118, #4247, and #4248 which contained completed work by @Sarthak2601. - This PR extracts just the 'domain' pieces from the above, and changes a bunch about its architecture & adds tests. This PR should have little to no impact on the behavior of the app since the new logging functionality isn't being used yet, or is even accessible to broad components in the app. - This is starting as 'pt4' since it's continuing the work introduced in: #4114, #4115, and #4116. This PR makes a number of changes over the original design document and implementation, the most noteworthy being: - This PR organizes learner analytics logging into its own logger (and makes changes to event bundle creation & the generic ``OppiaLogger``). I think that we should move toward this pattern generally in the future rather than continuing with a generic ``OppiaLogger`` as it seems to help keep things much more focused. Existing logging should not be affected. - The notion of a device ID has been dropped as there's no reliable way retrieve such an ID (see https://developer.android.com/training/articles/user-data-ids). Instead, we're using a per-device ID (by leveraging ``PersistentCacheStore``), and have confirmed with study partners that this is workable. - The logging logic for the new logs was rearranged such that all new analytics logs will be logged for everyone, but the user and installation-tied IDs won't be logged in such cases (since they are more sensitive data). These events are generally useful for the platform, so we shouldn't restrict them as such. - Learner ID generation for profiles only occurs if the experiment is enabled, and otherwise stays empty. We may add future cleanup code to ensure it's erased across studies, but this at least lays the initial groundwork to keep such IDs separate when they aren't needed. For a high-level on the design, please refer to [this design document](https://docs.google.com/document/d/1c8lpH-IUvoU1t4LUoYNqNilP2e9yCnzGnSSG0yBxBrY/edit). Other noteworthy design choices: - ``DebugEventLogger`` was updated to call through to the real logger (as it makes event verification simpler in developer builds; normally analytics is off so this won't have any effects for the broader team) - Both ``DebugEventLogger`` and ``FakeEventLogger`` were updated to be thread-safe - Some extended functionality was added to ``FakeEventLogger`` - ``LearnerAnalyticsLogger`` is designed a bit differently compared to other domain classes in that it actually provides session-specific objects to the application-wide singleton graph (which is needed for logging certain situations, such as the user playing/stopping audio during a play session) - ``LoggingIdentifierController`` makes use of a lazy retrieval for session ID now (which is fine because it's guaranteed to compute exactly one initial ID) - ``StateFlow`` is used for easier cross-thread communication, including to expose internal asynchronous state across domain components (the only way we had to do this before was ``Deferred``, and that can be clunky; the new approach is much cleaner) - An ``EventLogSubject`` was introduced to make testing event logs easier. It's used extensively in tests for this feature, but most existing use cases weren't migrated. #4272 is tracking adding tests for this subject (hence the test file exemption). - There were TODOs introduced on #4064 to provide explicit clarity to reviewers on what needs to be changed in later PRs (as there's some things being introduced before the final PR that aren't actually used yet to help break up the project) - Multiple test suites verify behaviors with and without the feature enabled to be very explicit about what behavior occurs when - ``EventBundleCreatorTest`` in particular has very strict tests to ensure that sensitive IDs are logged exactly when expected (initially, never since they aren't turned on in this PR; this is fixed in a later PR) - ``ExplorationDataController`` was updated to introduce new play entrypoints, but these aren't "interesting" yet as the underlying ``ExplorationProgressController`` changes are coming in a later PR. Further, testing coverage technically removes checking ``playExploration``, but it'll be removed (and it's technically tested through the other functions since they call through). - A new ``ClipboardManager`` was introduced with the specific design of not allowing the broad app access to clipboard information from other apps. Instead, it provides an interface to confirm whether the app's known clipboard has been kept. A regex content check was added to ensure developers never use the clipboard service directly and instead use this manager. - ``PersistentCacheStore`` was updated to include a new ``primeInMemoryAndDiskCacheAsync`` function which works more predictably for initialization than ``primeInMemoryCacheAsync`` (formerly ``primeCacheAsync``). In particular, ``primeInMemoryCacheAsync`` is better for ensuring that the cache will quickly be read once it needs to be (and, if it isn't, will default in the same way the cache store normally defaults). However, there are cases when the app wants to change the default values such that: (1) the normal default is never used, (2) the default has to be computed and isn't cheap, and (3) it should never compute that default again once saved on disk. ``primeInMemoryAndDiskCacheAsync`` makes these assurances which, in turn, makes the installation ID cache store even possible without potential race conditions or breaking Dagger's cheap-initialization best practice. Test exemptions: all exemptions are annotations or interfaces except ``EventBundleCreatorTest`` (which is explained above). ## Essential Checklist <!-- Please tick the relevant boxes by putting an "x" in them. --> - [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 N/A -- This PR doesn't make UI changes, and existing flows shouldn't be affected. Commits: * strings for learner analytics * platform parameter impl for learner analytics * nit * nit * event action enum update * addition of contexts * nit * controller level logging and contexts * nit * nit fixes. * nit fixes. * event bundle modifications * sync status, logging identifiers, profile update, lifecycle owner * ui impl: part 1 -- basic * admin control strings * strings correction * strings correction * device id correction * exhaustive when fix. * exhaustive when fix. * todo formatting * nits. * nits. * collapsed contexts, added spacing, added comments * event action removal + nits * tests + dev options event logs fixes post event action removal * nits * removal of method for event action formatted string * nits, null context changes. * nits * reserved fixes and help index fix * bazel imports * bazel build fixes * test fixes * nit * logging identifier controller, module + uuid wrapper, real impl * logging identifier controller tests, fake uuid, tests * sync status manager + fake * logging methods, test setup * profile management, tests * sync status update. * lifecycle observer * Post-merge fixes + Bazel support. * Lots of reorganizing & changes. New tests and documentation have also been added. More broadly, this changes the device ID computation, but actually breaks it so more work will be needed in subsequent commits. * Lint fixes. * Post-merge fix (proper merge of maven_install). * Lint fixes (includes post-merge cleanups). * Lots of stuff. Restructured a lot of the UI, addressed most failing static checks (except KDocs and lint which will be in the follow-up commit), added tests, fixed copying, and generally finished the UI. Sync status seems broken, and it's not yet clear whether events are actually being logged (I need to investigate this). Analytics are disabled in local testing, so that might also be the reason for logs being stuck in an uploading state. * Documentation + lint fixes. This also changes the contract of ClipboardController. * Finish remaining planned tests. * Move over changes from learner-analytics-proto. * Manually pull in changes from 3d6c716. Note that this is operating on a different base). * Post-merge fixes. These at least ensure that the app can build, but many tests will still fail (which is fine seeing as much of this code is going to be split up soon, anyway). Rebase version: app build is no longer guaranteed. * Lint fixes. * Undo all learner analytics changes. I'll be pulling in specific components in specific PRs to organize the changes across 4 PRs. Note that I took this approach to preserve the history from the earlier commits. Those changes will still be included in this PR chain, just a bit awkwardly (i.e. it'll look like I introduced them originally, but that distinction is lost during the squash-and-merge, anyway). * Manually pull in non-app module changes. A bunch of work is still needed to finish these, and I'm still trying to figure out whether I can de-couple the module changes to make reviewing a bit nicer. * Post-merge fixes. All tests verified as building & passing. * Add sync status for no connectivity case. * Remove unnecessary sync manager. * Copy over changes from #4263. These are the domain changes needed for finishing learner analytics support. Cleanup, documentation, and testing all still need to be completed. * Add domain changes for AudioPlayerController. These originate from #4263. * Add missing Javadoc from #4263. * Finish tests & documentation. This also renames 'device ID' to be 'installation ID' for more correctness. * Lint fixes. * Fix OS-specific issue in ClipboardController. Co-authored-by: Sarthak Agarwal <[email protected]>
BenHenning
added a commit
that referenced
this pull request
May 5, 2022
## Explanation Fix part of #4064 This PR integrates the constituent Dagger modules introduced in #4267 broadly in the app so that new logs can be easily added without changing a bunch of files (see #4270). Some notes on the history of this PR: - This PR is a rebase of #4257 to remove dependencies on #2173 (which has since been merged into developer) for a much cleaner history. - This PR is pulling out elements from #4118, #4247, and #4248 which contained completed work by @Sarthak2601. - This PR extracts just the 'domain integration' pieces from the above, and makes some fixes For a high-level on the design, please refer to [this design document](https://docs.google.com/document/d/1c8lpH-IUvoU1t4LUoYNqNilP2e9yCnzGnSSG0yBxBrY/edit). For the most part, this PR only changes module lists except for: - Changes in ``AnalyticsController`` (& corresponding tests) - Changes in ``LogUploadWorker`` (& corresponding tests) - ``OptionsFragmentTest`` (both of them) since they need to provide custom parameter overrides (the new override needed to be added) The non-test changes above were done to "partially integrate" the necessary pieces that couldn't be done in #4267 without blowing up the PR, and to ensure #4270 can stay focused on just introducing new events & their verification tests. ## Essential Checklist <!-- Please tick the relevant boxes by putting an "x" in them. --> - [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 N/A -- This PR doesn't make UI changes, and existing flows shouldn't be affected. Commits: * strings for learner analytics * platform parameter impl for learner analytics * nit * nit * event action enum update * addition of contexts * nit * controller level logging and contexts * nit * nit fixes. * nit fixes. * event bundle modifications * sync status, logging identifiers, profile update, lifecycle owner * ui impl: part 1 -- basic * admin control strings * strings correction * strings correction * device id correction * exhaustive when fix. * exhaustive when fix. * todo formatting * nits. * nits. * collapsed contexts, added spacing, added comments * event action removal + nits * tests + dev options event logs fixes post event action removal * nits * removal of method for event action formatted string * nits, null context changes. * nits * reserved fixes and help index fix * bazel imports * bazel build fixes * test fixes * nit * logging identifier controller, module + uuid wrapper, real impl * logging identifier controller tests, fake uuid, tests * sync status manager + fake * logging methods, test setup * profile management, tests * sync status update. * lifecycle observer * Post-merge fixes + Bazel support. * Lots of reorganizing & changes. New tests and documentation have also been added. More broadly, this changes the device ID computation, but actually breaks it so more work will be needed in subsequent commits. * Lint fixes. * Post-merge fix (proper merge of maven_install). * Lint fixes (includes post-merge cleanups). * Lots of stuff. Restructured a lot of the UI, addressed most failing static checks (except KDocs and lint which will be in the follow-up commit), added tests, fixed copying, and generally finished the UI. Sync status seems broken, and it's not yet clear whether events are actually being logged (I need to investigate this). Analytics are disabled in local testing, so that might also be the reason for logs being stuck in an uploading state. * Documentation + lint fixes. This also changes the contract of ClipboardController. * Finish remaining planned tests. * Move over changes from learner-analytics-proto. * Manually pull in changes from 3d6c716. Note that this is operating on a different base). * Post-merge fixes. These at least ensure that the app can build, but many tests will still fail (which is fine seeing as much of this code is going to be split up soon, anyway). Rebase version: app build is no longer guaranteed. * Lint fixes. * Undo all learner analytics changes. I'll be pulling in specific components in specific PRs to organize the changes across 4 PRs. Note that I took this approach to preserve the history from the earlier commits. Those changes will still be included in this PR chain, just a bit awkwardly (i.e. it'll look like I introduced them originally, but that distinction is lost during the squash-and-merge, anyway). * Manually pull in non-app module changes. A bunch of work is still needed to finish these, and I'm still trying to figure out whether I can de-couple the module changes to make reviewing a bit nicer. * Post-merge fixes. All tests verified as building & passing. * Add sync status for no connectivity case. * Remove unnecessary sync manager. * Copy over changes from #4263. These are the domain changes needed for finishing learner analytics support. Cleanup, documentation, and testing all still need to be completed. * Add domain changes for AudioPlayerController. These originate from #4263. * Add missing Javadoc from #4263. * Add module deps for learner analytics components. This ensures that the new domain components introduced in the previous branch can be used throughout the app in both production and test components. * Post-merge fixes from different branch. * Remove all UserIdProdModule references. The module was removed during development in favor of a more integrated approach to managing the logging-related IDs. * Post-merge fix. * Remove references to missing module per #4263. * Add files changes from manual merges/cherry-picks. * Finish tests & documentation. This also renames 'device ID' to be 'installation ID' for more correctness. * Lint fixes. * Post-merge fixes. * Lint fixes. * Remove files incorrectly readded. * Add follow-up tests to improve coverage. * Update domain's Gradle Mockito version to Bazel's. * Fix OS-specific issue in ClipboardController. Co-authored-by: Sarthak Agarwal <[email protected]>
BenHenning
added a commit
that referenced
this pull request
May 5, 2022
## Explanation Fix part of #4064 This PR integrates the constituent Dagger modules introduced in #4267 broadly in the app so that new logs can be easily added without changing a bunch of files (see #4270). Some notes on the history of this PR: - This PR is a rebase of #4258 to remove dependencies on #2173 (which has since been merged into developer) for a much cleaner history. - This PR is pulling out elements from #4118, #4247, and #4248 which contained completed work by @Sarthak2601. - This PR introduces new logging that didn't exist in the original PRs (#4118, #4247, and #4248) This PR broadly adds all of the analytics event described in the TDD above (& its corresponding PRD), needed for an upcoming user study with the app. It *does* enable actual logging if analytics were to be enabled in the Android manifest (even though the UI side isn't implemented yet--that's planned for #4271). ## 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 N/A -- This PR doesn't make UI changes, and existing flows shouldn't be affected. Commits: * strings for learner analytics * platform parameter impl for learner analytics * nit * nit * event action enum update * addition of contexts * nit * controller level logging and contexts * nit * nit fixes. * nit fixes. * event bundle modifications * sync status, logging identifiers, profile update, lifecycle owner * ui impl: part 1 -- basic * admin control strings * strings correction * strings correction * device id correction * exhaustive when fix. * exhaustive when fix. * todo formatting * nits. * nits. * collapsed contexts, added spacing, added comments * event action removal + nits * tests + dev options event logs fixes post event action removal * nits * removal of method for event action formatted string * nits, null context changes. * nits * reserved fixes and help index fix * bazel imports * bazel build fixes * test fixes * nit * logging identifier controller, module + uuid wrapper, real impl * logging identifier controller tests, fake uuid, tests * sync status manager + fake * logging methods, test setup * profile management, tests * sync status update. * lifecycle observer * Post-merge fixes + Bazel support. * Lots of reorganizing & changes. New tests and documentation have also been added. More broadly, this changes the device ID computation, but actually breaks it so more work will be needed in subsequent commits. * Lint fixes. * Post-merge fix (proper merge of maven_install). * Lint fixes (includes post-merge cleanups). * Lots of stuff. Restructured a lot of the UI, addressed most failing static checks (except KDocs and lint which will be in the follow-up commit), added tests, fixed copying, and generally finished the UI. Sync status seems broken, and it's not yet clear whether events are actually being logged (I need to investigate this). Analytics are disabled in local testing, so that might also be the reason for logs being stuck in an uploading state. * Documentation + lint fixes. This also changes the contract of ClipboardController. * Finish remaining planned tests. * Move over changes from learner-analytics-proto. * Manually pull in changes from 3d6c716. Note that this is operating on a different base). * Post-merge fixes. These at least ensure that the app can build, but many tests will still fail (which is fine seeing as much of this code is going to be split up soon, anyway). Rebase version: app build is no longer guaranteed. * Lint fixes. * Undo all learner analytics changes. I'll be pulling in specific components in specific PRs to organize the changes across 4 PRs. Note that I took this approach to preserve the history from the earlier commits. Those changes will still be included in this PR chain, just a bit awkwardly (i.e. it'll look like I introduced them originally, but that distinction is lost during the squash-and-merge, anyway). * Manually pull in non-app module changes. A bunch of work is still needed to finish these, and I'm still trying to figure out whether I can de-couple the module changes to make reviewing a bit nicer. * Post-merge fixes. All tests verified as building & passing. * Add sync status for no connectivity case. * Remove unnecessary sync manager. * Copy over changes from #4263. These are the domain changes needed for finishing learner analytics support. Cleanup, documentation, and testing all still need to be completed. * Add domain changes for AudioPlayerController. These originate from #4263. * Add missing Javadoc from #4263. * Add module deps for learner analytics components. This ensures that the new domain components introduced in the previous branch can be used throughout the app in both production and test components. * Post-merge fixes from different branch. * Remove all UserIdProdModule references. The module was removed during development in favor of a more integrated approach to managing the logging-related IDs. * Post-merge fix. * Remove references to missing module per #4263. * Add files changes from manual merges/cherry-picks. * Add event log callsites. * Add missing event log from #4263. This one is for audio voiceover playing. * Finish tests & documentation. This also renames 'device ID' to be 'installation ID' for more correctness. * Lint fixes. * Post-merge fixes. * Lint fixes. * Remove files incorrectly readded. * Add follow-up tests to improve coverage. * Post-merge & lint fixes. * Implement tests to verify event logging. These tests only verify event logging at the domain layer, though technically these events will also be logged from UI-based user interactions. Tests to verify that's the case will be added in #4271 as not all aspects of logging are properly hooked up in the UI layer as of this PR. * Update domain's Gradle Mockito version to Bazel's. * Add missing tests & needed behaviors. Fix broken app test. * Fix OS-specific issue in ClipboardController. Co-authored-by: Sarthak Agarwal <[email protected]>
Merged
6 tasks
|
Approved. This is really really awesome implementation. |
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 #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:
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:
From a functional perspective, this PR is introducing:
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). 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:
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
EditTextInputActionto 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.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 likeAndroidJUnit4wherein 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
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
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
Espresso test results
Note that in the case of
MathExpressionInteractionsViewTestthis is the first PR that demonstrates using parameterized tests with Espresso.