fix(web): fix keyboard-processing bugs uncovered by stricter TS settings 🔩 #11603
Conversation
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
| // The function baked into keyboards by the current Web compiler creates an | ||
| // array of single-char strings for BK. Refer to | ||
| // developer/src/kmc-kmn/src/kmw-compiler/visual-keyboard-compiler.ts. | ||
| static readonly DEFAULT_RAW_SPEC = {'F':'Tahoma', 'BK': Layouts.dfltText.split('')} as const; |
There was a problem hiding this comment.
From an old keyboard (viewable here):
BK: new Array("`","1","2","3","4","5","6","7","8","9","0","-","=","","","","q","w","e","r","t","y","u","i","o","p","[","]","\\","","","","a","s","d","f","g","h","j","k","l",";","'","","","","","","\\","z","x","c","v","b","n","m",",",".","/","","","","",""," ","~","!","@","#","$","%","^","&","*","(",")","_","+","","","","Q","W","E","R","T","Y","U","I","O","P","{","}","|","","","","A","S","D","F","G","H","J","K","L",":","\"","","","","","","|","Z","X","C","V","B","N","M","<",">","?","","","","",""," ","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","","",""," ")From the output generated by modern compilers when run live (via khmer_angkor):
(130) ['«', '១', '២', '៣', '៤', '៥', '៦', '៧', '៨', '៩', '០', 'ឥ', 'ឲ', '', '', '', 'ឆ', '', '', 'រ', 'ត', 'យ', '', '', '', 'ផ', '', 'ឪ', 'ឮ', '', '', '', '', 'ស', 'ដ', 'ថ', 'ង', 'ហ', '', 'ក', 'ល', '', '', '', '', '', '', '', '', 'ឋ', 'ខ', 'ច', 'វ', 'ប', 'ន', 'ម', '', '។', '', '', '', '', '', '', '', '»', '!', 'ៗ', '"', '៛', '%', '', '', '', '(', ')', '', '=', '', '', '', 'ឈ', '', '', 'ឬ', 'ទ', '', '', '', '', 'ភ', '', 'ឧ', 'ឭ', '', '', '', '', '', 'ឌ', …]
It is technically OK in JavaScript to index a string as if it were an array, which is why things have been working. TS strictness settings don't like it, though.
| @@ -86,7 +86,7 @@ const Codes = { | |||
| [')!@#$%^&*(',':+<_>?~', '{|}"'] | |||
| ], | |||
|
|
|||
| isKnownOSKModifierKey(keyID: string): boolean { | |||
| isKeyNotCorrected(keyID: string): boolean { | |||
There was a problem hiding this comment.
I changed the name for two reasons:
- This highlights the limited uses of the method.
- Given that the only use is to determine what keys we don't fat-finger correct, and that K_TAB + K_TABFWD are being included... well, K_TAB isn't a modifier key, so the name isn't quite right.
| if(Codes.keyCodes[keyID] >= 50000) { // A few are used by `sil_euro_latin`. | ||
| return true; // is a 'K_' key defined for layer shifting or 'control' use. | ||
| } | ||
| // Refer to text/codes.ts - these are Keyman-custom "keycodes" used for | ||
| // layer shifting keys. To be safe, we currently let K_TABBACK and | ||
| // K_TABFWD through, though we might be able to drop them too. | ||
| const code = Codes[keyID]; | ||
| if(code > 50000 && code < 50011) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Once I stopped and looked at this... "wait, why do we have two near-identical checks"? And... "when did this happen?"
The answer to the latter question: it happened in #7242. I don't know what I was thinking then. It's clear we should only have one of the two.
Granted, settling on the one that I did kind of implies we should also ignore K_TAB for consistency. (I haven't made this change yet, though.)
| @@ -938,6 +938,7 @@ export default class KeyboardInterface extends KeyboardHarness { | |||
| if(systemId == SystemStoreIDs.TSS_LAYER && this.activeDevice.touchable) { | |||
| // Denote the changed store as part of the matched rule's behavior. | |||
| this.ruleBehavior.setStore[systemId] = strValue; | |||
| return true; | |||
There was a problem hiding this comment.
Note line 929, which says we should have been doing this all along.
There was a problem hiding this comment.
I think this should be three separate pull requests -- they are three unrelated bugs.
The three bug fix titles could have been:
- fix(web):
[keyboard].KV.BKshould always be an array but default fallback was providing a string - refactor(web): Remove dead code in
isKnownOSKModifierKeyand rename it toisKeyNotCorrected - fix(web): always return true or false for
KSETS(setStore)
In my analysis, none of the three changes have any end-user impact.
- as you noted,
[keyboard].KV.BKas string or array had basically the same outcome, but would be a trap for future maintainers - there is a question you ask regarding
isKeyNotCorrectedwhich probably could turn into its own discussion:Granted, settling on the one that I did kind of implies we should also ignore K_TAB for consistency. (I haven't made this change yet, though.)
- keyboards do not use the return value from
KSETS:
keyman/developer/src/kmc-kmn/src/kmw-compiler/javascript-strings.ts
Lines 883 to 884 in ab4fcef
Generally, including Not in a function name is a code smell. A better name may be isFrameKey?
Apart from that, LGTM. I guess I will approve this PR this time, but let's try and get into the habit of fixing one bug at a time in a PR.
Co-authored-by: Marc Durdin <[email protected]>
jahorton
changed the title
|
Changes in this pull request will be available for download in Keyman version 18.0.47-alpha |
Originally part of #11424, these changes address bugs that were uncovered thanks to stricter TS settings. There are three in total, but they're in the same region of code and are all quite small, so I lumped 'em together.
@keymanapp-test-bot skip