From 909d833474d3c8e6985c74419c6dca52006991e2 Mon Sep 17 00:00:00 2001 From: Marc Durdin Date: Mon, 22 Nov 2021 10:47:05 +1100 Subject: [PATCH] fix(web): improve keyboard switch performance Summary of these changes: Two targeted improvements; more optimization is possible but IMO this is a significant enough improvement to go through test: 1. In oskView.ts, avoid calling `VisualKeyboard.refreshLayout` twice from `OSKView.refreshLayout`. This roughly halves the time spent in `VisualKeyboard.refreshLayout` for a keyboard switch. 2. In modelManager.ts, we avoid a very expensive `unloadModel` / `loadModel` sequence from `registerModel` by checking to see if the model spec we are passed is already registered. This avoids an expensive callback to Keyman for Android which causes the banner to 'bounce'. Longer notes: So `VisualKeyboard.refreshLayout` is called 20+ times during a keyboard switch. It is a very expensive call. It is the bulk of the time spent when you press the globe key in Keyman for Android. This seems ripe for optimization; see the following functions in keyboard.html: * `setKeymanLanguage` -> `setActiveKeyboard` -> 6 calls * `enableSuggestions` -> `registerModel` -> 8 calls * `stateChange` -> 6+ calls `OSKView.refreshLayout` always seems to call `VisualKeyboard.refreshLayout` twice: first with `setSize`, and then again itself. Keyman for Android and KeymanWeb seem to fight over who is responsible for selecting models (KMW first calls `loadModel`, then Keyman for Android calls `unloadModel`, `loadModel`, through `enableSuggestions`). The fix for rotation (`correctOSKTextSize()`), as called from `stateChange()`, is expensive as well, because it rebuilds the entire keyboard from scratch. Along with that, we have the size of the webview changing as the model is unloaded and reloaded and lots of back-and-forth between the KeymanWeb and KMEA which seems unnecessary. Shouldn't KeymanWeb be responsible for loading and unloading models, once they are all registered? We should only need to register the models once at page load time (when config changes, do we just reload the page?). --- .../KMEA/app/src/main/assets/keyboard.html | 3 +- web/source/osk/oskView.ts | 55 +++++++++---------- web/source/text/prediction/modelManager.ts | 9 ++- 3 files changed, 34 insertions(+), 33 deletions(-) diff --git a/android/KMEA/app/src/main/assets/keyboard.html b/android/KMEA/app/src/main/assets/keyboard.html index 5255542aac..d0d4f93cf0 100644 --- a/android/KMEA/app/src/main/assets/keyboard.html +++ b/android/KMEA/app/src/main/assets/keyboard.html @@ -118,8 +118,7 @@ //window.console.log('onStateChange change: ' + change); // Refresh KMW OSK - var kmw=window['keyman']; - kmw['correctOSKTextSize'](); + keyman.correctOSKTextSize(); fragmentToggle = (fragmentToggle + 1) % 100; if(change != 'configured') { // doesn't change the display; only initiates suggestions. diff --git a/web/source/osk/oskView.ts b/web/source/osk/oskView.ts index 2a8756854f..a4b3cfc492 100644 --- a/web/source/osk/oskView.ts +++ b/web/source/osk/oskView.ts @@ -22,7 +22,7 @@ namespace com.keyman.osk { manual = "manual", automatic = "automatic" } - + export abstract class OSKView { _Box: HTMLDivElement; @@ -34,23 +34,23 @@ namespace com.keyman.osk { protected device: com.keyman.utils.DeviceSpec; protected readonly hostDevice: com.keyman.utils.DeviceSpec; - private _boxBaseMouseDown: (e: MouseEvent) => boolean; + private _boxBaseMouseDown: (e: MouseEvent) => boolean; private _boxBaseTouchStart: (e: TouchEvent) => boolean; private _boxBaseTouchEventCancel: (e: TouchEvent) => boolean; private keyboard: keyboards.Keyboard; private _target: text.OutputTarget; - + /** * The configured width for this OSKManager. May be `undefined` or `null` - * to allow automatic width scaling. + * to allow automatic width scaling. */ private _width: ParsedLengthStyle; /** * The configured height for this OSKManager. May be `undefined` or `null` - * to allow automatic height scaling. + * to allow automatic height scaling. */ private _height: ParsedLengthStyle; @@ -111,7 +111,7 @@ namespace com.keyman.osk { // Register a listener for model change events so that we can hot-swap the banner as needed. // Handled here b/c banner changes may trigger a need to re-layout the OSK. const _this = this; - keymanweb.core.languageProcessor.on('statechange', + keymanweb.core.languageProcessor.on('statechange', function(state: text.prediction.StateChangeEnum) { let currentType = _this.bannerView.activeType; _this.bannerView.selectBanner(state); @@ -182,7 +182,7 @@ namespace com.keyman.osk { /** * Gets and sets the IME-like interface (`OutputTarget`) to be affected by events from * the OSK. - * + * * If `activationMode` is `'conditional'`, this property's state controls the visibility * of the OSKView. */ @@ -259,11 +259,11 @@ namespace com.keyman.osk { /** * A property denoting whether or not the OSK should be presented if it meets its - * activation conditions. - * + * activation conditions. + * * When `activationMode == 'manual'`, `displayIfActive == true` is the lone * activation condition. - * + * * Note: cannot be set to `false` if `activationMode == 'static'`. */ get displayIfActive(): boolean { @@ -318,7 +318,7 @@ namespace com.keyman.osk { /** * The configured width for this VisualKeyboard. May be `undefined` or `null` - * to allow automatic width scaling. + * to allow automatic width scaling. */ get width(): ParsedLengthStyle { return this._width; @@ -326,7 +326,7 @@ namespace com.keyman.osk { /** * The configured height for this VisualKeyboard. May be `undefined` or `null` - * to allow automatic height scaling. + * to allow automatic height scaling. */ get height(): ParsedLengthStyle { return this._height; @@ -357,7 +357,7 @@ namespace com.keyman.osk { } /** - * The top-level style string for the font size used by the predictive banner + * The top-level style string for the font size used by the predictive banner * and the primary keyboard visualization elements. */ get baseFontSize(): string { @@ -397,7 +397,7 @@ namespace com.keyman.osk { return ParsedLengthStyle.special(fontScale, 'em'); } else { return this.computedHeight ? ParsedLengthStyle.inPixels(this.computedHeight / 8) : undefined; - } + } } public get activeKeyboard(): keyboards.Keyboard { @@ -509,9 +509,6 @@ namespace com.keyman.osk { availableHeight -= this.bannerView.height + 5; } this.vkbd.setSize(this.computedWidth, availableHeight, pending); - if(!pending) { - this.vkbd.refreshLayout(); - } const bs = this._Box.style; // OSK size settings can only be reliably applied to standard VisualKeyboard @@ -614,7 +611,7 @@ namespace com.keyman.osk { private layerChangeHandler: text.SystemStoreMutationHandler = function(this: OSKView, source: text.MutableSystemStore, newValue: string) { - // This handler is also triggered on state-key state changes (K_CAPS) that + // This handler is also triggered on state-key state changes (K_CAPS) that // may not actually change the layer. if(this.vkbd) { this.vkbd._UpdateVKShiftStyle(); @@ -691,7 +688,7 @@ namespace com.keyman.osk { /** * The main function for presenting the OSKView. - * + * * This includes: * - refreshing its layout * - displaying it @@ -719,7 +716,7 @@ namespace com.keyman.osk { /* In case it's still '0' from a hide() operation. * - * (Opacity is only modified when device.touchable = true, + * (Opacity is only modified when device.touchable = true, * though a couple of extra conditions may apply.) */ this._Box.style.opacity = '1'; @@ -808,7 +805,7 @@ namespace com.keyman.osk { } /** - * + * * @returns `false` if the OSK is in an invalid state for being presented to the user. */ protected mayShow(): boolean { @@ -829,8 +826,8 @@ namespace com.keyman.osk { } /** - * - * @param hiddenByUser + * + * @param hiddenByUser * @returns `false` if the OSK is in an invalid state for being hidden from the user. */ protected mayHide(hiddenByUser: boolean): boolean { @@ -851,12 +848,12 @@ namespace com.keyman.osk { /** * Applies CSS styling and handling needed to perform a fade animation when * hiding the OSK. - * + * * Note: currently reflects an effectively-dead code path, though this is * likely not intentional. Other parts of the KMW engine seem to call hideNow() * synchronously after each and every part of the engine that calls this function, * cancelling the Promise. - * + * * @returns A Promise denoting either cancellation of the hide (`false`) or * completion of the hide & its animation (`true`) */ @@ -993,10 +990,10 @@ namespace com.keyman.osk { /** * Display build number - * + * * In the future, this should raise an event that the consuming KeymanWeb * engine may listen for & respond to, rather than having it integrated - * as part of the OSK itself. + * as part of the OSK itself. */ showBuild() { let keymanweb = com.keyman.singleton; @@ -1006,11 +1003,11 @@ namespace com.keyman.osk { /** * Display list of installed keyboards in pop-up menu - * + * * In the future, this language menu should be defined as a UI module like the standard * desktop UI modules. The globe key should then trigger an event to _request_ that the * consuming engine display the active UI module's menu. - * + * **/ showLanguageMenu() { if(this.hostDevice.touchable) { diff --git a/web/source/text/prediction/modelManager.ts b/web/source/text/prediction/modelManager.ts index cce2aeccee..607f291ad6 100644 --- a/web/source/text/prediction/modelManager.ts +++ b/web/source/text/prediction/modelManager.ts @@ -8,10 +8,10 @@ namespace com.keyman.text.prediction { // Allows for easy model lookup by language code; useful when switching keyboards. languageModelMap: {[language:string]: ModelSpec} = {}; - + init() { let keyman = com.keyman.singleton; - + // Registers this module for keyboard (language) and model change events. keyman['addEventListener']('keyboardchange', this.onKeyboardChange.bind(this)); } @@ -46,6 +46,11 @@ namespace com.keyman.text.prediction { let keyman = com.keyman.singleton; let activeLanguage = keyman.keyboardManager.getActiveLanguage(); + if(JSON.stringify(model) == JSON.stringify(this.registeredModels[model.id])) { + // We are already registered, let's not go through and re-register + // because we'll already have the correct model active + return; + } this.registeredModels[model.id] = model; // Register the model for each targeted language code variant.