Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(web): correct edit-path ordering at source 🖲️ #12925

Merged
merged 2 commits into from
Feb 3, 2025

Conversation

jahorton
Copy link
Contributor

Rather than correcting the context-matching edit-path after it's generated, what if we just... got it right the first time?

Finally decided to really look into the cause and remedy this... and add a unit test while I was at it.

@keymanapp-test-bot skip

@jahorton jahorton requested a review from mcdurdin as a code owner January 17, 2025 03:27
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Jan 17, 2025

User Test Results

Test specification and instructions

User tests are not required

@keymanapp-test-bot keymanapp-test-bot bot added this to the A18S19 milestone Jan 17, 2025
@jahorton jahorton changed the title fix(web): correct edit-path ordering at source fix(web): correct edit-path ordering at source 🖲️ Jan 17, 2025
@darcywong00 darcywong00 modified the milestones: A18S19, A18S20 Jan 18, 2025
Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the implications of this? What problems was it causing?

@jahorton
Copy link
Contributor Author

What are the implications of this? What problems was it causing?

Note the changes in context-tracker.ts.

In particular, the predictive-text "context-tracking" engine seeks to align contexts before and after changes based on preserving a central band that doesn't change, then a thin area that might be altered, and only then areas that have deletions and insertions.

The ordering of the edit path, as it was before, required extra logic to "correct" this ordering on the right-hand side, near the text insertion point. Suppose a degenerate keystroke were added that emitted the following string: a b c d. In such a case, the logic in context-tracker.ts would not have caught it - there are too many whitespaces in there. Also, suppose a future in which we offer multi-word predictions - the same would apply then.

The "correction" logic was unable to handle these cases, and I originally did extend it - and the results are more complex than what is seen here. Then I realized, after a bit of inspection... that I could fix the underlying problem at its source, and in a manner that operated more simply and concisely.


This was giving me a few issues during at least one stage of development for #12884.

@darcywong00 darcywong00 modified the milestones: A18S20, B18S1 Feb 1, 2025
Base automatically changed from change/web/context-tracker-backspaces-and-alignment to master February 3, 2025 08:10
@jahorton jahorton merged commit 3c7ae5d into master Feb 3, 2025
17 checks passed
@jahorton jahorton deleted the fix/web/context-match-path-ordering branch February 3, 2025 08:10
@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 18.0.183-alpha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants