chore(web): clean up remaining low-level Web .tsconfig settings 🔩 #11424

jahorton · 2024-05-13T06:12:52Z

This PR serves to finish off #11473 for all Keyman Engine for Web code outside ${KEYMAN_ROOT}/web.

This used to be earlier in the chain, but now multiple related bug fixes detected due to the enhanced settings have been hoisted into what are now ancestor PRs.

@keymanapp-test-bot skip

keymanapp-test-bot · 2024-05-13T06:12:57Z

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

Android
Developer
iOS
- Keyman for iOS (simulator image)
- FirstVoices Keyboards for iOS (simulator image)
- TestFlight internal PR build version - 18.0.47 (0.11424.11158)
Keyboards
- Test Keyboards
Web
- KeymanWeb Test Home
Windows

jahorton · 2024-05-13T06:13:43Z

web/tsconfig.base.json

-    // TODO: These override ../tsconfig.base.json settings, and so should be removed if possible,
-    //       but existing code in web/ breaks some of these settinsg
-    "noImplicitThis": false,
-    "noImplicitReturns": false,
-    "noImplicitAny": false,
-    "strictFunctionTypes": false,
-    "noUnusedLocals": false,


The big motivator: cleaning this up. I was able to push it back significantly outside of web/ (and common/web/gesture-recognizer).

The latter: any TS compilation errors encountered by mocha when loading tests are not passed through, leaving the cause somewhat opaque.

I resolve these over the following PRs, one flag at a time, along with the corresponding settings in web/.

common/web/gesture-recognizer/tsconfig.json

mcdurdin

There are a lot of changes here which appear to be bug fixes. I guess I would like to know if they have any tangible impact on behaviour of KeymanWeb, and if any of them should be back-ported to 17.0-stable. Any fixes that have a tangible impact should probably have separate issues opened so that we can track them. I know that's a bunch of work, but bundling all the fixes together otherwise makes it really hard to track behavioural changes as a result of this PR.

If you could add reasons in comments for the @ts-ignores (some are reasonably obvious I guess but not all of them), that would also be great.

common/web/gesture-recognizer/src/engine/headless/gestures/specs/modelDefValidator.ts

common/web/gesture-recognizer/src/engine/touchEventEngine.ts

common/web/gesture-recognizer/src/tools/unit-test-resources/src/inputSequenceSimulator.ts

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

common/web/keyboard-processor/src/text/codes.ts

common/web/keyboard-processor/src/text/kbdInterface.ts

common/web/lm-worker/src/main/model-compositor.ts

common/web/recorder/src/index.ts

jahorton

I'll have to pause here for now re: the questions in the comments, but figured I could go ahead and submit my current progress.

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

common/web/input-processor/src/text/prediction/predictionContext.ts

common/web/keyboard-processor/src/keyboards/activeLayout.ts

common/web/keyboard-processor/src/keyboards/defaultLayouts.ts

common/web/keyboard-processor/src/keyboards/keyboard.ts

common/web/keyboard-processor/src/text/codes.ts

mcdurdin

I just wanted to follow up on a couple of the Promise<> return values because I think the scenarios are more complex than you maybe had anticipated. Hope this helps!

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

common/web/input-processor/src/text/prediction/predictionContext.ts

jahorton

OK, that should complete the first pass-through. I'll be using my own replies to help track what needs handling in what ways; apologies for any notification-noise this causes.

common/web/keyboard-processor/src/text/kbdInterface.ts

common/web/lm-worker/src/main/model-compositor.ts

common/web/recorder/src/index.ts

jahorton · 2024-05-23T07:34:15Z

There are a lot of changes here which appear to be bug fixes. I guess I would like to know if they have any tangible impact on behaviour of KeymanWeb, and if any of them should be back-ported to 17.0-stable. Any fixes that have a tangible impact should probably have separate issues opened so that we can track them. I know that's a bunch of work, but bundling all the fixes together otherwise makes it really hard to track behavioural changes as a result of this PR.

So, it sounds like the implication is that I should...

Put this back into draft
Create separate PRs for any noted bug fixes (possibly with associated issues)
Base the remainder of this PR after those separate PRs.

This PR can't proceed well without resolving said issues, so "the remainder" - which is intended as the main point of this PR - depends on the fixes existing either before or at the same time within commit history.

I remember there being some other noted issues in some of the follow-ups, so it may be worth bumping those changes up as part of step 2.

mcdurdin · 2024-05-23T07:43:49Z

So, it sounds like the implication is that I should...

Put this back into draft

Create separate PRs for any noted bug fixes (possibly with associated issues)

Base the remainder of this PR after those separate PRs.

Yes, that would be great, thanks. That'll make it really easy to cherry-pick any fixes that we think need to be back-ported as well. Sorry for the hassle!

I remember there being some other noted issues in some of the follow-ups, so it may be worth bumping those changes up as part of step 2.

Sounds good to me.

…/ and common/web/gesture-recognizer

mcdurdin

This is looking clean now, good work. LGTM

mcdurdin · 2024-05-30T02:20:15Z

common/web/keyboard-processor/src/keyboards/keyboard.ts

@@ -30,15 +30,42 @@ export interface VariableStoreDictionary {
  [name: string]: string;
 };

+export type KeyboardObject = {


Is this the only definition we have anywhere for this type? It feels like it belongs in common/web/types in the future, with copious comments...

So far as I can tell, yes. I always did want to write up a formal type for it, and this push gave the perfect excuse.

I'd be happy for the type to be hoisted to common/web/types; be my guest!

I just went ahead and added documentation for all fields.

mcdurdin · 2024-05-30T02:22:19Z

common/web/lm-worker/src/main/model-compositor.ts

+      let sample: Suggestion & {
+        p?: number,
+        "lexical-p"?: number,
+        "correction-p"?: number
+      } = value.sample;


Where else would this type be used?

These fields are currently only used for debugging; I've found them helpful in the past when determining what influenced the suggestions being displayed and how strongly earlier suggestions "won" over later ones. These pass through the lm-layer -> Web-engine boundary and are available for the Suggestion objects held by the suggestion banner, allowing them to be viewable without needing any breakpoints.

Without these two fields, the values they represent are locked within a limited time-window within the lm-worker, requiring breakpoints at the right location.

Of note: p, at least, would be very useful for implementing autocorrect. There's a chance one of the other two would be relevant as well. For example, we may wish to require much stricter keystroke input ("correction") for rarer words, possibly raising the overall threshold required to trigger for such cases.

The closest related issue I could find re: autocorrect: #1841. There doesn't seem to be a baseline "autocorrect" issue, but #1841 at least references the notion of autocorrect.

mcdurdin · 2024-05-30T23:10:04Z

common/web/keyboard-processor/src/keyboards/keyboard.ts

+  /**
+   * Virtual Key Dictionary: the engine pre-processed, unminified dictionary.  This is built within
+   * Keyman Engine for Web at runtime as needed based on the definitions in `KVKD`.
+   */
  VKDictionary?: Record<string, number>,


I reckon we should put VKDictionary and _kmw into a subclass, as they are internal to Keyman Engine for Web. But that can wait until we move to common/web/types.

Co-authored-by: Marc Durdin <marc@durdin.net>

ermshiperete

👍 I like these type definitions with comments!

ermshiperete · 2024-05-31T08:52:05Z

common/web/keyboard-processor/src/keyboards/defaultLayouts.ts

+  K102?: boolean,
+  /**
+   * Keyboard Layer Specification: an object-based map of layer name to the keycaps for its
+   * 65 keys.  The 65 keys are ordered from left to right, then top to bottom.


Why 65 (keys)?

Took some digging, but I found it.

This array is precisely 65 entries long and maps each index of the 65 to its represented hardware key. Note that some are reserved - that's the role of K_*.

keyman/common/web/keyboard-processor/src/keyboards/defaultLayouts.ts

Lines 51 to 59 in a024274

static readonly dfltCodes: ReadonlyArray<string> = [

"K_BKQUOTE","K_1","K_2","K_3","K_4","K_5","K_6","K_7","K_8","K_9","K_0",

"K_HYPHEN","K_EQUAL","K_*","K_*","K_*","K_Q","K_W","K_E","K_R","K_T",

"K_Y","K_U","K_I","K_O","K_P","K_LBRKT","K_RBRKT","K_BKSLASH","K_*",

"K_*","K_*","K_A","K_S","K_D","K_F","K_G","K_H","K_J","K_K","K_L",

"K_COLON","K_QUOTE","K_*","K_*","K_*","K_*","K_*","K_oE2",

"K_Z","K_X","K_C","K_V","K_B","K_N","K_M","K_COMMA","K_PERIOD",

"K_SLASH","K_*","K_*","K_*","K_*","K_*","K_SPACE"

];

In regard to how they're linked:

Web has a built-in default desktop layout, listing the key IDs at each position.

keyman/common/web/keyboard-processor/src/keyboards/defaultLayouts.ts

Lines 551 to 567 in a024274

"desktop":

{

"defaultHint": 'dot',

"font": "Tahoma,Helvetica",

"layer": [

{

"id": "default",

"row": [

{

"id": 1,

"key": [

{ "id": "K_BKQUOTE" },

{ "id": "K_1" },

{ "id": "K_2" },

{ "id": "K_3" },

{ "id": "K_4" },

{ "id": "K_5" },

When iterating through this default layout, we take the key ID and look up its position among the 65.

keyman/common/web/keyboard-processor/src/keyboards/defaultLayouts.ts

Lines 234 to 247 in a024274

// *** Step 2: Layer objects now exist; time to fill them with the appropriate key labels and key styles ***

for(n=0; n<layers.length; n++) {

var layer=layers[n], kx, shiftKey: LayoutKey = null;

var capsKey: LayoutKey = null, numKey: LayoutKey = null, scrollKey: LayoutKey = null; // null if not in the OSK layout.

var layerSpec = keyLabels[layer['id']];

var isShift = layer['id'] == 'shift' ? 1 : 0;

var isDefault = layer['id'] == 'default' || isShift ? 1 : 0;

rows=layer['row'];

for(i=0; i<rows.length; i++) {

keys=rows[i]['key'];

for(j=0; j<keys.length; j++) {

key=keys[j];

kx=Layouts.dfltCodes.indexOf(key['id']);

keyman-server · 2024-06-03T18:08:10Z

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

keymanapp-test-bot bot added this to the A18S2 milestone May 13, 2024

github-actions bot added web/ common/ common/web/ chore labels May 13, 2024

jahorton commented May 13, 2024

View reviewed changes

jahorton force-pushed the chore/web/update-typescript branch from ecd68fb to 408fe43 Compare May 15, 2024 06:11

jahorton force-pushed the chore/web/partial-config-cleanup branch 2 times, most recently from 369ecb8 to 92ffb33 Compare May 16, 2024 02:37

jahorton force-pushed the chore/web/update-typescript branch from 408fe43 to d70463a Compare May 16, 2024 02:38

jahorton marked this pull request as ready for review May 22, 2024 15:02

jahorton requested review from ermshiperete and mcdurdin as code owners May 22, 2024 15:02

ermshiperete approved these changes May 22, 2024

View reviewed changes

common/web/gesture-recognizer/tsconfig.json Show resolved Hide resolved

mcdurdin requested changes May 23, 2024

View reviewed changes

jahorton commented May 23, 2024

View reviewed changes

mcdurdin reviewed May 23, 2024

View reviewed changes

common/web/input-processor/src/text/prediction/languageProcessor.ts Outdated Show resolved Hide resolved

common/web/input-processor/src/text/prediction/predictionContext.ts Outdated Show resolved Hide resolved

jahorton commented May 23, 2024

View reviewed changes

mcdurdin modified the milestones: A18S2, A18S3 May 24, 2024

jahorton marked this pull request as draft May 28, 2024 02:38

mcdurdin mentioned this pull request May 28, 2024

fix(web): Don't apply suggestion unless fully configured #11594

Merged

jahorton force-pushed the chore/web/partial-config-cleanup branch from 1e08574 to 01be6f6 Compare May 29, 2024 02:38

jahorton changed the base branch from chore/web/update-typescript to refactor/web/typed-property-transplantation May 29, 2024 02:38

jahorton mentioned this pull request May 29, 2024

fix(web): fix keyboard-processing bugs uncovered by stricter TS settings 🔩 #11603

Merged

jahorton added 8 commits May 29, 2024 10:36

chore(web): removes all Web-related implicit-any usage outside of web…

b56628e

…/ and common/web/gesture-recognizer

chore(web): preps gesture-recognizer, but doesn't remove setting

09416e5

chore(web): also, strictFunctionTypes - an easy one at lower-levels

32d72bf

fix(web): gesture-recognizer tests aren't ready yet

35b7ed6

chore(web): noImplicitReturns, strictNullChecks

3530314

chore(web): resolves some nits observed in review

0424441

chore(web): better use of scope within getVKDictionaryCode

2f42f39

fix(web): better suggestionApplier fix for the 'reversion' path

12c91d9

jahorton force-pushed the chore/web/partial-config-cleanup branch from 01be6f6 to 12c91d9 Compare May 29, 2024 03:36

jahorton changed the base branch from refactor/web/typed-property-transplantation to fix/web/kbd-proc-strictness-bugs May 29, 2024 03:37

jahorton requested a review from mcdurdin May 29, 2024 03:37

jahorton marked this pull request as ready for review May 29, 2024 03:37

mcdurdin approved these changes May 30, 2024

View reviewed changes

mcdurdin mentioned this pull request May 30, 2024

refactor(web): hoist KeyboardObject type to common/web/types #11612

Open

jahorton added 3 commits May 30, 2024 16:37

docs(web): documents KeyboardObject properties

61dfcc9

docs(web): fixes detail about readonly keystroke params

10033f4

docs(web): for EncodedVisualKeyboard

487a28d

mcdurdin reviewed May 30, 2024

View reviewed changes

jahorton mentioned this pull request May 31, 2024

fix(web): Await the async predictionContext.resetContext() #11605

Open

docs(web): adds suggested documentation from code review

c4932b7

Co-authored-by: Marc Durdin <marc@durdin.net>

ermshiperete reviewed May 31, 2024

View reviewed changes

docs(web): on KLS format and its arrays of 65 entries

3d77cf2

jahorton changed the title ~~chore(web): remaining low-level Web .tsconfig cleanup 🔩~~ chore(web): clean up remaining low-level Web .tsconfig settings 🔩 Jun 3, 2024

Base automatically changed from fix/web/kbd-proc-strictness-bugs to master June 3, 2024 03:55

chore(web): Merge branch 'master' into chore/web/partial-config-cleanup

46e4518

jahorton merged commit 604d2fc into master Jun 3, 2024
19 checks passed

jahorton deleted the chore/web/partial-config-cleanup branch June 3, 2024 08:16

mcdurdin mentioned this pull request Jun 6, 2024

bug(common): xml2js import in comment/web/types is not building when called as a part of common/web/keyboard-processor #11716

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(web): clean up remaining low-level Web .tsconfig settings 🔩 #11424

chore(web): clean up remaining low-level Web .tsconfig settings 🔩 #11424

jahorton commented May 13, 2024 •

edited

keymanapp-test-bot bot commented May 13, 2024 •

edited

jahorton May 13, 2024

jahorton May 17, 2024

mcdurdin left a comment

jahorton left a comment

mcdurdin left a comment

jahorton left a comment

jahorton commented May 23, 2024 •

edited

mcdurdin commented May 23, 2024

mcdurdin left a comment

mcdurdin May 30, 2024

jahorton May 30, 2024

jahorton May 30, 2024

mcdurdin May 30, 2024

jahorton May 30, 2024 •

edited

jahorton May 30, 2024

mcdurdin May 30, 2024

ermshiperete left a comment

ermshiperete May 31, 2024

jahorton Jun 3, 2024

jahorton Jun 3, 2024

keyman-server commented Jun 3, 2024

	static readonly dfltCodes: ReadonlyArray<string> = [
	"K_BKQUOTE","K_1","K_2","K_3","K_4","K_5","K_6","K_7","K_8","K_9","K_0",
	"K_HYPHEN","K_EQUAL","K_","K_","K_*","K_Q","K_W","K_E","K_R","K_T",
	"K_Y","K_U","K_I","K_O","K_P","K_LBRKT","K_RBRKT","K_BKSLASH","K_*",
	"K_","K_","K_A","K_S","K_D","K_F","K_G","K_H","K_J","K_K","K_L",
	"K_COLON","K_QUOTE","K_","K_","K_","K_","K_*","K_oE2",
	"K_Z","K_X","K_C","K_V","K_B","K_N","K_M","K_COMMA","K_PERIOD",
	"K_SLASH","K_","K_","K_","K_","K_*","K_SPACE"
	];

	"desktop":
	{
	"defaultHint": 'dot',
	"font": "Tahoma,Helvetica",
	"layer": [
	{
	"id": "default",
	"row": [
	{
	"id": 1,
	"key": [
	{ "id": "K_BKQUOTE" },
	{ "id": "K_1" },
	{ "id": "K_2" },
	{ "id": "K_3" },
	{ "id": "K_4" },
	{ "id": "K_5" },

	// * Step 2: Layer objects now exist; time to fill them with the appropriate key labels and key styles *
	for(n=0; n<layers.length; n++) {
	var layer=layers[n], kx, shiftKey: LayoutKey = null;
	var capsKey: LayoutKey = null, numKey: LayoutKey = null, scrollKey: LayoutKey = null; // null if not in the OSK layout.
	var layerSpec = keyLabels[layer['id']];
	var isShift = layer['id'] == 'shift' ? 1 : 0;
	var isDefault = layer['id'] == 'default' \|\| isShift ? 1 : 0;

	rows=layer['row'];
	for(i=0; i<rows.length; i++) {
	keys=rows[i]['key'];
	for(j=0; j<keys.length; j++) {
	key=keys[j];
	kx=Layouts.dfltCodes.indexOf(key['id']);

chore(web): clean up remaining low-level Web .tsconfig settings 🔩 #11424

chore(web): clean up remaining low-level Web .tsconfig settings 🔩 #11424

Conversation

jahorton commented May 13, 2024 • edited

keymanapp-test-bot bot commented May 13, 2024 • edited

User Test Results

Test Artifacts

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcdurdin left a comment

Choose a reason for hiding this comment

jahorton left a comment

Choose a reason for hiding this comment

mcdurdin left a comment

Choose a reason for hiding this comment

jahorton left a comment

Choose a reason for hiding this comment

jahorton commented May 23, 2024 • edited

mcdurdin commented May 23, 2024

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

jahorton May 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ermshiperete 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

keyman-server commented Jun 3, 2024

jahorton commented May 13, 2024 •

edited

keymanapp-test-bot bot commented May 13, 2024 •

edited

jahorton commented May 23, 2024 •

edited

jahorton May 30, 2024 •

edited