change(common/models): context tracking within the lm-worker 🐵#9855
Merged
jahorton merged 3 commits intofeature-gesturesfrom Nov 6, 2023
Merged
Conversation
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
mcdurdin
approved these changes
Oct 26, 2023
…to change/common/models/pred-text-context-tracking
Base automatically changed from
feat/web/test-kbd-prototyping
to
feature-gestures
November 6, 2023 01:27
This was referenced Dec 5, 2023
This file contains hidden or 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
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.
This PR changes the underlying assumptions of the predictive-text worker, generalizing the cases it can handle.
In particular, the worker no longer assumes perfect continuity and synchronization of context with its KeymanWeb host. This is necessary due to interactions with multitaps, as those need "context rewind" operations that the worker is not privy to.
Many of the changes are due to significant instance reuse that was "fine" under the assumptions we held until now, but that are no longer viable. Thus, the new clone-constructors. We can still make significant re-use of previous calculations... but we need to make sure those calculations don't have unwanted cross-effects should an unexpected context-rewind occur.
In previous versions of Keyman and our predictive-text worker, we assumed that...
Previously, step 2 above was only triggered by two causes:
The issue is: a multitap...
Thus, supporting multitaps invalidates the assumptions at the top if things are left as they are... and my experience with testing #9740 shows that moderate multitap use will rapidly break predictive text if we don't correct these assumptions.
While it would be possible to instead update the worker on the results of a multitap... I personally find that path to be significantly less straightforward. (For one thing, reverting a suggestion always crossed 'context token' boundaries, thanks to whitespace. Not so with multitaps!) There's a significant chance said path would require new APIs into the worker, unlike this solution.
@keymanapp-test-bot skip