feat(common/models): mid-context suggestions & reversions, fix(common/models): correction-search SMP issues #4427

jahorton · 2021-02-05T07:47:05Z

While working on #4411, I noticed that our predictive text has, to this point, often assumed that it will always be operating at the end of the current context. This PR seeks to round out that rough edge and provide support for mid-context scenarios:

accepting suggestions at the end of a word in the middle of the context (easy)
reverting such a suggestion
accepting suggestions in the middle of a word/token
reverting suggestions that were accepted mid-word/token (hard)

Support won't be completely perfect, but it's a definite upgrade from how things were before. The main issue: the caret will always be placed at the end of text affected by a reversion, even if it was originally before some of the reverted characters. (Because a suggestion triggered right-deletions.)

Also note that no post-caret text will actually be used by the predictive text engine, same as before.

…redictions' into feat/common/models/mid-context-suggestions

jahorton · 2021-02-08T05:51:30Z

ios/engine/KMEI/KeymanEngine/Classes/InputViewController.swift

+    if numCharsToRightDelete > 0 {
+      for _ in 0..<numCharsToRightDelete {
+        textDocumentProxy.adjustTextPosition(byCharacterOffset: 1)
+        textDocumentProxy.deleteBackward()
+      }
+    }
+


This should probably be done more properly. It was sufficient for initial testing, at least.

Unfortunately, there is no deleteForward command on that object, hence the text-position shenanigans.

It's not possible to adjustTextPosition(byCharacterOffset: numCharsToRightDelete)?

I am guessing this needs some real testing for interactions with clusters, SMP because it's likely that the cursor cannot be positioned in the middle of a cluster.

I'm sure it's possible to do that... but it may make any SMP-double-checking even rougher.

I'll definitely need to reference this block:

keyman/ios/engine/KMEI/KeymanEngine/Classes/InputViewController.swift

Lines 366 to 378 in 58a2bda

for _ in 0..<numCharsToDelete {

let oldContext = textDocumentProxy.documentContextBeforeInput ?? ""

textDocumentProxy.deleteBackward()

let newContext = textDocumentProxy.documentContextBeforeInput ?? ""

let unitsDeleted = oldContext.utf16.count - newContext.utf16.count

if unitsDeleted > 1 {

if !InputViewController.isSurrogate(oldContext.utf16.last!) {

let lowerIndex = oldContext.utf16.index(oldContext.utf16.startIndex,

offsetBy: newContext.utf16.count)

let upperIndex = oldContext.utf16.index(lowerIndex, offsetBy: unitsDeleted - 1)

textDocumentProxy.insertText(String(oldContext[lowerIndex..<upperIndex]))

}

}

Noting the step-by-step deletion procedure it follows... and the need to compare/contrast Swift's preferred UTF-8 representation and UTF-16, it's probably best to go step-by-step here too.

jahorton · 2021-02-08T06:29:12Z

Android will need a few tweaks to support right-deletions in order to better support mid-token suggestion acceptance - but only in-app. Oddly, it's already supported for the system keyboard... just not the in-app one. Huh.

@darcywong00 Might need your help on this one. I can identify the system-keyboard block that handles this pretty easily:

keyman/android/KMEA/app/src/main/java/com/tavultesoft/kmea/KMManager.java

Lines 2547 to 2559 in 58a2bda

    
           // Perform right-deletions 
        
           for (int i = 0; i < dr; i++) { 
        
             CharSequence chars = ic.getTextAfterCursor(1, 0); 
        
             if (chars != null && chars.length() > 0) { 
        
               char c = chars.charAt(0); 
        
               SystemKeyboardShouldIgnoreSelectionChange = true; 
        
               if (Character.isHighSurrogate(c)) { 
        
                 ic.deleteSurroundingText(0, 2); 
        
               } else { 
        
                 ic.deleteSurroundingText(0, 1); 
        
               } 
        
             } 
        
           }

There simply is no equivalent for the in-app, and the surrounding code is remarkably different between the two cases. All the in-app one does:

keyman/android/KMEA/app/src/main/java/com/tavultesoft/kmea/KMManager.java

Lines 2354 to 2356 in 58a2bda

    
           if(dr != 0) { 
        
             Log.d(TAG, "Right deletions requested but are not presently supported by the in-app keyboard."); 
        
           }

I'd prefer not to make things worse there than they already are, hence the call for help here.

darcywong00 · 2021-02-08T06:55:35Z

There simply is no equivalent for the in-app, and the surrounding code is remarkably different between the two cases. All the in-app one does:

I hope you've got a time machine cause you wrote both right-deletion blocks in #1732 😄

jahorton · 2021-02-08T07:33:37Z

There simply is no equivalent for the in-app, and the surrounding code is remarkably different between the two cases. All the in-app one does:

I hope you've got a time machine cause you wrote both right-deletion blocks in #1732 😄

And I can see why I didn't - take a look at how different the rest of the insertText code is for in-app vs system! Were it up to me, I'd refactor one of the two to use the other's approach if possible.

mcdurdin

I like the improvements in general. I have a few questions, and a concern around testing on iOS (presumably similar on Android although that may be coming later?)

common/predictive-text/worker/model-compositor.ts

mcdurdin · 2021-02-08T22:19:07Z

ios/engine/KMEI/KeymanEngine/Classes/InputViewController.swift

+    if numCharsToRightDelete > 0 {
+      for _ in 0..<numCharsToRightDelete {
+        textDocumentProxy.adjustTextPosition(byCharacterOffset: 1)
+        textDocumentProxy.deleteBackward()
+      }
+    }
+


It's not possible to adjustTextPosition(byCharacterOffset: numCharsToRightDelete)?

I am guessing this needs some real testing for interactions with clusters, SMP because it's likely that the cursor cannot be positioned in the middle of a cluster.

mcdurdin · 2021-02-08T22:21:51Z

ios/engine/KMEI/KeymanEngine/Classes/KeymanWebDelegate.swift

@@ -16,7 +16,7 @@ protocol KeymanWebDelegate: class {
  /// - Parameters:
  ///   - numCharsToDelete: The number of UTF-16 code units to delete before inserting the new text.
  ///   - newText: The string to insert.
-  func insertText(_ keymanWeb: KeymanWebViewController, numCharsToDelete: Int, newText: String)
+  func insertText(_ keymanWeb: KeymanWebViewController, numCharsToLeftDelete: Int, newText: String, numCharsToRightDelete: Int)


I'd have kinda liked this to have a different parameter order:

Suggested change

func insertText(_ keymanWeb: KeymanWebViewController, numCharsToLeftDelete: Int, newText: String, numCharsToRightDelete: Int)

func insertText(_ keymanWeb: KeymanWebViewController, numCharsToLeftDelete: Int, numCharsToRightDelete: Int, newText: String)

From Web:

keyman/web/source/kmwbase.ts

Lines 633 to 638 in 58a2bda

/**

* @param {number} dn Number of pre-caret characters to delete

* @param {string} s Text to insert

* @param {number=} dr Number of post-caret characters to delete

*/

['oninserttext']: (dn: number, s: string, dr?: number) => void;

keyman/web/source/dom/domOverrides.ts

Lines 36 to 38 in 58a2bda

if(keyman.isEmbedded) {

// A special embedded callback used to setup direct callbacks to app-native code.

keyman['oninserttext'](ruleTransform.deleteLeft, ruleTransform.insert, ruleTransform.deleteRight);

From Android:

keyman/android/KMEA/app/src/main/java/com/tavultesoft/kmea/KMManager.java

Lines 2351 to 2353 in 58a2bda

// This annotation is required in Jelly Bean and later:

@JavascriptInterface

public void insertText(final int dn, final String s, final int dr) {

That said, both of these arose during 14.0 - there's no evidence of them in 13.0. In their original versions, they closely matched the iOS function above: deleteLeft, then newText.

Admittedly, the order was chosen b/c it's a new parameter, and new things - especially potentially-undefined parameters (b/c JS/TS) - go on the right-hand side. That said, there is a beauty to the order:

abc de | fg hij

With respect to the caret, we first delete-left, then insert the text at the caret's position, then delete-right if needed after the caret. Though, I suppose that temporal order doesn't particularly matter - it's largely transitive as long as delete-lefts come before text insertion. It does make sense with spatial order, though.

That said... I don't mind changing it... but only if we also change the parameter order in those locations. And I think that's best left to a separate PR.

Okay with that refactor being a separate PR

Tracked as #4529.

mcdurdin · 2021-02-08T22:22:34Z

ios/engine/KMEI/KeymanEngine/Classes/KeymanWebViewController.swift

-      // Use it when we're ready to implement that.
-      // Our .insertText will need to be adjusted accordingly.
-      _ = Int(fragment[drRange.upperBound...])!
+      let dr = Int(fragment[drRange.upperBound...])!


Suggested change

let dr = Int(fragment[drRange.upperBound...])!

let numCharsToRightDelete = Int(fragment[drRange.upperBound...])!

Perhaps could rename numCharsToDelete to numCharsToLeftDelete as well?

mcdurdin · 2021-02-08T22:25:29Z

common/predictive-text/worker/model-compositor.ts

+    if(postContextTokenization) {
+      // Handles display string for reversions triggered by accepting a suggestion mid-token.
+      revertedPrefix = postContextTokenization.left[postContextTokenization.left.length-1];
+      revertedPrefix += postContextTokenization.caretSplitsToken ? postContextTokenization.right[0] : '';


Can we be certain that postContextTokenization.right always contains at least one element?

If postContextTokenization.caretSplitsToken == true, yes. Otherwise, no.

mcdurdin · 2021-02-08T22:27:01Z

common/predictive-text/worker/model-compositor.ts

+      suggestions.forEach(function(suggestion) {
+        // A reversion's transform ID is the additive inverse of its original suggestion;
+        // we revert to the state of said original suggestion.
+        suggestion.transformId = -reversion.transformId;
+      });


Given this is repeated on linees 565-569, do you want to extract it into a function?

mcdurdin · 2021-02-08T22:30:03Z

common/core/web/input-processor/src/text/prediction/languageProcessor.ts

@@ -255,7 +255,7 @@ namespace com.keyman.text.prediction {
            // the input will be automatically rewound to the preInput state.
            transform: original.transform,
            // The ID part is critical; the reversion can't be applied without it.
-            transformId: original.token, // reversions use the additive inverse.
+            transformId: -original.token, // reversions use the additive inverse.


I'm trying to understand the significance of this change. Is it a bug fix -- the comment suggests as such? Or is it just to support the other changes you are making now?

Bug fix. I think at some point, the intent was to use the base ID (which already used the additive inverse), but transformID is the field in active use for ID checks now. Not sure when reversions broke. Anyway, that comment was very important in figuring out why things were broken.

…redictions' into feat/common/models/mid-context-suggestions

jahorton · 2021-02-09T08:48:33Z

Right now, iOS seems covered, even for SMP cases... except for reverting suggestions that were accepted mid-word. (The reversions aren't showing up at the moment; it's related to async [sadly] context-reset ops within the iOS Keyman engine.)

Android in-app also still needs work.

keyman-server · 2021-02-11T18:01:01Z

~~Changes in this pull request will be available for download in Keyman version 14.0.242-beta~~

jahorton · 2021-02-12T01:22:40Z

Well... thanks, Apple:

We were definitely right to be concerned about how clusters would be handled. For those who can't read Khmer script, that's a four-character jump. (Three of them visible.)

So, my assumption later in the loop for repositioning the caret is incorrect, causing issues on later loop iterations.

jahorton · 2021-02-12T03:13:04Z

Okay, I've got a fair bit of the core worked out there, though there's something really weird going on now. There seems to be a desync between the text-manipulation method and what actually gets output - of course, only when right-deletions are happening.

So, let's take this as our starting point:

I've confirmed via temporary debug-log statements that this, according to the textDocumentProxy object used for text manipulation, has an expected final context of ម្រាយ បន្ស៊ី . Exactly what a user would expect. So, of course, what do we get?

Possibility 1

Uh... that's not what textDocumentProxy told us we'd get. The heck?

Possibility 2

(Note: these screenshots were taken from a clean context, rather than with the English text present at the start.)

Uh... what, mate? Didn't even do anything?

Turns out, it actually did. If you hit BKSP, it'll remove the hidden 'subconsonant' marker. Alternatively, if you reselect the same suggestion again...

And again...

So... the true result incrementally inches closer to the desired suggestion. Wha?

Again, note that in both cases, the actual text-manipulation handling itself computes the correct text immediately, and even the textDocumentProxy confirms this. Something is interfering with this process. The question is... is there some yet-undiscovered bug in our code that has only appeared now, and only for right-deletions at that... or is it an Apple-side bug?

There's also the fact that the result isn't even 100% predictable, as noted by the two variations seen above!

…-context-suggestions

jahorton · 2021-02-25T07:48:22Z

Since the iOS engine is having trouble with right-deletions and a resolution is proving tricky, I've gone ahead and turned off predictive text's right-deletions for now. Instead, any suggestions accepted mid-token will insert a standard word-break afterward. (Note: this is a perfect match for the behavior of iOS's default predictive text; it doesn't right-delete.)

I can simply add the right-deletion aspect as a 'feature request' for the future, allowing us to revisit it at another time.

I have tried a few other approaches, and one seemed to get remarkably close much of the time... the issue being that it also gave way worse results some of the other times. So... yeah, not changing it over until it's stable.

mcdurdin · 2021-02-26T00:06:19Z

I can simply add the right-deletion aspect as a 'feature request' for the future, allowing us to revisit it at another time.

Sounds good to me. Have you opened an issue for this yet?

jahorton · 2021-02-26T01:15:18Z

I can simply add the right-deletion aspect as a 'feature request' for the future, allowing us to revisit it at another time.

Sounds good to me. Have you opened an issue for this yet?

It's now up as #4538.

jahorton · 2021-02-26T03:40:16Z

Code related to the new issue (for the deferred right-deletion functionality) has been split off into #4541.

Note that the most recent commit here (which reverted them for this PR) was hand-written, with #4541's first commit a reversion of that.

mcdurdin

LGTM.

Appreciate you going the extra mile on this one -- it's been a bit of a challenge to get it right given the trickiness of the iOS API around right-deletion.

If I was going to be really nitpicky, I'd suggest reverting the whitespace only change in InputViewController.swift so that we have no changes to the Swift code at all, but it's pretty unimportant!

keyman-server · 2021-02-26T18:01:22Z

Changes in this pull request will be available for download in Keyman version 14.0.248-beta

MakaraSok · 2021-03-02T08:40:57Z

Retest on Android 10 (on both emulator and physical device) based on #4427 (comment):

"accepting suggestions in the middle of a word/token" does not delete the post-half of the word, even though this is the intention, it is not quite helpful because the post-half is not intelligible and has to be manually delete anyway.
I like the ability to switch back and forth to the suggested word automatically when tapped on. For Khmer language, it seems like there is no space after the word after a suggestion is chosen in a new line:

Khmer Angkor - in the first line, a space is seen after a suggestion is chosen; in the second line, it takes three taps on the spacebar to get a regular spacẹ
https://www.youtube.com/watch?v=VcaL1R7X-L8

EuroLatin (SIL) - no space after the chosen suggestion, it takes two taps on the spacebar to output a regular space
https://youtu.be/lC7ZLZ94ALw
The globe key on the emulator does not respond as expected -- making it impossible to switch between keyboard unless doing it from within Keyman app
https://youtu.be/1p3qXUun3aA

For any more specific test, ping me again. :)

keyman-server · 2021-03-09T18:09:34Z

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

jahorton added 5 commits February 3, 2021 10:59

fix(common/models): adds wordbreak text only at end of context

e505963

feat(common/models): mid-context suggestion acceptance

138a7db

fix(common/models): fixes reversion application

2b999d6

fix(common/models): mid-context post-revert suggestion acceptance

e1bab44

fix(common/models): branch polishing, unit test fix

861d1b5

jahorton added web/ ios/ common/ common/models/ common/core/ common/web/ labels Feb 5, 2021

jahorton added this to the B14S5 milestone Feb 5, 2021

github-actions bot added ios/engine/ feat labels Feb 5, 2021

darcywong00 modified the milestones: B14S5, B14S6 Feb 8, 2021

jahorton added 2 commits February 8, 2021 09:31

chore(common/models): Merge branch 'fix/common/models/context-reset-p…

a2adfd7

…redictions' into feat/common/models/mid-context-suggestions

chore(common/models): Merge branch 'fix/common/models/context-reset-p…

0483f3e

…redictions' into feat/common/models/mid-context-suggestions

jahorton commented Feb 8, 2021

View reviewed changes

jahorton marked this pull request as ready for review February 8, 2021 06:29

mcdurdin reviewed Feb 8, 2021

View reviewed changes

jahorton added 3 commits February 9, 2021 09:22

docs(common/models): removes defunct comment-doc

f0f1ae3

fix(ios): better SMP right-deletion handling

6973f4c

chore(common/models): Merge branch 'fix/common/models/context-reset-p…

8334e49

…redictions' into feat/common/models/mid-context-suggestions

Base automatically changed from fix/common/models/context-reset-predictions to beta February 9, 2021 08:46

fix(common/models): correction-search smp issues

785b950

mcdurdin mentioned this pull request Feb 11, 2021

bug(common): history collater not taking base branch into account on PR merges #4483

Closed

jahorton added 2 commits February 12, 2021 09:56

fix(ios): nixes source of (some) duplicate context resets

e14b689

fix(common/models): better right-deletion logic

d574d1e

jahorton mentioned this pull request Feb 12, 2021

bug(iOS): UI issues when directly installing a single-language lexical model KMP #4493

Closed

mcdurdin modified the milestones: B14S6, B14S7 Feb 22, 2021

jahorton mentioned this pull request Feb 22, 2021

chore: merge B14S6 beta to alpha #4503

Merged

jahorton added 3 commits February 25, 2021 09:16

chore(common/models): Merge branch 'beta' into feat/common/models/mid…

28ed690

…-context-suggestions

change(common/models): disables right-deletion for now

8516561

docs(common/models): adds comments about bug in iOS insertText

fc2a178

jahorton mentioned this pull request Feb 25, 2021

chore(web): change insertText method parameter ordering #4529

Closed

jahorton mentioned this pull request Feb 26, 2021

feat(common/models): accepting a suggestion mid-word should right-delete text after the caret #4538

Open

chore: reverts right-delete changes

a232f32

jahorton mentioned this pull request Feb 26, 2021

feat(android, ios): suggestions applied mid-word right-delete remnant of word #4541

Closed

mcdurdin approved these changes Feb 26, 2021

View reviewed changes

jahorton merged commit c7cf3d8 into beta Feb 26, 2021

jahorton deleted the feat/common/models/mid-context-suggestions branch February 26, 2021 05:11

jahorton mentioned this pull request Mar 5, 2021

bug(common/models): Predictive text handling of BKSP input leaves much to be desired #3730

Closed

mcdurdin added the has-user-test label Feb 12, 2022

jahorton mentioned this pull request Apr 18, 2023

CLDR-16603 kbd: transform DTD changes unicode-org/cldr#2762

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(common/models): mid-context suggestions & reversions, fix(common/models): correction-search SMP issues #4427

feat(common/models): mid-context suggestions & reversions, fix(common/models): correction-search SMP issues #4427

jahorton commented Feb 5, 2021 •

edited

jahorton Feb 8, 2021

mcdurdin Feb 8, 2021

jahorton Feb 9, 2021

jahorton commented Feb 8, 2021 •

edited

darcywong00 commented Feb 8, 2021

jahorton commented Feb 8, 2021 •

edited

mcdurdin left a comment

mcdurdin Feb 8, 2021

mcdurdin Feb 8, 2021

jahorton Feb 9, 2021

mcdurdin Feb 9, 2021

jahorton Feb 25, 2021

mcdurdin Feb 8, 2021

mcdurdin Feb 8, 2021

jahorton Feb 9, 2021

mcdurdin Feb 8, 2021

mcdurdin Feb 8, 2021

jahorton Feb 9, 2021

jahorton commented Feb 9, 2021

keyman-server commented Feb 11, 2021 •

edited by mcdurdin

jahorton commented Feb 12, 2021

jahorton commented Feb 12, 2021

jahorton commented Feb 25, 2021

mcdurdin commented Feb 26, 2021

jahorton commented Feb 26, 2021

jahorton commented Feb 26, 2021 •

edited

mcdurdin left a comment

keyman-server commented Feb 26, 2021

MakaraSok commented Mar 2, 2021

keyman-server commented Mar 9, 2021

	for _ in 0..<numCharsToDelete {
	let oldContext = textDocumentProxy.documentContextBeforeInput ?? ""
	textDocumentProxy.deleteBackward()
	let newContext = textDocumentProxy.documentContextBeforeInput ?? ""
	let unitsDeleted = oldContext.utf16.count - newContext.utf16.count
	if unitsDeleted > 1 {
	if !InputViewController.isSurrogate(oldContext.utf16.last!) {
	let lowerIndex = oldContext.utf16.index(oldContext.utf16.startIndex,
	offsetBy: newContext.utf16.count)
	let upperIndex = oldContext.utf16.index(lowerIndex, offsetBy: unitsDeleted - 1)
	textDocumentProxy.insertText(String(oldContext[lowerIndex..<upperIndex]))
	}
	}

	func insertText(_ keymanWeb: KeymanWebViewController, numCharsToLeftDelete: Int, newText: String, numCharsToRightDelete: Int)
	func insertText(_ keymanWeb: KeymanWebViewController, numCharsToLeftDelete: Int, numCharsToRightDelete: Int, newText: String)

	/**
	* @param {number} dn Number of pre-caret characters to delete
	* @param {string} s Text to insert
	* @param {number=} dr Number of post-caret characters to delete
	*/
	['oninserttext']: (dn: number, s: string, dr?: number) => void;

	if(keyman.isEmbedded) {
	// A special embedded callback used to setup direct callbacks to app-native code.
	keyman['oninserttext'](ruleTransform.deleteLeft, ruleTransform.insert, ruleTransform.deleteRight);

	// This annotation is required in Jelly Bean and later:
	@JavascriptInterface
	public void insertText(final int dn, final String s, final int dr) {

	let dr = Int(fragment[drRange.upperBound...])!
	let numCharsToRightDelete = Int(fragment[drRange.upperBound...])!

feat(common/models): mid-context suggestions & reversions, fix(common/models): correction-search SMP issues #4427

feat(common/models): mid-context suggestions & reversions, fix(common/models): correction-search SMP issues #4427

Conversation

jahorton commented Feb 5, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jahorton commented Feb 8, 2021 • edited

darcywong00 commented Feb 8, 2021

jahorton commented Feb 8, 2021 • edited

mcdurdin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jahorton commented Feb 9, 2021

keyman-server commented Feb 11, 2021 • edited by mcdurdin

jahorton commented Feb 12, 2021

jahorton commented Feb 12, 2021

jahorton commented Feb 25, 2021

mcdurdin commented Feb 26, 2021

jahorton commented Feb 26, 2021

jahorton commented Feb 26, 2021 • edited

mcdurdin left a comment

Choose a reason for hiding this comment

keyman-server commented Feb 26, 2021

MakaraSok commented Mar 2, 2021

keyman-server commented Mar 9, 2021

jahorton commented Feb 5, 2021 •

edited

jahorton commented Feb 8, 2021 •

edited

jahorton commented Feb 8, 2021 •

edited

keyman-server commented Feb 11, 2021 •

edited by mcdurdin

jahorton commented Feb 26, 2021 •

edited