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): raise error if virtual key in context string #9908

Merged
merged 2 commits into from
Nov 2, 2023

Conversation

mcdurdin
Copy link
Member

Fixes #7880.

Virtual keys have never been allowed in context. This should be an error in a keyboard. Note: virtual keys in output are officially unsupported but still kinda sorta a little bit work in Windows -- but I doubt they will ever be officially supported.

Also includes small side journey to tidy up names and constants for two other tests (error_duplicate_group and error_duplicate_store).

@keymanapp-test-bot skip

Fixes #7880.

Virtual keys have never been allowed in context. This should be an error
in a keyboard. Note: virtual keys in output are officially unsupported
but still kinda sorta a little bit work in Windows -- but I doubt they
will ever be officially supported.

Also includes small side journey to tidy up names and constants for two
other tests (error_duplicate_group and error_duplicate_store).
@mcdurdin mcdurdin requested a review from darcywong00 as a code owner October 31, 2023 07:06
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Oct 31, 2023

User Test Results

Test specification and instructions

User tests are not required

@@ -54,15 +54,22 @@ describe('CompilerMessages', function () {
// CERR_DuplicateGroup

it('should generate CERR_DuplicateGroup if the kmn contains two groups with the same name', async function() {
await testForMessage(this, ['invalid-keyboards', 'cerr_duplicate_group.kmn'], 0x302071); //TODO: consolidate messages from kmcmplib, CompilerMessages.CERR_DuplicateGroup
await testForMessage(this, ['invalid-keyboards', 'error_duplicate_group.kmn'], KmnCompilerMessages.ERROR_DuplicateGroup); //TODO: consolidate messages from kmcmplib, CompilerMessages.CERR_DuplicateGroup
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
await testForMessage(this, ['invalid-keyboards', 'error_duplicate_group.kmn'], KmnCompilerMessages.ERROR_DuplicateGroup); //TODO: consolidate messages from kmcmplib, CompilerMessages.CERR_DuplicateGroup
await testForMessage(this, ['invalid-keyboards', 'error_duplicate_group.kmn'], KmnCompilerMessages.ERROR_DuplicateGroup);

Comment on lines 7 to 12
store (consonant) "lmn"
store (sylcons) U+1E37 U+1E43 U+1E47


c ERROR_VirtualKeyInContext
[K_BKQUOTE] + any(consonant) > index (sylcons, 2)
Copy link
Member Author

@mcdurdin mcdurdin Nov 2, 2023

Choose a reason for hiding this comment

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

Suggested change
store (consonant) "lmn"
store (sylcons) U+1E37 U+1E43 U+1E47
c ERROR_VirtualKeyInContext
[K_BKQUOTE] + any(consonant) > index (sylcons, 2)
c ERROR_VirtualKeyInContext
[K_BKQUOTE] + 'a' > 'b'

@@ -1316,6 +1316,9 @@ KMX_BOOL CheckContextStatementPositions(PKMX_WCHAR context) {
AddWarningBool(CWARN_IfShouldBeAtStartOfContext);
}
break;
case CODE_EXTENDED:
AddCompileError(CERR_VirtualKeyInContext);
Copy link
Member Author

Choose a reason for hiding this comment

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

Update the function comment to note this test.

Copy link
Contributor

@rc-swag rc-swag left a comment

Choose a reason for hiding this comment

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

lgtm

Base automatically changed from fix/developer/enable-and-update-tests to master November 2, 2023 03:53
@mcdurdin mcdurdin merged commit a98e5f6 into master Nov 2, 2023
2 checks passed
@mcdurdin mcdurdin deleted the fix/developer/7880-error-if-virtual-key-in-context branch November 2, 2023 04:05
@keyman-server
Copy link
Collaborator

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

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

Successfully merging this pull request may close these issues.

bug(developer): Keyman Developer does not flag virtual keycodes in context string as an error
3 participants