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(ios): multitap consistency after new-lines #10728

Merged
merged 6 commits into from
Feb 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ open class InputViewController: UIInputViewController, KeymanWebDelegate {

let range = NSRange(location: before.unicodeScalars.count, length: 0)

self.setContextState(text: contextWindowText, range: range)
self.setContextState(text: contextWindowText, range: range, doSync: true)
}

updater(preCaretContext, postCaretContext)
Expand Down Expand Up @@ -606,7 +606,7 @@ open class InputViewController: UIInputViewController, KeymanWebDelegate {
* will be expecting SMP-aware measurements. Swift's `.unicodeScalars`
* property on `String`s lines up best with this.
*/
func setContextState(text: String?, range: NSRange) {
func setContextState(text: String?, range: NSRange, doSync: Bool = false) {
// Check for any LTR or RTL marks at the context's start; if they exist, we should
// offset the selection range.
let characterOrderingChecks = [ "\u{200e}" /*LTR*/, "\u{202e}" /*RTL 1*/, "\u{200f}" /*RTL 2*/ ]
Expand All @@ -626,10 +626,7 @@ open class InputViewController: UIInputViewController, KeymanWebDelegate {
selRange = NSRange(location: selRange.location - 1, length: selRange.length)
}

keymanWeb.setText(context)
if range.location != NSNotFound {
keymanWeb.setCursorRange(selRange)
}
keymanWeb.setContext(text: context, range: selRange, doSync: doSync)
}

func resetKeyboardState() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -223,14 +223,7 @@ extension KeymanWebViewController {
setSpacebarText(userData.optSpacebarText)
}

func setCursorRange(_ range: NSRange) {
if range.location != NSNotFound {
webView!.evaluateJavaScript("setCursorRange(\(range.location),\(range.length));", completionHandler: nil)
self.currentCursorRange = range
}
}

func setText(_ text: String?) {
func setContext(text: String?, range: NSRange?, doSync: Bool = false) {
var text = text ?? ""

// Remove any system-added LTR/RTL marks.
Expand All @@ -246,7 +239,16 @@ extension KeymanWebViewController {
let jsonText = jsonString[start..<end]

self.currentText = String(jsonText)
webView!.evaluateJavaScript("setKeymanVal(\"\(jsonText)\");", completionHandler: nil)

var finalRange = range ?? self.currentCursorRange ?? nil
self.currentCursorRange = finalRange

//
if (finalRange?.location ?? NSNotFound) != NSNotFound {
webView!.evaluateJavaScript("setKeymanContext(\"\(jsonText)\", \(doSync ? "true" : "false"), \(finalRange!.location), \(finalRange!.length));", completionHandler: nil)
} else {
webView!.evaluateJavaScript("setKeymanContext(\"\(jsonText)\", \(doSync ? "true" : "false"));", completionHandler: nil)
}
} catch {
os_log("%{public}s", log: KeymanEngineLogger.engine, type: .error, error.localizedDescription)
SentryManager.capture(error.localizedDescription)
Expand Down Expand Up @@ -363,7 +365,7 @@ extension KeymanWebViewController {
if let packageID = lexicalModel.packageID {
event.extra?["package"] = packageID
}

os_log("%{public}s: %{public}s", log: KeymanEngineLogger.resources, type: .error, errorMessage, lexicalModel.id)
SentryManager.capture(event)
throw KeyboardError.fileMissing
Expand Down Expand Up @@ -572,7 +574,7 @@ extension KeymanWebViewController: WKScriptMessageHandler {
// This may need filtering for proper use with Sentry?
// Then again, if KMW is logging it... we already have to worry
// about it showing up in Web-oriented Sentry logs.

let logMessage = "KMW Log: \(message)"
os_log("%{public}s", log: KeymanEngineLogger.engine, type: .info, logMessage)
SentryManager.breadcrumb(logMessage)
Expand Down Expand Up @@ -672,10 +674,7 @@ extension KeymanWebViewController: KeymanWebDelegate {
resizeKeyboard()

// There may have been attempts to set these values before the keyboard loaded!
self.setText(self.currentText)
if let cursorRange = self.currentCursorRange {
self.setCursorRange(cursorRange)
}
self.setContext(text: self.currentText, range: self.currentCursorRange)

var newKb = Manager.shared.currentKeyboard

Expand Down
22 changes: 0 additions & 22 deletions ios/engine/KMEI/KeymanEngine/Classes/TextField.swift
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ public class TextField: UITextField, KeymanResponder {
public var isInputClickSoundEnabled = true

private var delegateProxy: TextFieldDelegateProxy!
private var shouldUpdateKMText = false

private var keyboardChangedObserver: NotificationObserver?

Expand Down Expand Up @@ -52,8 +51,6 @@ public class TextField: UITextField, KeymanResponder {

self.inputView = Manager.shared.inputViewController.view

NotificationCenter.default.addObserver(self, selector: #selector(self.textFieldTextDidChange),
name: UITextField.textDidChangeNotification, object: self)
keyboardChangedObserver = NotificationCenter.default.addObserver(forName: Notifications.keyboardChanged,
observer: self,
function: TextField.keyboardChanged)
Expand Down Expand Up @@ -155,19 +152,6 @@ public class TextField: UITextField, KeymanResponder {
@objc func enableInputClickSound() {
isInputClickSoundEnabled = true
}

@objc func textFieldTextDidChange(_ notification: Notification) {
if shouldUpdateKMText {
// Catches copy/paste operations
let textRange = selectedTextRange!
let newRange = NSRange(location: offset(from: beginningOfDocument, to: textRange.start),
length: offset(from: textRange.start, to: textRange.end))

Manager.shared.setContextState(text: text, range: newRange)
// This is called when editing in-app; do not reset context here.
shouldUpdateKMText = false
}
}
}

extension KeymanResponder where Self: TextField {
Expand Down Expand Up @@ -196,12 +180,6 @@ extension KeymanResponder where Self: TextField {

// MARK: - UITextFieldDelegate
extension TextField: UITextFieldDelegate {
public func textField(_ textField: UITextField, shouldChangeCharactersIn range: NSRange,
replacementString string: String) -> Bool {
shouldUpdateKMText = true // Enable text update to catch copy/paste operations
return true
}

public func textFieldShouldClear(_ textField: UITextField) -> Bool {
if textField == self {
Manager.shared.clearText()
Expand Down
16 changes: 0 additions & 16 deletions ios/engine/KMEI/KeymanEngine/Classes/TextView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -262,22 +262,6 @@ extension TextView: UITextViewDelegate {
os_log("%{public}s", log:KeymanEngineLogger.ui, type: .debug, messageTwo)
}

public func textView(_ textView: UITextView, shouldChangeTextIn range: NSRange,
replacementText text: String) -> Bool {
// Enable text update to catch copy/paste operations
shouldUpdateKMText = true
return true
}

public func textViewDidChange(_ textView: UITextView) {
if shouldUpdateKMText {
// Catches copy/paste operations
Manager.shared.setContextState(text: textView.text, range: textView.selectedRange)
// This is called when editing in-app; do not reset context here.
shouldUpdateKMText = false
}
}

public func textViewShouldEndEditing(_ textView: UITextView) -> Bool {
if textView == self {
resignFirstResponder()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -276,33 +276,21 @@ function doResetContext() {
keyman.resetContext();
}

function setCursorRange(pos, length) {
var context = keyman.context;

//console.log('setCursorRange('+pos+', '+length+')');

var start = pos;
var end = pos + length;

if(context.selStart != start || context.selEnd != end) {
keyman.context.setSelection(start, end);
keyman.resetContext();
}
}

function setKeymanVal(text) {
//console.log('setKeymanVal('+JSON.stringify(text)+')');

function setKeymanContext(text, doSync, selStart, selLength) {
// console.log(`setKeymanContext(${JSON.stringify(text)}, ${doSync}, ${pos}, ${length})`);
if(text == undefined) {
text = '';
}

if(keyman.context.getText() != text) {
keyman.context.setText(text);
// Both pos + length are optional parameters.
// undefined + <number> => NaN; undefined + undefined => NaN.
let selEnd = selStart + selLength;
selEnd = isNaN(selEnd) ? undefined : selEnd;

const shouldReset = keyman.context.updateContext(text, selStart, selEnd);
if(!doSync || shouldReset) {
keyman.resetContext();
}

return keyman.context.getText();
Copy link
Member

Choose a reason for hiding this comment

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

Was this never used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Here's the only thing calling it:

webView!.evaluateJavaScript("setKeymanVal(\"\(jsonText)\");", completionHandler: nil)

Note how nothing is checking for a result. To be extra clear here, that would be done by the second argument if one were provided.

https://developer.apple.com/documentation/webkit/wkwebview/1415017-evaluatejavascript

No callback means no need for a return value.

}

function executePopupKey(keyID, keyText) {
Expand Down
7 changes: 0 additions & 7 deletions web/src/app/webview/src/configuration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,13 +58,6 @@ export class WebviewConfiguration extends EngineConfiguration {

return baseReport;
}

onRuleFinalization(ruleBehavior: RuleBehavior) {
if(!isEmptyTransform(ruleBehavior.transcription?.transform)) {
const transform = ruleBehavior.transcription.transform;
this.oninserttext(transform.deleteLeft, transform.insert, transform.deleteRight ?? 0);
}
}
}

export interface WebviewInitOptionSpec extends InitOptionSpec {
Expand Down
71 changes: 61 additions & 10 deletions web/src/app/webview/src/contextManager.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
import { type Keyboard, Mock, OutputTarget } from '@keymanapp/keyboard-processor';
import { type Keyboard, Mock, OutputTarget, Transcription, findCommonSubstringEndIndex } from '@keymanapp/keyboard-processor';
import { KeyboardStub } from 'keyman/engine/package-cache';
import { ContextManagerBase, ContextManagerConfiguration } from 'keyman/engine/main';
import { WebviewConfiguration } from './configuration.js';

export type OnInsertTextFunc = (deleteLeft: number, text: string, deleteRight: number) => void;

class ContextHost extends Mock {
export class ContextHost extends Mock {
readonly oninserttext?: OnInsertTextFunc;
private savedState: Mock;

constructor(oninserttext: OnInsertTextFunc) {
super();
Expand All @@ -15,21 +16,70 @@ class ContextHost extends Mock {

apply(transform: Transform): void {
super.apply(transform);
this.updateHost();
}

// Signal the necessary text changes to the embedding app, if it exists.
if(this.oninserttext) {
this.oninserttext(transform.deleteLeft, transform.insert, transform.deleteRight);
updateHost(transcription?: Transcription): void {
const savedState = this.savedState;

if(this.savedState) {
let transform = null;

if(transcription) {
const preInput = transcription.preInput;
// If our saved state matches the `preInput` from the incoming transcription, just reuse its transform.
// Will generally not match during multitap operations, though.
//
// Helps ensure backspaces pass through even if we don't currently have text available in context for them.
if(preInput.text == savedState.text && preInput.selStart == savedState.selStart && preInput.selEnd == savedState.selEnd) {
transform = transcription.transform;
}
}

transform ||= this.buildTransformFrom(this.savedState);

// Signal the necessary text changes to the embedding app, if it exists.
if(this.oninserttext) {
this.oninserttext(transform.deleteLeft, transform.insert, transform.deleteRight);
}
}

// Save the current context state for use in future diffs.
this.savedState = Mock.from(this);
}

restoreTo(original: OutputTarget): void {
const reversionTransform = original.buildTransformFrom(this);

this.savedState = Mock.from(this);
super.restoreTo(original);
}

updateContext(text: string, selStart: number, selEnd: number): boolean {
let shouldResetContext = false;
if(text != this.text) {
let tempMock = new Mock(text, selStart ?? text._kmwLength(), selEnd ?? text._kmwLength());

let newLeft = tempMock.getTextBeforeCaret();
let oldLeft = this.getTextBeforeCaret();

if(this.oninserttext) {
this.oninserttext(reversionTransform.deleteLeft, reversionTransform.insert, reversionTransform.deleteRight);
let unexpectedBeforeCharCount = findCommonSubstringEndIndex(newLeft, oldLeft, true) + 1;
shouldResetContext = !!unexpectedBeforeCharCount;
}

if(shouldResetContext) {
this.text = text;
}

if(selStart === undefined || selEnd === undefined) {
// Regardless of keyboard, we should check the SMP-aware length of the string.
// Our host app will not know whether or not the keyboard uses SMP chars,
// and we want a consistent interface for context synchronization between
// host app + app/webview KMW.
this.setSelection(this.text._kmwLength());
}

this.savedState = Mock.from(this);

return shouldResetContext;
}

// In app/webview, apps are expected to immediately update the selection range AFTER
Expand All @@ -40,7 +90,8 @@ class ContextHost extends Mock {
// Our host app will not know whether or not the keyboard uses SMP chars,
// and we want a consistent interface for context synchronization between
// host app + app/webview KMW.
this.setSelection(this.text.kmwLength());
this.setSelection(this.text._kmwLength());
this.savedState = Mock.from(this);
}
}

Expand Down
9 changes: 7 additions & 2 deletions web/src/app/webview/src/keymanEngine.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import { DefaultRules, DeviceSpec } from '@keymanapp/keyboard-processor'
import { DefaultRules, DeviceSpec, RuleBehavior, isEmptyTransform } from '@keymanapp/keyboard-processor'
import { KeymanEngine as KeymanEngineBase, KeyboardInterface } from 'keyman/engine/main';
import { AnchoredOSKView, ViewConfiguration, StaticActivator } from 'keyman/engine/osk';
import { getAbsoluteX, getAbsoluteY } from 'keyman/engine/dom-utils';
import { type KeyboardStub, toPrefixedKeyboardId, toUnprefixedKeyboardId } from 'keyman/engine/package-cache';

import { WebviewConfiguration, WebviewInitOptionDefaults, WebviewInitOptionSpec } from './configuration.js';
import ContextManager from './contextManager.js';
import ContextManager, { ContextHost } from './contextManager.js';
import PassthroughKeyboard from './passthroughKeyboard.js';
import { buildEmbeddedGestureConfig, setupEmbeddedListeners } from './oskConfiguration.js';

Expand All @@ -15,6 +15,11 @@ export default class KeymanEngine extends KeymanEngineBase<WebviewConfiguration,
// be compiled down to an IIFE.
constructor(worker: Worker, sourceUri: string) {
const config = new WebviewConfiguration(sourceUri); // currently set to perform device auto-detect.

config.onRuleFinalization = (ruleBehavior: RuleBehavior) => {
(this.context as ContextHost).updateHost(ruleBehavior.transcription);
}

config.stubNamespacer = (stub) => {
// If the package has not yet been applied as namespacing...
if(stub.KP && stub.KI.indexOf(`${stub.KP}::`) == -1) {
Expand Down
Loading