change(web): facilitates differentiated configuration objects for KMW variants 🧩 #8458

jahorton · 2023-03-17T03:44:18Z

The eventual app/webview and app/browser variants of Keyman Engine for Web have many configuration aspects in common, but some of those aspects are quite specific to one variant or the other:

app/webview looks for a number of specialized hooks used in the KMW-embedding process to allow our mobile apps to better integrate with it.
app/browser looks to integrate itself into whatever websites / webpages utilize it.

While the full scope of app/browser's specialized needs haven't been fully determined at this time due to prioritizing app/webview, there's already sufficient cause to motivate tailored configuration classes for the two. These classes will likely serve as a central "settings" for other components that may be passed as needed, rather than exposing the entirety of top-level objects.

This PR also includes handling of a few configuration-related "loose ends".

@keymanapp-test-bot skip

…ange/web/engine-configuration

keymanapp-test-bot · 2023-03-17T03:44:21Z

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

Windows
- Keyman for Windows
- FirstVoices Keyboards for Windows

jahorton · 2023-03-17T03:50:37Z

web/src/app/webview/src/keymanEngine.ts

+  constructor(worker: Worker, sourceUri: string) {
+    const config = new WebviewConfiguration(sourceUri);  // currently set to perform device auto-detect.
+    config.stubNamespacer = (stub) => {
+      // If the package has not yet been applied as namespacing...
+      if(stub.KP && stub.KI.indexOf(`${stub.KP}::`) == -1) {
+        // Apply namespacing.  To make 100% sure that we don't muck up internal prefixing,
+        // we ensure it is applied consistently in the manner specified below.
+        stub.KI = toPrefixedKeyboardId(`${stub.KP}::${toUnprefixedKeyboardId(stub.KI)}`);
+      }
+    }
+
+    super(worker, config, new ContextManager());


This block best showcases the main reason for the change in constructor parameter ordering: app/webview (and eventually app/browser) will construct their own Configuration object in their respective KeymanEngine constructor. Each derived-class constructor may take additional parameters, but the first parameter - for worker - will be consistent for both.

The reason: Worker can't be specified at this level if we wish to provide separate debug vs release build products. The non-minified + sourcemapped variant of the worker should be used for debug, while the minified + unsourcemapped variant would be used for release. These will be specified by the top-level entry point for both debug and release esbuild bundling configurations.

jahorton · 2023-03-17T03:54:53Z

web/src/engine/configuration/src/index.ts

Note that only path-related classes remain in this child project. A followup PR (#8459) will rename it accordingly.

As it's still utilized by more than one of the other child projects (engine/keyboard-cache and engine/main), it's still worth keeping around as a separate module of sorts.

There's a small part of me that thinks we could have the reduced paths interface used by engine/osk within this child project too, but then we'd might want to undo that when reusing the OSK in our other products;. The KMW-specific path fields defined by this child-project's objects wouldn't be of use to other platforms, after all, and wouldn't make much sense within a common space, unlike the OSK.

jahorton · 2023-03-17T04:00:35Z

web/src/engine/dom-utils/src/stylesheets.ts

@@ -106,8 +108,9 @@ export class StylesheetManager {
    // Modern browsers: use WOFF, TTF and fallback finally to SVG. Don't provide EOT
    if(os == DeviceSpec.OperatingSystem.iOS) {
      if(ttf != '') {
-        // once upon a time, we did a cache-prevention thing here... but that was removed long, long ago.
-        // ttf = this.unCached(ttf);


Good thing I checked - I was wrong! Fortunately, the kmwembedded.ts part I'd missed had not been removed yet, so I caught this mistake and remedied it here.

jahorton · 2023-03-17T04:04:13Z

web/src/engine/main/src/engineConfiguration.ts

+    this._stubNamespacer = functor;
+  }
+
+  debugReport(): Record<string, any> {


Intended for use with keyman.getDebugInfo().

As seen in the Sentry dashboard:

At this point, it may well implement most of the current report seen above, but we will likely add a few extra properties from other sources (like the engine object and/or the OSK) as well - so this isn't intended as the top-level call for said function.

jahorton · 2023-03-17T04:05:01Z

web/src/engine/main/src/engineConfiguration.ts

+  private _defaultSpacebarText: SpacebarText;
+  private _stubNamespacer?: (KeyboardStub) => void;
+
+  public applyCacheBusting: boolean = false;


app/webview: true
app/browser: false

jahorton · 2023-03-17T04:06:34Z

web/src/app/webview/src/keymanEngine.ts


    this.hardKeyboard = new PassthroughKeyboard(config.hardDevice);
  }

-  initialize() {
-    super.initialize();
+  init(options: Required<WebviewInitOptionSpec>) {


Yay, a keyman.init for the modularized app/webview! Well, the start of one, anyway...

web/build.sh

mcdurdin · 2023-03-20T00:44:27Z

web/src/app/webview/src/keymanEngine.ts

+    const config = new WebviewConfiguration(sourceUri);  // currently set to perform device auto-detect.
+    config.stubNamespacer = (stub) => {
+      // If the package has not yet been applied as namespacing...
+      if(stub.KP && stub.KI.indexOf(`${stub.KP}::`) == -1) {


Couldn't you just search for ::? Although wouldn't it be better for the namespace to be separated from KI rather than mutating input data?

Although wouldn't it be better for the namespace to be separated from KI rather than mutating input data?

I could see an argument for that, but the pre-modular namespacing system worked by prepending the package name as part of the ID. I opted to maintain that.

There's then the issue of Package_id::Keyboard_kbd_id vs Keyboard_Package_id::kbd_id due to the whole Keyboard_ ID-prefixing prepend bit that's been in KMW since forever. The function highlighted in the code block above is designed to ensure that it's always the former pattern and never the latter by detecting the latter and converting it over while leaving the former intact.

Though... I'm not 100% sure that Package-prepended keyboards actually use the Keyboard_ bit too in the existing implementation.

Confirmation for that last point...

(from the current master)

keyman/web/src/engine/main/keyboards/kmwkeyboards.ts

Line 470 in 706cc07

// Skip on embedded which namespaces packageID::Keyboard_keyboardID

Contrast with...

(also from the current master)

keyman/web/src/app/embed/kmwembedded.ts

Lines 168 to 174 in 706cc07

// Establishes keyboard namespacing.

keymanweb.namespaceID = function(Pstub) {

if(typeof(Pstub['KP']) != 'undefined') {

// An embedded use case wants to utilize package-namespacing.

Pstub['KI'] = Pstub['KP'] + "::" + Pstub['KI'];

}

}

Any incoming keyboard IDs are already mutated to have a prepended package ID as part of the keyboard's ID itself.

We need to make sure that the Keyboard_ applies at the location we want, hence the if-condition seen above.

web/src/app/webview/src/keymanEngine.ts

mcdurdin · 2023-03-20T00:46:40Z

web/src/engine/dom-utils/src/stylesheets.ts

@@ -106,8 +108,9 @@ export class StylesheetManager {
    // Modern browsers: use WOFF, TTF and fallback finally to SVG. Don't provide EOT


No SVG fonts! They're as dead as EOT.

Suggested change

// Modern browsers: use WOFF, TTF and fallback finally to SVG. Don't provide EOT

// Modern browsers: use WOFF2, WOFF, and fallback finally to TTF. SVG, EOT not supported.

See also lines 122-127. Perhaps should be done as a separate issue.

Now tracked at #8500.

mcdurdin · 2023-03-20T00:49:19Z

web/src/engine/main/src/engineConfiguration.ts

+    if(typeof options.setActiveOnRegister == 'boolean') {
+      this._activateFirstKeyboard = options.setActiveOnRegister;
+    } else if (typeof options.setActiveOnRegister == 'string') {
+      let str = options.setActiveOnRegister.toLowerCase();
+      this._activateFirstKeyboard = str === 'true';


Why is this option different types? Seems weird.

The documentation at https://help.keyman.com/DEVELOPER/ENGINE/WEB/15.0/reference/core/init does suggest we could just go with string.

That said, from master:

keyman/web/src/engine/main/kmwbase.ts

Lines 94 to 102 in ecd97d2

// Defines default option values

options: OptionType = {

root: '',

resources: '',

keyboards: '',

fonts: '',

attachType: '',

ui: null,

setActiveOnRegister: 'true', // TODO: convert to boolean

I suppose that TODO can be deferred instead.

https://github.com/search?q=org%3Akeymanapp%20setActiveOnRegister&type=code

Yuck! This is the type of thing where the string type should be marked as deprecated in 17.0 and replaced with a boolean in 18.0+. For now though, it is only documented as supporting string.

So given KM Developer Server passes a boolean, and keymanweb.com passes a string, I'd be really happy to just turn this into Boolean and mark it as a breaking change in the release notes, fixing keymanweb.com at the same time. It's a super simple fix for anyone using kmw on their own site, and they'll probably feel good about themselves when they convert "true" into true too!

See #4304 for history of this

fixing keymanweb.com at the same time

I'm not seeing a super-simple way to make an update for this right now for keymanweb.com without including lines 40-41 above. I suppose we'd need to make that update come beta instead?

I've gone ahead and removed those lines in my most recent commit.

For keymanweb.com we should do that when this merges into master, and then version-compare to determine which data type to use.

Added as a TODO item on #8047 (the base feature branch's PR).

…r-configure-fix' into change/web/engine-configuration

jahorton added 9 commits March 14, 2023 15:24

change(web): engine config moved to main/engine, keyboard namespacing

2f6871f

change(web): handling of embed/webview cache busting

ebf1002

change(web): css cache-bust, webview-mode default options

8dedc5c

change(web): differentiated app-level configs, getDebugInfo

9c8c426

fix(web): osk cachebusting for keyboard stylesheets

c9a4c19

chore(web): minor toolchain tweak

18b6c83

fix(web): fixes a method signature

3732834

fix(web): app/webview now sets OSK's doCacheBusting

236ebc2

chore(web/engine): Merge branch 'feature-esmodule-web-engine' into ch…

581babe

…ange/web/engine-configuration

jahorton added this to the A17S8 milestone Mar 17, 2023

github-actions bot added common/ common/web/ web/ labels Mar 17, 2023

change(web): keyman.initialize -> keyman.init

da06b23

jahorton commented Mar 17, 2023

View reviewed changes

jahorton marked this pull request as ready for review March 17, 2023 04:06

jahorton requested review from mcdurdin and ermshiperete as code owners March 17, 2023 04:06

jahorton mentioned this pull request Mar 17, 2023

change(web): engine/configuration -> engine/paths 🧩 #8459

Merged

mcdurdin modified the milestones: A17S8, A17S9 Mar 18, 2023

mcdurdin approved these changes Mar 20, 2023

View reviewed changes

mcdurdin mentioned this pull request Mar 20, 2023

chore(common): make verify_npm_setup smart enough to run only once per build #8471

Closed

This was referenced Mar 23, 2023

chore(web): supported font revision #8500

Open

chore(web): merge master into feature-esmodule-web-engine (for configure fix) 🧩 #8501

Merged

chore(web/engine): Merge branch 'chore/web/update-feature-esmodule-fo…

20567d4

…r-configure-fix' into change/web/engine-configuration

jahorton changed the base branch from feature-esmodule-web-engine to chore/web/update-feature-esmodule-for-configure-fix March 23, 2023 08:21

chore(web): addresses final PR review concerns

d2f4e98

Base automatically changed from chore/web/update-feature-esmodule-for-configure-fix to feature-esmodule-web-engine March 26, 2023 15:48

jahorton mentioned this pull request Mar 27, 2023

feat(web): conversion of Keyman Engine for Web to ES modules 🧩 #8047

Merged

10 tasks

jahorton merged commit 40c6c14 into feature-esmodule-web-engine Mar 27, 2023

jahorton deleted the change/web/engine-configuration branch March 27, 2023 01:14

jahorton mentioned this pull request Apr 1, 2023

epic: conversion of Keyman Engine for Web to ES modules 🧩 #8560

Merged

20 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change(web): facilitates differentiated configuration objects for KMW variants 🧩 #8458

change(web): facilitates differentiated configuration objects for KMW variants 🧩 #8458

jahorton commented Mar 17, 2023

keymanapp-test-bot bot commented Mar 17, 2023 •

edited

Loading

jahorton Mar 17, 2023

jahorton Mar 17, 2023 •

edited

Loading

jahorton Mar 17, 2023

jahorton Mar 17, 2023

jahorton Mar 17, 2023

jahorton Mar 17, 2023

mcdurdin Mar 20, 2023

jahorton Mar 20, 2023

jahorton Mar 23, 2023 •

edited

Loading

mcdurdin Mar 20, 2023

jahorton Mar 23, 2023

mcdurdin Mar 20, 2023

jahorton Mar 20, 2023

mcdurdin Mar 20, 2023

mcdurdin Mar 20, 2023

mcdurdin Mar 20, 2023

jahorton Mar 23, 2023 •

edited

Loading

mcdurdin Mar 24, 2023

jahorton Mar 27, 2023 •

edited

Loading

	// Establishes keyboard namespacing.
	keymanweb.namespaceID = function(Pstub) {
	if(typeof(Pstub['KP']) != 'undefined') {
	// An embedded use case wants to utilize package-namespacing.
	Pstub['KI'] = Pstub['KP'] + "::" + Pstub['KI'];
	}
	}

		@@ -106,8 +108,9 @@ export class StylesheetManager {
		// Modern browsers: use WOFF, TTF and fallback finally to SVG. Don't provide EOT

	// Modern browsers: use WOFF, TTF and fallback finally to SVG. Don't provide EOT
	// Modern browsers: use WOFF2, WOFF, and fallback finally to TTF. SVG, EOT not supported.

	// Defines default option values
	options: OptionType = {
	root: '',
	resources: '',
	keyboards: '',
	fonts: '',
	attachType: '',
	ui: null,
	setActiveOnRegister: 'true', // TODO: convert to boolean

change(web): facilitates differentiated configuration objects for KMW variants 🧩 #8458

change(web): facilitates differentiated configuration objects for KMW variants 🧩 #8458

Conversation

jahorton commented Mar 17, 2023

keymanapp-test-bot bot commented Mar 17, 2023 • edited Loading

User Test Results

Test Artifacts

Choose a reason for hiding this comment

jahorton Mar 17, 2023 • edited Loading

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 Mar 23, 2023 • edited Loading

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 Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jahorton Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

keymanapp-test-bot bot commented Mar 17, 2023 •

edited

Loading

jahorton Mar 17, 2023 •

edited

Loading

jahorton Mar 23, 2023 •

edited

Loading

jahorton Mar 23, 2023 •

edited

Loading

jahorton Mar 27, 2023 •

edited

Loading