Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

mcdurdin
Copy link
Member

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.

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 chore(common): xml2js -> fast-xml-parser #12331.
  2. XML errors were not captured in the LDML XML reader. See also chore(common): xml2js -> fast-xml-parser #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 epic: web-core 🎼 #12291.

Fixes: #12395
Cherry-pick-of: #12402

User Testing

Preparation:

  1. Install Keyman Developer artifact from this PR.
  2. 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.

* 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
@keymanapp-test-bot keymanapp-test-bot bot changed the title fix(developer): rewrite ldml visual keyboard compiler 🍒 fix(developer): rewrite ldml visual keyboard compiler 🍒 🏠 Sep 12, 2024
@keymanapp-test-bot keymanapp-test-bot bot added this to the A18S10 milestone Sep 12, 2024
Copy link
Member

@srl295 srl295 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSLGTM

@dinakaranr
Copy link

Test Results

I tested this issue with the attached "Keyman developer 17.0.330-test-12406" build on 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 I am sharing my observation.

  • TEST_LDML_VISUAL_KEYBOARD_WINDOWS (Passed):
  1. Installed the Keyman 17.0.329 keyboard build.
  2. Installed the "imperial_aramaic.kmp" keyboard.
  3. Open the Notepad application.
  4. Open the OSK from the window tray icon.
  5. Select the imperial_aramaic keyboard.
  6. Enter some text in the notepad
  7. Verified the OSK shows the imperial_aramaic letters.
    It works well. Thank you.
  • TEST_LDML_VISUAL_KEYBOARD_MAC (Failed):
  1. Installed the Keyman 17.0.329 keyboard build.
  2. Installed the "imperial_aramaic.kmp" keyboard.
  3. Open the "TextEdit" application.
  4. Open the OSK from the toolbar.
  5. Select the imperial_aramaic keyboard.
  6. Enter some text in the "TextEdit"
  7. Verified the OSK does not show the imperial_aramaic letters.
    It seems to be a problem. Thank you.
    Please refer to the screenshot below
  • TEST_LDML_VISUAL_KEYBOARD_LINUX (Passed):
  1. Installed the Keyman 17.0.329 keyboard build.
  2. Installed the "imperial_aramaic.kmp" keyboard.
  3. Open the "TextEditor" application.
  4. Open the OSK from the toolbar.
  5. Select the imperial_aramaic keyboard.
  6. Enter some text in the "TextEditor".
  7. Verified the OSK shows the imperial_aramaic letters.
    It works well. Thank you.

@keymanapp-test-bot keymanapp-test-bot bot added user-test-failed and removed user-test-required User tests have not been completed labels Sep 13, 2024
@darcywong00 darcywong00 modified the milestones: A18S10, A18S11 Sep 14, 2024
@mcdurdin
Copy link
Member Author

@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

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

Thank you for fixing this issue.

Test Results

I tested this issue with the attached "Keyman developer 17.0.330-test-12406" 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):
  1. Installed the Keyman 17.0.329 keyboard build.
  2. Installed the "imperial_aramaic.kmp" keyboard.
  3. Open the "TextEdit" application.
  4. Open the OSK from the toolbar.
  5. Select the imperial_aramaic keyboard.
  6. Enter some text in the "TextEdit"
  7. Verified the OSK appeared and the imperial_aramaic letters displayed
  8. It works well. Thank you.
    Please refer to the screenshot below

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Sep 16, 2024
@mcdurdin mcdurdin merged commit 1acdc5b into stable-17.0 Sep 16, 2024
19 checks passed
@mcdurdin mcdurdin deleted the fix/developer/cherry-pick/12395-rewrite-ldml-visual-keyboard-compiler branch September 16, 2024 22:40
@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 17.0.330

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

bug(developer): generation of KVK from LDML keyboard seems to be creating an invalid .kvk file
5 participants