fix(developer): produce correct index parameter value for context in kmw compiler #13003
Conversation
…kmw compiler The KMW compiler would produce incorrect indices for the corresponding character in the context after adjusting for the presence of `nul` and `if` statements at the start of the context string, when using the `context` and `context(n)` statements in the output. This fix addresses the offset calculation for those scenarios and adds a unit test to verify that the new offsets are correct. Fixes: #12980
mcdurdin
requested review from
darcywong00,
SabineSIL,
markcsinclair and
srl295
as code owners
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
| @@ -759,15 +759,14 @@ export function JavaScript_OutputString(fk: KMX.KEYBOARD, FTabStops: string, fkp | |||
| case KMX.KMXFile.CODE_CONTEXT: | |||
| if (x > 0 || len == -1) { | |||
| let xContext = 0; | |||
| let n = 0; | |||
| let n = 1; | |||
There was a problem hiding this comment.
Can we document some of the magic:
- why
n = 1 - why l.781 adjusts
rec.ContextEx.Index + 1below?
There was a problem hiding this comment.
This was an off-by-one error in the use of the ContextChar function where there were prefix tests in the context (if, nul, etc). Adjusting n to 1 here resolves the issue. Similar resolution for rec.ContextEx.Index on L.781.
| if(&platform = 'web') notany(c) + 'c' > context 'C' | ||
| if(&platform = 'web') notany(d) + 'd' > context(2) 'D' | ||
| nul any(e) + 'e' > context 'E' | ||
| nul any(f) + 'f' > context(2) 'F' |
There was a problem hiding this comment.
Any value in also testing the combo
+ nul notany(g) + 'g' > ...?
There was a problem hiding this comment.
Fun detail: notany(g) can match nul. I'm not 100% on if that holds true here, with nul notany(g), but the Web-side implementation will happily match such a rule in that manner.
There was a problem hiding this comment.
... and upon re-inspection of the tests that led me to think this, I may have interpreted what I saw wrongly. It has been a while since I worked on that set of unit tests; my apologies for the confusion.
There was a problem hiding this comment.
Any value in also testing the combo
+ nul notany(g) + 'g' > ...?
I don't think it adds any additional coverage but am happy to add it.
The KMW compiler would produce incorrect indices for the corresponding character in the context after adjusting for the presence of
nulandifstatements at the start of the context string, when using thecontextandcontext(n)statements in the output.This fix addresses the offset calculation for those scenarios and adds a unit test to verify that the new offsets are correct.
I have audited the keyboards repository, and only one keyboard in the repository is impacted: sil_greek_polytonic. Once v18 is released, we should publish an updated version of that keyboard, rebuilt with the 18.0 compiler to resolve this issue.
Another keyboard, keymangr_all, is not in the keyboards repository but is also impacted by this issue. This keyboard currently has patches to workaround the issue, which would need to be removed if rebuilt with the new compiler. I will communicate this information to the author of the keyboard.
Fixes: #12980
Fixes: #12985
@keymanapp-test-bot skip