From 2938ee110b7a0be8ee7dd5bd6fcc11e662e30afa Mon Sep 17 00:00:00 2001 From: "Joshua A. Horton" Date: Thu, 15 Aug 2024 10:12:27 +0700 Subject: [PATCH 1/2] refactor(web): remove engine/interfaces dependency on engine/js-processor --- package-lock.json | 24 ++++++------ web/README.md | 17 ++++----- web/build.sh | 3 ++ .../prediction/languageProcessor.interface.ts | 2 +- .../src/prediction/predictionContext.ts | 38 +++---------------- web/src/engine/interfaces/tsconfig.json | 1 - .../engine/js-processor/src/outputTarget.ts | 3 +- web/src/engine/keyboard/src/index.ts | 1 + .../main/src/headless/languageProcessor.ts | 1 + web/src/engine/main/src/keymanEngine.ts | 23 ++++++++++- .../prediction/predictionContext.spec.js | 32 ++++++---------- 11 files changed, 67 insertions(+), 78 deletions(-) diff --git a/package-lock.json b/package-lock.json index ae0082784f2..5a3c53081d0 100644 --- a/package-lock.json +++ b/package-lock.json @@ -228,18 +228,6 @@ "typescript": "^5.4.5" } }, - "web/src/tools/testing/recorder-core": { - "name": "@keymanapp/recorder-core", - "license": "MIT", - "dependencies": { - "@keymanapp/keyman-version": "*", - "@keymanapp/models-types": "*", - "@keymanapp/web-utils": "*" - }, - "devDependencies": { - "typescript": "^5.4.5" - } - }, "common/web/sentry-manager": { "name": "@keymanapp/web-sentry-manager", "license": "MIT", @@ -14544,6 +14532,18 @@ "jsdom": "^23.0.1", "mocha": "^10.0.0" } + }, + "web/src/tools/testing/recorder-core": { + "name": "@keymanapp/recorder-core", + "license": "MIT", + "dependencies": { + "@keymanapp/keyman-version": "*", + "@keymanapp/models-types": "*", + "@keymanapp/web-utils": "*" + }, + "devDependencies": { + "typescript": "^5.4.5" + } } } } diff --git a/web/README.md b/web/README.md index 7cd9e02f002..b5f0c7c4c4c 100644 --- a/web/README.md +++ b/web/README.md @@ -81,11 +81,11 @@ title: Dependency Graph %%{init: {"flowchart": {"htmlLabels": false}} }%% graph TD; OSK["/web/src/engine/osk"]; - KP["/web/src/engine/keyboard"]; + KeyboardSpec["/web/src/engine/keyboard"]; JSProc["/web/src/engine/js-processor"]; - OSK-->KP; + OSK-->KeyboardSpec; WebUtils["@keymanapp/web-utils
(/common/web/utils)"]; - KP---->WebUtils; + KeyboardSpec---->WebUtils; Wordbreakers["@keymanapp/models-wordbreakers
(/common/models/wordbreakers)"]; Models["@keymanapp/models-templates
(/common/models/templates)"]; Models-->WebUtils; @@ -107,15 +107,15 @@ graph TD; subgraph Headless["`**Headless** Fully headless components`"] direction LR - KP; - JSProc-->KP; + KeyboardSpec; + JSProc-->KeyboardSpec; WebUtils; PredText; Gestures; end subgraph ClassicWeb["`**ClassicWeb** - Previously unmodularized components`"] + Intermediate-level engine modules`"] Device["/web/src/engine/device-detect"]; Device----->WebUtils; Elements["/web/src/engine/element-wrappers"]; @@ -124,12 +124,11 @@ graph TD; KeyboardCache-->Interfaces; DomUtils["/web/src/engine/dom-utils"]; DomUtils-->WebUtils; - DomUtils-->KP; + DomUtils-->KeyboardSpec; OSK-->DomUtils; OSK-->Gestures; Interfaces["/web/src/engine/interfaces"]; - Interfaces-->KP; - Interfaces-->JSProc; + Interfaces-->KeyboardSpec; OSK-->Interfaces; CommonEngine["/web/src/engine/main"]; CommonEngine-->Device; diff --git a/web/build.sh b/web/build.sh index c3e17e9d433..50471db85f0 100755 --- a/web/build.sh +++ b/web/build.sh @@ -141,6 +141,9 @@ coverage_action() { builder_run_child_actions build:engine/device-detect builder_run_child_actions build:engine/dom-utils + +builder_run_child_actions build:engine/keyboard +builder_run_child_actions build:engine/js-processor builder_run_child_actions build:engine/element-wrappers builder_run_child_actions build:engine/events builder_run_child_actions build:engine/interfaces diff --git a/web/src/engine/interfaces/src/prediction/languageProcessor.interface.ts b/web/src/engine/interfaces/src/prediction/languageProcessor.interface.ts index df5ebae8968..84e78519879 100644 --- a/web/src/engine/interfaces/src/prediction/languageProcessor.interface.ts +++ b/web/src/engine/interfaces/src/prediction/languageProcessor.interface.ts @@ -1,7 +1,7 @@ /// import { EventEmitter } from "eventemitter3"; -import { OutputTarget } from "keyman/engine/js-processor"; +import { OutputTarget } from "keyman/engine/keyboard"; export class ReadySuggestions { suggestions: Suggestion[]; diff --git a/web/src/engine/interfaces/src/prediction/predictionContext.ts b/web/src/engine/interfaces/src/prediction/predictionContext.ts index 221875e62cb..cd856153b23 100644 --- a/web/src/engine/interfaces/src/prediction/predictionContext.ts +++ b/web/src/engine/interfaces/src/prediction/predictionContext.ts @@ -1,6 +1,6 @@ import { EventEmitter } from "eventemitter3"; import { type LanguageProcessorSpec , ReadySuggestions, type InvalidateSourceEnum, StateChangeHandler } from './languageProcessor.interface.js'; -import { type KeyboardProcessor, type OutputTarget } from 'keyman/engine/js-processor'; +import { type OutputTarget } from "keyman/engine/keyboard"; interface PredictionContextEventMap { update: (suggestions: Suggestion[]) => void; @@ -32,7 +32,7 @@ export default class PredictionContext extends EventEmitter string; /** * Represents the active context used when requesting and applying predictive-text operations. @@ -59,27 +59,18 @@ export default class PredictionContext extends EventEmitter Promise; private readonly suggestionReverter: (reversion: Reversion) => void; - /** - * Handler for post-processing once a suggestion has been applied: calls - * into the active keyboard's `begin postKeystroke` entry point. - * - * Called after the suggestion is applied but _before_ new predictions are - * requested based on the resulting context. - */ - private readonly postApplicationHandler: () => void; - - public constructor(langProcessor: LanguageProcessorSpec, kbdProcessor: KeyboardProcessor) { + public constructor(langProcessor: LanguageProcessorSpec, getLayerId: () => string) { super(); this.langProcessor = langProcessor; - this.kbdProcessor = kbdProcessor; + this.getLayerId = getLayerId; const validSuggestionState: () => boolean = () => this.currentTarget && langProcessor.state == 'configured'; this.suggestionApplier = (suggestion) => { if(validSuggestionState()) { - return langProcessor.applySuggestion(suggestion, this.currentTarget, () => kbdProcessor.layerId); + return langProcessor.applySuggestion(suggestion, this.currentTarget, getLayerId); } else { return null; } @@ -94,19 +85,6 @@ export default class PredictionContext extends EventEmitter { - // Tell the keyboard that the current layer has not changed - kbdProcessor.newLayerStore.set(''); - kbdProcessor.oldLayerStore.set(''); - // Call the keyboard's entry point. - kbdProcessor.processPostKeystroke(kbdProcessor.contextDevice, this.currentTarget) - // If we have a RuleBehavior as a result, run it on the target. This should - // only change system store and variable store values. - ?.finalize(kbdProcessor, this.currentTarget, true); - }; - this.connect(); } @@ -116,8 +94,6 @@ export default class PredictionContext extends EventEmitter; -export default abstract class OutputTarget { +export default abstract class OutputTarget implements OutputTargetInterface { private _dks: DeadkeyTracker; constructor() { diff --git a/web/src/engine/keyboard/src/index.ts b/web/src/engine/keyboard/src/index.ts index f5a4edea71d..28b6331c219 100644 --- a/web/src/engine/keyboard/src/index.ts +++ b/web/src/engine/keyboard/src/index.ts @@ -31,6 +31,7 @@ export * from "./defaultRules.js"; export { default as KeyEvent } from "./keyEvent.js"; export * from "./keyEvent.js"; export { default as KeyMapping } from "./keyMapping.js"; +export { OutputTarget } from "./outputTarget.interface.js"; export * from "@keymanapp/web-utils"; diff --git a/web/src/engine/main/src/headless/languageProcessor.ts b/web/src/engine/main/src/headless/languageProcessor.ts index c1c1721484b..cd39afc672b 100644 --- a/web/src/engine/main/src/headless/languageProcessor.ts +++ b/web/src/engine/main/src/headless/languageProcessor.ts @@ -336,6 +336,7 @@ export class LanguageProcessor extends EventEmitter { public shutdown() { this.lmEngine.shutdown(); + this.removeAllListeners(); } public get isActive(): boolean { diff --git a/web/src/engine/main/src/keymanEngine.ts b/web/src/engine/main/src/keymanEngine.ts index b91920e70d3..cb4c6305938 100644 --- a/web/src/engine/main/src/keymanEngine.ts +++ b/web/src/engine/main/src/keymanEngine.ts @@ -1,5 +1,5 @@ import { type KeyEvent, type Keyboard, KeyboardKeymanGlobal } from "keyman/engine/keyboard"; -import { ProcessorInitOptions, RuleBehavior } from 'keyman/engine/js-processor'; +import { OutputTarget, ProcessorInitOptions, RuleBehavior } from 'keyman/engine/js-processor'; import { DOMKeyboardLoader as KeyboardLoader } from "keyman/engine/keyboard/dom-keyboard-loader"; import { InputProcessor } from './headless/inputProcessor.js'; import { OSKView } from "keyman/engine/osk"; @@ -241,6 +241,8 @@ export default class KeymanEngine< this.modelCache = new ModelCache(); const kbdCache = this.keyboardRequisitioner.cache; + const keyboardProcessor = this.core.keyboardProcessor; + const predictionContext = new PredictionContext(this.core.languageProcessor, () => keyboardProcessor.layerId); this.contextManager.configure({ resetContext: (target) => { // Could reset the target's deadkeys here, but it's really more of a 'core' task. @@ -253,10 +255,27 @@ export default class KeymanEngine< this.core.resetContext(target); } }, - predictionContext: new PredictionContext(this.core.languageProcessor, this.core.keyboardProcessor), + predictionContext: predictionContext, keyboardCache: this.keyboardRequisitioner.cache }); + /* + * Handler for post-processing once a suggestion has been applied. + * + * This is called after the suggestion is applied but _before_ new + * predictions are requested based on the resulting context. + */ + this.core.languageProcessor.on('suggestionapplied', () => { + // Tell the keyboard that the current layer has not changed + keyboardProcessor.newLayerStore.set(''); + keyboardProcessor.oldLayerStore.set(''); + // Call the keyboard's entry point. + keyboardProcessor.processPostKeystroke(keyboardProcessor.contextDevice, predictionContext.currentTarget as OutputTarget) + // If we have a RuleBehavior as a result, run it on the target. This should + // only change system store and variable store values. + ?.finalize(keyboardProcessor, predictionContext.currentTarget as OutputTarget, true); + }); + // #region Event handler wiring this.config.on('spacebartext', () => { // On change of spacebar-text mode, we currently need a layout refresh to update the diff --git a/web/src/test/auto/headless/engine/interfaces/prediction/predictionContext.spec.js b/web/src/test/auto/headless/engine/interfaces/prediction/predictionContext.spec.js index dbc24cad4ff..fe66d0904d7 100644 --- a/web/src/test/auto/headless/engine/interfaces/prediction/predictionContext.spec.js +++ b/web/src/test/auto/headless/engine/interfaces/prediction/predictionContext.spec.js @@ -5,7 +5,7 @@ import { LanguageProcessor, TranscriptionCache } from 'keyman/engine/main'; import { PredictionContext } from 'keyman/engine/interfaces'; import { Worker as LMWorker } from "@keymanapp/lexical-model-layer/node"; import { DeviceSpec } from 'keyman/engine/keyboard'; -import { KeyboardProcessor, Mock } from 'keyman/engine/js-processor'; +import { Mock } from 'keyman/engine/js-processor'; function compileDummyModel(suggestionSets) { return ` @@ -15,14 +15,6 @@ LMLayerWorker.loadModel(new models.DummyModel({ `; } -// Common spec used for each test's setup. It's actually irrelevant for the tests, -// but KeyboardProcessor needs an instance. -const deviceSpec = new DeviceSpec( - DeviceSpec.Browser.Chrome, - DeviceSpec.FormFactor.Desktop, - DeviceSpec.OperatingSystem.Windows -); - const appleDummySuggestionSets = [[ // Set 1: { @@ -67,6 +59,10 @@ const appleDummyModel = { code: compileDummyModel(appleDummySuggestionSets) }; +function dummiedGetLayer() { + return 'default'; +} + describe("PredictionContext", () => { let worker; @@ -82,8 +78,7 @@ describe("PredictionContext", () => { const langProcessor = new LanguageProcessor(worker, new TranscriptionCache()); await langProcessor.loadModel(appleDummyModel); // await: must fully 'configure', load script into worker. - const kbdProcessor = new KeyboardProcessor(deviceSpec); - const predictiveContext = new PredictionContext(langProcessor, kbdProcessor); + const predictiveContext = new PredictionContext(langProcessor, dummiedGetLayer); let updateFake = sinon.fake(); predictiveContext.on('update', updateFake); @@ -108,7 +103,7 @@ describe("PredictionContext", () => { mock.insertTextBeforeCaret('e'); // appl| + e = apple let transcription = mock.buildTranscriptionFrom(initialMock, null, true); - await langProcessor.predict(transcription, kbdProcessor.layerId); + await langProcessor.predict(transcription, dummiedGetLayer()); // First predict call results: our second set of dummy suggestions, the first of which includes // a 'keep' of the original text. @@ -123,8 +118,7 @@ describe("PredictionContext", () => { const langProcessor = new LanguageProcessor(worker, new TranscriptionCache()); await langProcessor.loadModel(appleDummyModel); // await: must fully 'configure', load script into worker. - const kbdProcessor = new KeyboardProcessor(deviceSpec); - const predictiveContext = new PredictionContext(langProcessor, kbdProcessor); + const predictiveContext = new PredictionContext(langProcessor, dummiedGetLayer); let mock = new Mock("appl", 4); // "appl|", with '|' as the caret position. const initialSuggestions = await predictiveContext.setCurrentTarget(mock); @@ -149,8 +143,7 @@ describe("PredictionContext", () => { const langProcessor = new LanguageProcessor(worker, new TranscriptionCache()); await langProcessor.loadModel(appleDummyModel); // await: must fully 'configure', load script into worker. - const kbdProcessor = new KeyboardProcessor(deviceSpec); - const predictiveContext = new PredictionContext(langProcessor, kbdProcessor); + const predictiveContext = new PredictionContext(langProcessor, dummiedGetLayer); let textState = new Mock("appl", 4); // "appl|", with '|' as the caret position. @@ -164,7 +157,7 @@ describe("PredictionContext", () => { let previousTextState = Mock.from(textState); textState.insertTextBeforeCaret('e'); // appl| + e = apple let transcription = textState.buildTranscriptionFrom(previousTextState, null, true); - await langProcessor.predict(transcription, kbdProcessor.layerId); + await langProcessor.predict(transcription, dummiedGetLayer()); // Verify setup. assert.equal(updateFake.callCount, 1); @@ -217,8 +210,7 @@ describe("PredictionContext", () => { const langProcessor = new LanguageProcessor(worker, new TranscriptionCache()); await langProcessor.loadModel(appleDummyModel); // await: must fully 'configure', load script into worker. - const kbdProcessor = new KeyboardProcessor(deviceSpec); - const predictiveContext = new PredictionContext(langProcessor, kbdProcessor); + const predictiveContext = new PredictionContext(langProcessor, dummiedGetLayer); let textState = new Mock("appl", 4); // "appl|", with '|' as the caret position. @@ -233,7 +225,7 @@ describe("PredictionContext", () => { let suggestionCaptureFake = sinon.fake(); predictiveContext.once('update', suggestionCaptureFake); - await langProcessor.predict(transcription, kbdProcessor.layerId); + await langProcessor.predict(transcription, dummiedGetLayer()); // We need to capture the suggestion we wish to apply. We could hardcode a forced // value, but that might become brittle in the long-term. From c4340542f77648d7072ab2d43d83990456439b34 Mon Sep 17 00:00:00 2001 From: "Joshua A. Horton" Date: Thu, 15 Aug 2024 15:54:15 +0700 Subject: [PATCH 2/2] fix(web): patch up newly-merged-in automated test --- .../engine/interfaces/prediction/predictionContext.spec.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/web/src/test/auto/headless/engine/interfaces/prediction/predictionContext.spec.js b/web/src/test/auto/headless/engine/interfaces/prediction/predictionContext.spec.js index dc91c512136..4217aeb69b6 100644 --- a/web/src/test/auto/headless/engine/interfaces/prediction/predictionContext.spec.js +++ b/web/src/test/auto/headless/engine/interfaces/prediction/predictionContext.spec.js @@ -118,8 +118,7 @@ describe("PredictionContext", () => { const langProcessor = new LanguageProcessor(worker, new TranscriptionCache()); await langProcessor.loadModel(appleDummyModel); // await: must fully 'configure', load script into worker. - const kbdProcessor = new KeyboardProcessor(deviceSpec); - const predictiveContext = new PredictionContext(langProcessor, kbdProcessor); + const predictiveContext = new PredictionContext(langProcessor, dummiedGetLayer); let updateFake = sinon.fake(); predictiveContext.on('update', updateFake); @@ -146,13 +145,13 @@ describe("PredictionContext", () => { // Mocking: corresponds to the second set of mocked predictions - round 2 of // 'apple', 'apply', 'apples'. - const skippedPromise = langProcessor.predict(baseTranscription, kbdProcessor.layerId); + const skippedPromise = langProcessor.predict(baseTranscription, dummiedGetLayer()); mock.insertTextBeforeCaret('e'); // appl| + e = apple const finalTranscription = mock.buildTranscriptionFrom(initialMock, null, true); // Mocking: corresponds to the third set of mocked predictions - 'applied'. - const expectedPromise = langProcessor.predict(finalTranscription, kbdProcessor.layerId); + const expectedPromise = langProcessor.predict(finalTranscription, dummiedGetLayer()); await Promise.all([skippedPromise, expectedPromise]); const expected = await expectedPromise;