Skip to content

Commit

Permalink
Merge pull request #12604 from keymanapp/fix/developer/12307-correct-…
Browse files Browse the repository at this point in the history
…whitespace-handling-in-virtual-keys-and-remove-partially-implemented-virtual-key-series

fix(developer): correct whitespace handling in virtual keys and remove partially implemented virtual key series in kmcmplib compiler
  • Loading branch information
markcsinclair authored Nov 25, 2024
2 parents c80a0ae + ab9b052 commit 09dc2e0
Show file tree
Hide file tree
Showing 3 changed files with 72 additions and 69 deletions.
96 changes: 50 additions & 46 deletions developer/src/kmcmplib/src/Compiler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2034,6 +2034,7 @@ KMX_DWORD GetXStringImpl(PKMX_WCHAR tstr, PFILE_KEYBOARD fk, PKMX_WCHAR str, KMX
int type, mx = 0, n, n1, n2, tokenFound = FALSE, z, sFlag = 0, j;
KMX_DWORD i;
KMX_BOOL finished = FALSE;
KMX_BOOL wsRequired = FALSE;
KMX_WCHAR c;

*tstr = 0;
Expand Down Expand Up @@ -2412,50 +2413,53 @@ KMX_DWORD GetXStringImpl(PKMX_WCHAR tstr, PFILE_KEYBOARD fk, PKMX_WCHAR str, KMX
}
continue;
case 11:
p++; sFlag = ISVIRTUALKEY /* 0 */; finished = FALSE;
p++; sFlag = ISVIRTUALKEY /* 0 */; finished = FALSE; wsRequired = FALSE;

//printf("--EXTENDEDSTRING--\n");

do
{
if (wsRequired && !iswspace(*p))
return KmnCompilerMessages::ERROR_InvalidToken; // #12307

while (iswspace(*p)) p++;

switch (towupper(*p))
{
case 'N':
if (u16nicmp(p, u"NCAPS", 5) == 0)
sFlag |= NOTCAPITALFLAG, p += 5;
sFlag |= NOTCAPITALFLAG, wsRequired = TRUE, p += 5;
else finished = TRUE;
break;
case 'L':
if (u16nicmp(p, u"LALT", 4) == 0)
sFlag |= LALTFLAG, p += 4;
sFlag |= LALTFLAG, wsRequired = TRUE, p += 4;
else if (u16nicmp(p, u"LCTRL", 5) == 0)
sFlag |= LCTRLFLAG, p += 5;
sFlag |= LCTRLFLAG, wsRequired = TRUE, p += 5;
else finished = TRUE;
break;
case 'R':
if (u16nicmp(p, u"RALT", 4) == 0)
sFlag |= RALTFLAG, p += 4;
sFlag |= RALTFLAG, wsRequired = TRUE, p += 4;
else if (u16nicmp(p, u"RCTRL", 5) == 0)
sFlag |= RCTRLFLAG, p += 5;
sFlag |= RCTRLFLAG, wsRequired = TRUE, p += 5;
else finished = TRUE;
break;
case 'A':
if (u16nicmp(p, u"ALT", 3) == 0)
sFlag |= K_ALTFLAG, p += 3;
sFlag |= K_ALTFLAG, wsRequired = TRUE, p += 3;
else finished = TRUE;
break;
case 'C':
if (u16nicmp(p, u"CTRL", 4) == 0)
sFlag |= K_CTRLFLAG, p += 4;
sFlag |= K_CTRLFLAG, wsRequired = TRUE, p += 4;
else if (u16nicmp(p, u"CAPS", 4) == 0)
sFlag |= CAPITALFLAG, p += 4;
sFlag |= CAPITALFLAG, wsRequired = TRUE, p += 4;
else finished = TRUE;
break;
case 'S':
if (u16nicmp(p, u"SHIFT", 5) == 0)
sFlag |= K_SHIFTFLAG, p += 5;
sFlag |= K_SHIFTFLAG, wsRequired = TRUE, p += 5;
else finished = TRUE;
break;
default:
Expand Down Expand Up @@ -2489,60 +2493,58 @@ KMX_DWORD GetXStringImpl(PKMX_WCHAR tstr, PFILE_KEYBOARD fk, PKMX_WCHAR str, KMX
tstr[mx++] = CODE_EXTENDED;
tstr[mx++] = sFlag;

while (iswspace(*p)) p++;

q = p;

if (*q == ']')
{
if (*q == ']') {
return KmnCompilerMessages::ERROR_InvalidToken; // I3137 - key portion of VK is missing e.g. "[CTRL ALT]", this generates invalid kmx file that can crash Keyman or compiler later on // I3511
}

while (*q != ']')
{
if (*q == '\'' || *q == '"')
{
if(!VerifyKeyboardVersion(fk, VERSION_60)) {
return KmnCompilerMessages::ERROR_60FeatureOnly_VirtualCharKey;
}
if (!kmcmp::FMnemonicLayout) {
ReportCompilerMessage(KmnCompilerMessages::WARN_VirtualCharKeyWithPositionalLayout);
}
KMX_WCHAR chQuote = *q;
q++; if (*q == chQuote || *q == '\n' || *q == 0) return KmnCompilerMessages::ERROR_InvalidToken;
tstr[mx - 1] |= VIRTUALCHARKEY;
tstr[mx++] = *q;
q++; if (*q != chQuote) return KmnCompilerMessages::ERROR_InvalidToken;
q++;
while (iswspace(*q)) q++;
if (*q != ']') return KmnCompilerMessages::ERROR_InvalidToken;
break; /* out of while loop */
if (*q == '\'' || *q == '"') {
if(!VerifyKeyboardVersion(fk, VERSION_60)) {
return KmnCompilerMessages::ERROR_60FeatureOnly_VirtualCharKey;
}

if (!kmcmp::FMnemonicLayout) {
ReportCompilerMessage(KmnCompilerMessages::WARN_VirtualCharKeyWithPositionalLayout);
}

KMX_WCHAR chQuote = *q;

q++; // skip quote
if (*q == chQuote || *q == '\n' || *q == 0)
return KmnCompilerMessages::ERROR_InvalidToken;

tstr[mx - 1] |= VIRTUALCHARKEY;
tstr[mx++] = *q;

q++; // skip key
if (*q != chQuote)
return KmnCompilerMessages::ERROR_InvalidToken;
q++; // skip quote
} else {
for (j = 0; !iswspace(*q) && *q != ']' && *q != 0; q++, j++);

if (*q == 0) return KmnCompilerMessages::ERROR_InvalidToken;
if (*q == 0)
return KmnCompilerMessages::ERROR_InvalidToken;

KMX_WCHAR vkname[SZMAX_VKDICTIONARYNAME]; // I3438

if (j >= SZMAX_VKDICTIONARYNAME) return KmnCompilerMessages::ERROR_InvalidToken;
if (j >= SZMAX_VKDICTIONARYNAME)
return KmnCompilerMessages::ERROR_InvalidToken;

u16ncpy(vkname, p, j); // I3481
u16ncpy(vkname, p, j); // I3481
vkname[j] = 0;

if (u16icmp(vkname, u"K_NPENTER") == 0)
i = 5; // I649 - K_NPENTER hack
else
{
for (i = 0; i <= VK__MAX; i++)
{
else {
for (i = 0; i <= VK__MAX; i++) {
if (u16icmp(vkname, VKeyNames[i]) == 0 || u16icmp(vkname, VKeyISO9995Names[i]) == 0)
break;
}
}

if (i == VK__MAX + 1)
{
if (i == VK__MAX + 1) {
if(!VerifyKeyboardVersion(fk, VERSION_90)) {
return KmnCompilerMessages::ERROR_InvalidToken;
}
Expand All @@ -2552,16 +2554,18 @@ KMX_DWORD GetXStringImpl(PKMX_WCHAR tstr, PFILE_KEYBOARD fk, PKMX_WCHAR str, KMX
return KmnCompilerMessages::ERROR_InvalidToken;
}

p = q;

tstr[mx++] = (int)i;

if (kmcmp::FMnemonicLayout && (i <= VK__MAX) && VKeyMayBeVCKey[i]) {
ReportCompilerMessage(KmnCompilerMessages::WARN_VirtualKeyWithMnemonicLayout); // I3438
}

while (iswspace(*q)) q++;
}

while (iswspace(*q)) q++;

if (*q != ']')
return KmnCompilerMessages::ERROR_InvalidToken;

tstr[mx++] = UC_SENTINEL_EXTENDEDEND;
tstr[mx] = 0;
//printf("--EXTENDEDEND--\n");
Expand Down
6 changes: 4 additions & 2 deletions developer/src/kmcmplib/tests/get-test-source.sh
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,10 @@
set -eu
find "$1" -name '*.kmn' | \
grep -E '(release|experimental)/([a-z0-9_]+)/([a-z0-9_]+)/source/\3\.kmn$' | \
grep -vE 'masaram_gondi|anii|sil_kmhmu|fv_statimcets|fv_nuucaanul'
grep -vE 'masaram_gondi|anii|sil_kmhmu|fv_statimcets|fv_nuucaanul|basic_kbdcherp|basic_kbdolch'
# #12623: exclude masaram_gondi due to #11806
# #12631: exclude anii, sil_kmhmu as ico references have mismatching case
# #12631: exclude fv_statimcets, fv_nuucaanul as these include U+2002 which is not
# treated as whitespace on mac arch
# treated as whitespace on mac arch
# #12604: exclude basic_kbdcherp, basic_kbdolch as these do not include now necessary
# whitespace, see also issue #12307
39 changes: 18 additions & 21 deletions developer/src/kmcmplib/tests/gtest-compiler-test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1807,25 +1807,25 @@ TEST_F(CompilerTest, GetXStringImpl_type_osb_test) {
u16cpy(str, u"[CTRL ALT]");
EXPECT_EQ(KmnCompilerMessages::ERROR_InvalidToken, GetXStringImpl(tstr, &fileKeyboard, str, u"", output, 80, 0, &newp, FALSE));

// virtual key, modifiers only (no key portion), space after, KmnCompilerMessages::ERROR_InvalidToken
fileKeyboard.version = VERSION_90;
u16cpy(str, u"[CTRL ALT ]");
EXPECT_EQ(KmnCompilerMessages::ERROR_InvalidToken, GetXStringImpl(tstr, &fileKeyboard, str, u"", output, 80, 0, &newp, FALSE));

// virtual key, in VKeyNames, no space between modifier and key portion, ERROR_InvalidToken (see #12307)
// fileKeyboard.version = VERSION_90;
// u16cpy(str, u"[CTRLK_A]");
// EXPECT_EQ(KmnCompilerMessages::ERROR_InvalidToken, GetXStringImpl(tstr, &fileKeyboard, str, u"", output, 80, 0, &newp, FALSE));
fileKeyboard.version = VERSION_90;
u16cpy(str, u"[CTRLK_A]");
EXPECT_EQ(KmnCompilerMessages::ERROR_InvalidToken, GetXStringImpl(tstr, &fileKeyboard, str, u"", output, 80, 0, &newp, FALSE));

// virtual key, in VKeyNames, no space between modifiers, ERROR_InvalidToken (see #12307)
// fileKeyboard.version = VERSION_90;
// u16cpy(str, u"[CTRLALT K_A]");
// EXPECT_EQ(KmnCompilerMessages::ERROR_InvalidToken, GetXStringImpl(tstr, &fileKeyboard, str, u"", output, 80, 0, &newp, FALSE));
fileKeyboard.version = VERSION_90;
u16cpy(str, u"[CTRLALT K_A]");
EXPECT_EQ(KmnCompilerMessages::ERROR_InvalidToken, GetXStringImpl(tstr, &fileKeyboard, str, u"", output, 80, 0, &newp, FALSE));

// virtual key, '_' between modifier and key portion', ERROR_InvalidToken (see #12307)
// fileKeyboard.version = VERSION_90;
// u16cpy(str, u"[CTRL_K_A]");
// EXPECT_EQ(STATUS_Success, GetXStringImpl(tstr, &fileKeyboard, str, u"", output, 80, 0, &newp, FALSE));
// tstr_virtual_key_valid[2] = ISVIRTUALKEY | K_CTRLFLAG;
// tstr_virtual_key_valid[3] = 259; // VK__MAX + 4
// EXPECT_EQ(0, u16cmp(tstr_virtual_key_valid, tstr));
// EXPECT_EQ(0, msgproc_errors.size());
// tstr_virtual_key_valid[3] = 65;
fileKeyboard.version = VERSION_90;
u16cpy(str, u"[CTRL_K_A]");
EXPECT_EQ(KmnCompilerMessages::ERROR_InvalidToken, GetXStringImpl(tstr, &fileKeyboard, str, u"", output, 80, 0, &newp, FALSE));

// virtual char key, single quote, ERROR_60FeatureOnly_VirtualCharKey
fileKeyboard.version = VERSION_50;
Expand Down Expand Up @@ -2029,13 +2029,10 @@ TEST_F(CompilerTest, GetXStringImpl_type_osb_test) {
EXPECT_EQ(KmnCompilerMessages::WARN_VirtualKeyWithMnemonicLayout, msgproc_errors[0].errorCode);
msgproc_errors.clear();

// virtual key, in VKeyNames, multiple keys, valid (see #12307)
// fileKeyboard.version = VERSION_90;
// u16cpy(str, u"[K_A K_B K_C]");
// EXPECT_EQ(STATUS_Success, GetXStringImpl(tstr, &fileKeyboard, str, u"", output, 80, 0, &newp, FALSE));
// sFlag = ISVIRTUALKEY;
// KMX_WCHAR tstr_virtual_key_multi_valid[] = { UC_SENTINEL, CODE_EXTENDED, sFlag, 65, 66, 67, UC_SENTINEL_EXTENDEDEND, 0 };
// EXPECT_EQ(0, u16cmp(tstr_virtual_key_multi_valid, tstr));
// virtual key, in VKeyNames, multiple keys, KmnCompilerMessages::ERROR_InvalidToken (see #12307)
fileKeyboard.version = VERSION_90;
u16cpy(str, u"[K_A K_B]");
EXPECT_EQ(KmnCompilerMessages::ERROR_InvalidToken, GetXStringImpl(tstr, &fileKeyboard, str, u"", output, 80, 0, &newp, FALSE));
}

// KMX_DWORD process_baselayout(PFILE_KEYBOARD fk, PKMX_WCHAR q, PKMX_WCHAR tstr, int *mx)
Expand Down

0 comments on commit 09dc2e0

Please sign in to comment.