search

keymanapp / keyman

Keyman cross platform input methods system running on Android, iOS, Linux, macOS, Windows and mobile and desktop web
https://keyman.com/
Other
402 stars 112 forks source link

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

Open markcsinclair opened 3 weeks ago

markcsinclair commented 3 weeks ago

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:

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[bot] commented 3 weeks ago

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

markcsinclair commented 3 weeks ago

Ready to review

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

mcdurdin commented 2 weeks ago

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

markcsinclair commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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.

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 1 week ago

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