fix(developer): rewrite ldml visual keyboard compiler #12402

mcdurdin · 2024-09-11T23:17:30Z

The visual keyboard compiler was never finished in 17.0. This rewrites it to:

Use the kmxplus data rather than reading from xml directly
Fill in visualkeyboard.header.kbdname
Support modifiers
Handle encoded characters like \u{1234}
Handle string variables like ${one} (see TODO below)

Additional unit tests have been added to verify the behavior of the visual keyboard compiler in more detail.

TODO-LDML: string variables appear to have a secondary bug -- they seem to be returning the string 'undefined'. I have disabled the related tests and will examine this separately, and enable those tests once fixed. See #12403.

TODO-LDML: we should probably add a compiler warning + unit test for <layers formId="us"><layer id="base">, because this pattern does not make sense: when using non-touch forms, the <layer> element should use modifiers attribute, and correspondingly, modifiers attribute should not be used when formId is touch. See #12423.

Other fixes:

The LDML XML reader was relying on its input being a Node.js Buffer even though it was declared Uint8Array, as it implicitly used Buffer.toString() to do text conversion. (Buffer subclasses from Uint8Array). This breaks when using Uint8Array directly and means we had an implicit dependency on Node.js. See also chore(common): xml2js -> fast-xml-parser #12331.
XML errors were not captured in the LDML XML reader. See also chore(common): xml2js -> fast-xml-parser #12331.
The unused and unfinished touch-layout-compiler.ts and keymanweb-compiler.ts have been removed along with corresponding unit tests and fixtures. These are replaced by Core implementations; see epic: web-core 🎼 #12291.

Will be cherry-picked to stable-17.0 (probably together with resolution to string variable bug)

Fixes: #12395

User Testing

Preparation:

Install Keyman Developer artifact from this PR.
Open the release/i/imperial_aramaic keyboards project in Keyman Developer, compile it, and locate the resulting build/imperial_aramaic.kmp.

TEST_LDML_VISUAL_KEYBOARD_WINDOWS: Install the imperial_aramaic.kmp file into Keyman for Windows 17.0.329. Verify that the On Screen Keyboard displays Aramaic characters.

TEST_LDML_VISUAL_KEYBOARD_MAC: Install the imperial_aramaic.kmp file into Keyman for macOS 17.0.329. Verify that the On Screen Keyboard displays Aramaic characters.

TEST_LDML_VISUAL_KEYBOARD_LINUX: Install the imperial_aramaic.kmp file into Keyman for Linux 17.0.329. Verify that the On Screen Keyboard displays Aramaic characters.

The visual keyboard compiler was never finished in 17.0. This rewrites it to: 1. Use the kmxplus data rather than reading from xml directly 2. Fill in `visualkeyboard.header.kbdname` 3. Support modifiers 4. Handle encoded characters like `\u{1234}` 5. Handle string variables like `${one}`* Additional unit tests have been added to verify the behavior of the visual keyboard compiler in more detail. TODO-LDML: string variables appear to have a secondary bug -- they seem to be returning the string 'undefined'. I have disabled the related tests and will examine this separately, and enable those tests once fixed. TODO-LDML: we should probably add a compiler warning + unit test for `<layers formId="us"><layer id="base">`, because this pattern does not make sense: when using non-touch forms, the `<layer>` element should use `modifiers` attribute, and correspondingly, `modifiers` attribute should _not_ be used when `formId` is `touch`. Other fixes: 1. The LDML XML reader was relying on its input being a Node.js `Buffer` even though it was declared `Uint8Array`, as it implicitly used `Buffer.toString()` to do text conversion. (`Buffer` subclasses from `Uint8Array`). This breaks when using `Uint8Array` directly and means we had an implicit dependency on Node.js. See also #12331. 2. XML errors were not captured in the LDML XML reader. See also #12331. 3. The unused and unfinished touch-layout-compiler.ts and keymanweb-compiler.ts have been removed along with corresponding unit tests and fixtures. These are replaced by Core implementations; see #12291. Fixes: #12395

keymanapp-test-bot · 2024-09-11T23:17:37Z

User Test Results

Test specification and instructions

✅ TEST_LDML_VISUAL_KEYBOARD_WINDOWS (PASSED) (notes)
✅ TEST_LDML_VISUAL_KEYBOARD_MAC (PASSED) (notes)
✅ TEST_LDML_VISUAL_KEYBOARD_LINUX (PASSED) (notes)

Test Artifacts

Developer
Keyboards
- Test Keyboards

mcdurdin · 2024-09-11T23:20:07Z

developer/src/kmc-ldml/src/compiler/compiler.ts

@@ -146,7 +153,7 @@ export class LdmlKeyboardCompiler implements KeymanCompiler {
    const kmx_binary = builder.compile();

    const vkcompiler = new LdmlKeyboardVisualKeyboardCompiler(this.callbacks);
-    const vk = vkcompiler.compile(source);
+    const vk = vkcompiler.compile(kmx.kmxplus, this.callbacks.path.basename(outputFilename, '.kmx'));


Suggested change

const vk = vkcompiler.compile(kmx.kmxplus, this.callbacks.path.basename(outputFilename, '.kmx'));

const vk = vkcompiler.compile(kmx.kmxplus, outputFilename.replace(/\.kmx$/, ''));

Matches pattern for setting outputFilename -- we could add a baseFilename variable instead...

mcdurdin · 2024-09-11T23:22:19Z

developer/src/kmc-ldml/test/helpers/index.ts

-  if(compilerTestCallbacks.messages.length > 0) {
-    console.log(compilerTestCallbacks.messages);
-  }


The messages are printed already in the afterEach() function.

darcywong00 · 2024-09-12T02:54:54Z

developer/src/kmc-ldml/test/fixtures/basic.xml

@@ -23,7 +24,8 @@
  </keys>

  <layers formId="us" minDeviceWidth="123">
-    <layer id="base">
+    <!-- TODO-LDML: unit test for <layers formId="us" with <layer id="base" should be illegal -->
+    <layer modifiers="none">


Use the kmxplus data rather than reading from xml directly

Does this mean the test fixture should use basic.kmx?

No -- this is a separate e2e test for the entire ldml compiler.

The visual keyboard compiler was never finished in 17.0. This rewrites it to: 1. Use the kmxplus data rather than reading from xml directly 2. Fill in `visualkeyboard.header.kbdname` 3. Support modifiers 4. Handle encoded characters like `\u{1234}` 5. Handle string variables like `${one}`* Additional unit tests have been added to verify the behavior of the visual keyboard compiler in more detail. * String variable tests will be enabled in next commit (which is a cherry-pick of #12404). Other fixes: 1. The LDML XML reader was relying on its input being a Node.js `Buffer` even though it was declared `Uint8Array`, as it implicitly used `Buffer.toString()` to do text conversion. (`Buffer` subclasses from `Uint8Array`). This breaks when using `Uint8Array` directly and means we had an implicit dependency on Node.js. See also #12331. 2. XML errors were not captured in the LDML XML reader. See also #12331. 3. The unused and unfinished touch-layout-compiler.ts and keymanweb-compiler.ts have been removed along with corresponding unit tests and fixtures. These will be replaced by Core implementations; see #12291. Fixes: #12395 Cherry-pick-of: #12402

darcywong00 · 2024-09-12T03:37:13Z

developer/src/kmc-ldml/test/test-visual-keyboard-compiler.ts

+    assert.equal(vk.keys[3].shift, VisualKeyboard.VisualKeyboardShiftState.KVKS_RCTRL | VisualKeyboard.VisualKeyboardShiftState.KVKS_RALT);
+    assert.equal(vk.keys[4].shift, VisualKeyboard.VisualKeyboardShiftState.KVKS_SHIFT | VisualKeyboard.VisualKeyboardShiftState.KVKS_RALT);


ok, so do these correspond to

<layer modifiers="ctrlR altR, shift altR"><row keys="x" /></layer>

and then

<layer modifiers="caps"><row keys="x" /></layer>

has a .shift = null?

Yes, the OSK does not support Caps so we omit the Caps layer.

darcywong00

lgtm

srl295 · 2024-09-12T13:52:52Z

developer/src/common/web/utils/src/common-messages.ts

+  static ERROR_InvalidXml = SevError | 0x0008;
+  static Error_InvalidXml = (o:{e: any}) =>
+  m(this.ERROR_InvalidXml, `The XML file could not be read: ${(o.e ?? '').toString()}`);


https://github.com/keymanapp/keyman/pull/12331/files#diff-2b4b31c2a094a648cb0ff6d73cc1811eb465f1f15ba7bb6ea4f5a3795327cbafR47-R57 😿

Your change definitely trumps mine, mine was a stopgap for getting this across the line 😁

srl295

LGTM!!

dinakaranr · 2024-09-13T13:03:33Z

Test Results

I tested this issue with the attached "Keyman developer 18.0.107-alpha-test-12402" build on the Windows 10.
Open the imperial_aramaic.kps project file.
Compile the build and generate the keyboard build.
Copy the imperial_aramaic.kmp file and share it to Windows, MacOS, and Linux.
Here is my observation.

TEST_LDML_VISUAL_KEYBOARD_WINDOWS (Passed):

Installed the Keyman 17.0.329 keyboard build.
Installed the "imperial_aramaic.kmp" keyboard.
Open the Notepad application.
Open the OSK from the window tray icon.
Select the imperial_aramaic keyboard.
Enter some text in the notepad
Verified the OSK shows the imperial_aramaic letters.
It works well. Thank you.

TEST_LDML_VISUAL_KEYBOARD_MAC (Failed):

Installed the Keyman 17.0.329 keyboard build.
Installed the "imperial_aramaic.kmp" keyboard.
Open the "TextEdit" application.
Open the OSK from the toolbar.
Select the imperial_aramaic keyboard.
Enter some text in the "TextEdit"
Verified the OSK does not show the imperial_aramaic letters.
It seems to be a problem. Thank you.

TEST_LDML_VISUAL_KEYBOARD_LINUX (Passed):

Installed the Keyman 17.0.329 keyboard build.
Installed the "imperial_aramaic.kmp" keyboard.
Open the "TextEditor" application.
Open the OSK from the toolbar.
Select the imperial_aramaic keyboard.
Enter some text in the "TextEditor".
Verified the OSK shows the imperial_aramaic letters.
It works well. Thank you.

…iling visual keyboard Fixes: #12395

mcdurdin · 2024-09-16T07:02:08Z

@dinakaranr thank you for that test. I have addressed an issue in the compiler, so can you please retest on the Mac? Make sure you rebuild the keyboard before copying to the Mac for testing. Thanks!

@keymanapp-test-bot retest TEST_LDML_VISUAL_KEYBOARD_MAC

…iling visual keyboard Fixes: #12395 Cherry-pick-of: #12402

dinakaranr · 2024-09-16T14:47:26Z

Thank you for fixing this issue.

Test Results

I tested this issue with the attached "Keyman developer 18.0.107-alpha-test-12402" build on Windows 10.
Open the imperial_aramaic.kps project file.
Compile and regenerate the keyboard build.
Copy the imperial_aramaic.kmp file and share it to Windows, MacOS, and Linux.
Here I am sharing my observation.

TEST_LDML_VISUAL_KEYBOARD_MAC (Passed):

Installed the Keyman 17.0.329 keyboard build.
Installed the "imperial_aramaic.kmp" keyboard.
Open the "TextEdit" application.
Open the OSK from the toolbar.
Select the imperial_aramaic keyboard.
Enter some text in the "TextEdit"
Verified the OSK appeared and the imperial_aramaic letters displayed
It works well. Thank you.

keyman-server · 2024-09-17T18:03:04Z

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

mcdurdin requested a review from darcywong00 as a code owner September 11, 2024 23:17

keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed labels Sep 11, 2024

keymanapp-test-bot bot added this to the A18S10 milestone Sep 11, 2024

github-actions bot added developer/ developer/compilers/ labels Sep 11, 2024

mcdurdin requested a review from srl295 September 11, 2024 23:17

github-actions bot added the fix label Sep 11, 2024

mcdurdin commented Sep 11, 2024

View reviewed changes

mcdurdin mentioned this pull request Sep 12, 2024

bug(developer): ldml string variables referenced before definition have 'undefined' string value #12403

Closed

darcywong00 reviewed Sep 12, 2024

View reviewed changes

mcdurdin mentioned this pull request Sep 12, 2024

fix(developer): rewrite ldml visual keyboard compiler 🍒 🏠 #12406

Merged

darcywong00 reviewed Sep 12, 2024

View reviewed changes

darcywong00 approved these changes Sep 12, 2024

View reviewed changes

srl295 reviewed Sep 12, 2024

View reviewed changes

srl295 approved these changes Sep 12, 2024

View reviewed changes

keymanapp-test-bot bot added user-test-failed and removed user-test-required User tests have not been completed labels Sep 13, 2024

darcywong00 modified the milestones: A18S10, A18S11 Sep 14, 2024

mcdurdin mentioned this pull request Sep 15, 2024

bug(developer): add ldml compiler warning for <layers formId="us"><layer id="base"> pattern #12423

Open

fix(developer): LDML compiler, add TSS_VISUALKEYBOARD store when comp…

f4321b3

…iling visual keyboard Fixes: #12395

keymanapp-test-bot bot added user-test-required User tests have not been completed and removed user-test-failed labels Sep 16, 2024

mcdurdin added a commit that referenced this pull request Sep 16, 2024

fix(developer): LDML compiler, add TSS_VISUALKEYBOARD store when comp…

94a4ac4

…iling visual keyboard Fixes: #12395 Cherry-pick-of: #12402

keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Sep 16, 2024

mcdurdin merged commit 6b83589 into master Sep 16, 2024
6 checks passed

mcdurdin deleted the fix/developer/12395-rewrite-ldml-visual-keyboard-compiler branch September 16, 2024 22:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(developer): rewrite ldml visual keyboard compiler #12402

fix(developer): rewrite ldml visual keyboard compiler #12402

mcdurdin commented Sep 11, 2024 •

edited

Loading

keymanapp-test-bot bot commented Sep 11, 2024 •

edited

Loading

mcdurdin Sep 11, 2024

mcdurdin Sep 11, 2024

darcywong00 Sep 12, 2024

mcdurdin Sep 12, 2024

darcywong00 Sep 12, 2024

mcdurdin Sep 12, 2024

darcywong00 left a comment

srl295 Sep 12, 2024

srl295 Sep 12, 2024

mcdurdin Sep 13, 2024

srl295 left a comment

dinakaranr commented Sep 13, 2024

mcdurdin commented Sep 16, 2024

dinakaranr commented Sep 16, 2024

keyman-server commented Sep 17, 2024

	const vk = vkcompiler.compile(kmx.kmxplus, this.callbacks.path.basename(outputFilename, '.kmx'));
	const vk = vkcompiler.compile(kmx.kmxplus, outputFilename.replace(/\.kmx$/, ''));

		assert.equal(vk.keys[3].shift, VisualKeyboard.VisualKeyboardShiftState.KVKS_RCTRL \| VisualKeyboard.VisualKeyboardShiftState.KVKS_RALT);
		assert.equal(vk.keys[4].shift, VisualKeyboard.VisualKeyboardShiftState.KVKS_SHIFT \| VisualKeyboard.VisualKeyboardShiftState.KVKS_RALT);

fix(developer): rewrite ldml visual keyboard compiler #12402

fix(developer): rewrite ldml visual keyboard compiler #12402

Conversation

mcdurdin commented Sep 11, 2024 • edited Loading

User Testing

keymanapp-test-bot bot commented Sep 11, 2024 • edited Loading

User Test Results

Test Artifacts

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

darcywong00 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

srl295 left a comment

Choose a reason for hiding this comment

dinakaranr commented Sep 13, 2024

Test Results

mcdurdin commented Sep 16, 2024

dinakaranr commented Sep 16, 2024

Test Results

keyman-server commented Sep 17, 2024

mcdurdin commented Sep 11, 2024 •

edited

Loading

keymanapp-test-bot bot commented Sep 11, 2024 •

edited

Loading