fix(developer): correct whitespace handling in virtual keys and remove partially implemented virtual key series in kmcmplib compiler

[markcsinclair]

Fix to the virtual key section of GetXStringImpl() in Compiler.cpp of kmcmplib to correct the whitespace handling (whitespaces were not previously required between modifiers or between modifiers and virtual keys), as well as to remove the partially implemented virtual key series code (multiple keys).

Additional test cases were added to gtest-compiler-test.cpp.

Note: the following keyboards are excluded because the snapshot versions have issues that the compiler now flags:

• basic_kbdcherp: contains [NCAPS SHIFT_K_P], which should not compile (fixed in [basic_targets] Update basic keyboards to include true tablet layout #3056)
• basic_kbdolch: contains [SHIFTK_K], which should not compile (fixed in Correct whitespace error in symbols_key store for basic_kbdolch #3212)

See developer/src/kmcmplib/tests/get-test-source.sh

These keyboards could be included again if we update the snapshot to a later version -- but that requires a rebuild of the fixture expected keyboards with v16.0 compiler, so will necessarily exclude other keyboards which are now v17+.

Fixes: #12307

@keymanapp-test-bot skip

[keymanapp-test-bot]

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

• Developer
  • Keyman Developer
  • Compiler Regression Tests
  • kmcomp.zip
• Keyboards
  • Test Keyboards

[markcsinclair]

Ready to review

I have tried to keep to the original style, but I have tidied up the layout a bit.

[mcdurdin]

Note: we should run this change against the keyboards repository to see if any existing keyboards are impacted.

[mcdurdin]

LGTM. Nice cleanup of some very tired code. Appreciate you treading carefully and matching existing style even though it must have hurt at times 😁

[markcsinclair]

Note: we should run this change against the keyboards repository to see if any existing keyboards are impacted.

I ran it against Windows x86, 'x64' and wasm using ./build.sh --full-test test and on linux using ./build.sh --full-test test:wasm and ./build.sh --full-test test:arch. Is that enough?

[mcdurdin]

I ran it against Windows x86, 'x64' and wasm using ./build.sh --full-test test and on linux using ./build.sh --full-test test:wasm and ./build.sh --full-test test:arch. Is that enough?

Yes, that should do the job I think -- so long as you saw many hundreds of keyboard names scrolling by then I think it should have worked. I haven't run that test for a while (need to add it into an overnight test suite at some point)

[markcsinclair]

Raised issue #12623 as full repository test was not running. Waiting on fixes #12629 #12630 and #12631 so that this fix can be cleanly checked against a full repository test.

[markcsinclair]

With the full_test option is defaulted to true (in kmcmplib/meson_options.txt), it is possible to run the full test without fix #12631

Line 24 of basic_kbdcherp.kmn contains [NCAPS SHIFT_K_P], which should not compile, and now emits an ERROR_InvalidToken
Line 122 of basic_kbdolch.kmn contains [SHIFTK_K], which should not compile, and now emits an ERROR_InvalidToken

If a space is added at the appropriate points ([NCAPS SHIFT _K_P] and [SHIFT K_K]), the files compile and pass the corresponding (full) tests. What is the best way to make these changes in the repository? (Noting that the phrase [NCAPS SHIFT _K_P] is referencing a custom _K_P virtual key.)

[mcdurdin]

If a space is added at the appropriate points ([NCAPS SHIFT _K_P] and [SHIFT K_K]), the files compile and pass the corresponding (full) tests. What is the best way to make these changes in the repository? (Noting that the phrase [NCAPS SHIFT _K_P] is referencing a custom _K_P virtual key.)

_K_P is not a valid key (and the compiler shouldn't really be allowing it). Only keys starting with K_, T_, and U_ are permitted, according to https://help.keyman.com/developer/language/guide/virtual-keys#toc-key-codes.

• basic_kbdcherp.kmn:24 was fixed in keymanapp/keyboards@3962155#diff-f196664c62e9cf760125529b3e4d4e87e0128e595ec05ed7cf2dbb30c0520c0c

• basic_kbdolch.kmn is still broken.

So for basic_kbdolch, we should open a PR on the keyboards repo which:

1. makes the fix to [SHIFT K_K]
   2 makes a version bump. That means updating the &KEYBOARDVERSION store to '1.0.1'
2. adds a line to release/basic/basic_kbdolch/HISTORY.md

It's a good idea to reference this PR in the commit to keyboards repo (keymanapp/keyman#12604).

[markcsinclair]

However, although basic_kbdcherp.kmn is updated (v1.1.0) in my local repository (/c/Projects/Keyman/keyboards), this is not what is being copied into /c/Projects/Keyman/keyman/developer/src/kmcmplib/tests/keyboards after a ./build.sh clean:x86 followed by a ./build.sh --full-test test:x86. Instead, I am getting v.1.0.1, which still has the bug.

There is a file keyboards_commit_ref.txt that contains a commit key, and forces the copy from the local repo to be taken from this (965ef1941f20a3bc0cb5d405792b4984cc46cd52) in the checkout_keyboards() function in checkout-keyboards.inc.sh. It dates from 16 Mar 2023, entitled chore: fix rac_wakhi encoding to utf-8.

[mcdurdin]

However, although basic_kbdcherp.kmn is updated (v1.1.0) in my local repository (/c/Projects/Keyman/keyboards), this is not what is being copied into /c/Projects/Keyman/keyman/developer/src/kmcmplib/tests/keyboards after a ./build.sh clean:x86 followed by a ./build.sh --full-test test:x86. Instead, I am getting v.1.0.1, which still has the bug.

There is a file keyboards_commit_ref.txt that contains a commit key, and forces the copy from the local repo to be taken from this (965ef1941f20a3bc0cb5d405792b4984cc46cd52) in the checkout_keyboards() function in checkout-keyboards.inc.sh. It dates from 16 Mar 2023, entitled chore: fix rac_wakhi encoding to utf-8.

Ah yes, sorry. See #12634 for a chore to update to a later version of the keyboards. Delayed this until 19.0 because it requires careful inspection to ensure that we don't accidentally propagate broken keyboard tests. The original purpose of this test was to ensure that the new 17.0 kmcmplib compiler rewrite did not diverge from the legacy 16.0 kmcmpdll compiler, so it is comparing against fixtures compiled with the 16.0 kmcmpdll compiler. (So we'd need to rebuild all the fixtures with the legacy compiler, at least those that are not using 17.0 features, etc.)

We may want to, at some point, consider another test for 'can we compile all current keyboards in master on keyboards repo', but that's not a really a reproducible test given it is a moving target.

Given all that, I think we add basic_kbdcherp and basic_kbdolch to the list of keyboards to skip testing for at present, like we did in #12631 for anii, sil_kmhmu, fv_statimcets, and fv_nuucaanul. See get-test-source.sh change in https://github.com/keymanapp/keyman/pull/12631/files#diff-35a7922a6e67f3fe139e97df293d2b55d6bd00867c864c2cb6ab513126b2dea3R12

[mcdurdin]

#12631 is now merged; I have updated this branch to pull changes and rebuild.

[markcsinclair]

Re-review, as it is now three weeks since approval. Changes here are just exclusion of the two errored keyboards in get-test-source.sh.

Re-review requested

[mcdurdin]

LGTM

[SabineSIL]

LGTM also

[keyman-server]

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