chore(developer): address review comments

Co-authored-by: Eberhard Beilharz <ermshiperete@users.noreply.github.com>

common/web/types/src/kmx/kmx-plus/kmx-plus-file.ts

@@ -9,6 +9,7 @@ import * as KMX from '../kmx.js';
 import * as r from 'restructure';
 import KMXFile = KMX.KMXFile;
 import { KMXPlusVersion } from '@keymanapp/ldml-keyboard-constants';
+import { ModifierKeyConstant } from '../../consts/modifier-key-constants.js';
 
 /* interfaces that match the COMP_PLUS_* structs -- for type safety */
 
@@ -116,7 +117,7 @@ export interface ICOMP_PLUS_KEYS_KEY {
 
 export interface ICOMP_PLUS_KEYS_KMAP {
   vkey: number;
-  mod: number;
+  mod: ModifierKeyConstant;
   key: ITABLE_REF; // keys.keys index
 };
 
@@ -134,7 +135,7 @@ export interface ICOMP_PLUS_KEYS {
 
 export interface ICOMP_PLUS_LAYR_ENTRY {
   id: ISTR_REF;    // str
-  mod: number;     // bitfield
+  mod: ModifierKeyConstant;     // bitfield
   row: ITABLE_REF; // layr.rows index
   count: number;
 };

common/web/types/src/kmx/kmx-plus/kmx-plus.ts

@@ -10,6 +10,7 @@ import * as util from '../../util/util.js';
 import { UnicodeSetParser, UnicodeSet } from '../../ldml-keyboard/unicodeset-parser-api.js';
 import { VariableParser } from '../../ldml-keyboard/pattern-parser.js';
 import { MarkerParser } from '../../ldml-keyboard/pattern-parser.js';
+import { ModifierKeyConstant } from '../../consts/modifier-key-constants.js';
 
 import isOneChar = util.isOneChar;
 import toOneChar = util.toOneChar;
@@ -620,7 +621,7 @@ export class LayrForm {
  */
  export class LayrEntry {
   id: StrsItem;
-  mod: number;
+  mod: ModifierKeyConstant;
   rows: LayrRow[] = [];
 };
 
@@ -663,7 +664,7 @@ export class KeysKeys {
 
 export class KeysKmap {
   vkey: number;
-  mod: number;
+  mod: ModifierKeyConstant;
   key: string; // for in-memory only
 };
 

developer/src/kmc-kmn/src/compiler/embed-osk/embed-osk-kvk.ts

@@ -145,12 +145,12 @@ export class EmbedOskKvkInKmx {
       layerBags.get(mod).set(key.vkey, keykey);
       keys.keys.push(keykey);
 
-      this.addKmap(keys, keykey, key.vkey, key.shift);
+      this.addKmap(keys, keykey, key.vkey, mod);
     }
     return layerBags;
   }
 
-  private addKmap(keys: KMXPlus.Keys, newKey: KMXPlus.KeysKeys, vkey: number, mod: number) {
+  private addKmap(keys: KMXPlus.Keys, newKey: KMXPlus.KeysKeys, vkey: number, mod: ModifierKeyConstant) {
     const newKmap = new KMXPlus.KeysKmap();
     newKmap.key = newKey.id.value;
     newKmap.mod = mod;

developer/src/kmc-kmn/src/compiler/embed-osk/embed-osk-touch-layout.ts

@@ -100,11 +100,13 @@ export class EmbedOskTouchLayoutInKmx {
    * developer/src/tike/xml/layoutbuilder/constants.js)
    * @returns
    */
-  private generateUniqueKeyId(keys: KMXPlus.Keys, platform: string, layer: string, id: string, keyModifier: string) {
+  private generateUniqueKeyId(keys: KMXPlus.Keys, platform: string, layer: string, id: string, keyModifier: string): string {
     // key id in KeymanWeb is `layer-id+override`
     // we need to pass in this data so that rules can still be applied
     // `+override` is the modifier for the key to be applied in the kmap -- not the layer
-    const baseId = platform + '~' + layer + '-' + id + (keyModifier !== '' ? '+'+keyModifier : '');
+    const baseId = keyModifier === '' ?
+      `${platform}~${layer}-${id}` :
+      `${platform}~${layer}-${id}+${keyModifier}`;
 
     const key = keys.keys.find(item => item.id.value == baseId);
     if(!key) {
@@ -194,14 +196,14 @@ export class EmbedOskTouchLayoutInKmx {
     kmxplus.keys.kmap.push(newKmap);
   }
 
-  private keyCodeToVkey(layerId: string, id: string, text: string) {
-    id = id.toUpperCase();
+  private keyCodeToVkey(layerId: string, keyCode: string, text: string) {
+    keyCode = keyCode.toUpperCase();
 
     // K_ --> always look up from standard map, give warning if not found, return 0
-    if(id.startsWith('K_')) {
-      const result: number = (<any>USVirtualKeyCodes)[id];
+    if(keyCode.startsWith('K_')) {
+      const result: number = (<any>USVirtualKeyCodes)[keyCode];
       if(typeof result !== 'number') {
-        this.callbacks.reportMessage(KmnCompilerMessages.Warn_TouchLayoutInvalidKeyId({layerId, id}));
+        this.callbacks.reportMessage(KmnCompilerMessages.Warn_TouchLayoutInvalidKeyId({layerId, id: keyCode}));
         return 0;
       }
       if(result > 255) {
@@ -214,12 +216,15 @@ export class EmbedOskTouchLayoutInKmx {
     }
 
     // T_ --> look up from VKDictionary, give warning if not found, return 0
-    if(id.startsWith('T_')) {
-      const index = this.vkDictionary.findIndex(key => key.toUpperCase() === id);
+    if(keyCode.startsWith('T_')) {
+      const index = this.vkDictionary.findIndex(key => key.toUpperCase() === keyCode);
       if(index < 0) {
         if(text != '') {
           // Only warn if the key cap isn't blank
-          this.callbacks.reportMessage(KmnCompilerMessages.Warn_TouchLayoutCustomKeyNotDefined({keyId: id, platformName: 'TODO-EMBED-OSK-IN-KMX', layerId, address: {keyIndex:0, rowIndex:0}}));
+          // This is a bit heuristic: a blank key is likely to be unused
+          // TODO-EMBED-OSK-IN-KMX: this needs to be cleaned up to match validate-layout-file:158
+          // "Check that each custom key code has at least *a* rule associated with it"
+          this.callbacks.reportMessage(KmnCompilerMessages.Warn_TouchLayoutCustomKeyNotDefined({keyId: keyCode, platformName: 'TODO-EMBED-OSK-IN-KMX', layerId, address: {keyIndex:0, rowIndex:0}}));
         }
         return 0;
       }
@@ -228,8 +233,8 @@ export class EmbedOskTouchLayoutInKmx {
     }
 
     // U_ --> look up from VKDictionary, return 0 if not found, will map at runtime (no warning)
-    if(id.startsWith('U_')) {
-      const index = this.vkDictionary.findIndex(key => key.toUpperCase() === id);
+    if(keyCode.startsWith('U_')) {
+      const index = this.vkDictionary.findIndex(key => key.toUpperCase() === keyCode);
       if(index < 0) {
         // will be mapped at runtime
         return 0;

developer/src/kmc-kmn/src/compiler/kmn-compiler-messages.ts

@@ -290,11 +290,14 @@ export class KmnCompilerMessages {
     this.WARN_TouchLayoutKeyIdUsedMoreThanOnceInALayer,
     `The On-Screen Keyboard key id '${def(o.id)}' is used more than once on layer '${def(o.layer)}'. The resolved id for the duplicate is '${def(o.resolvedId)}'`, `
     Key ids should not be re-used on the same layer, because each key should be
-    unique for a given layer. If you do want to use the same key rule for the same
-    layer, for example if you have a longpress and a flick with the same output,
-    you may need to add a rule to duplicate the output. The compiler will
-    de-duplicate the identifier but the unique value it generates may change, so
-    it cannot be safely used for example with CSS rules to identify the key.
+    unique for a given layer. The compiler will de-duplicate the identifier but
+    the unique value it generates may change, so it cannot be safely used for
+    example with CSS rules to identify the key.
+
+    If you do want to use the same key rule for the same layer, for example if
+    you have a longpress and a flick with the same output, it is safer to use
+    two different ids and repeat the rule, or use \`store\` and \`any\` to
+    match both key ids in the one rule.
     https://help.keyman.com/developer/current-version/reference/file-types/keyman-touch-layout#toc-key-code
   `);
 

developer/src/kmc-kmn/test/embed-osk-kvk.tests.ts

@@ -116,6 +116,7 @@ describe('Compiler OSK Embedding', function() {
 
         assert.isArray(keys.kmap);
         assert.lengthOf(keys.kmap, keys.keys.length);
+        // TODO-EMBED-OSK-IN-KMX: add tests for keys.kmap
 
         // bag will be a map of maps; this test has three layers with an unmodified base layer K_A, a shift+K_B, and Ctrl+Shift+C
         assert.isNotNull(bag);

developer/src/kmc-kmn/test/embed-osk-touch-layout.tests.ts

@@ -74,6 +74,9 @@ describe('Compiler OSK Embedding', function() {
 
   describe('EmbedOskTouchLayoutInKmx', function() {
 
+    // TODO-EMBED-OSK-IN-KMX: add a test to exercise de-dedup of key ids
+    // https://github.com/keymanapp/keyman/pull/15821#discussion_r3056712353
+
     describe('EmbedOskTouchLayoutInKmx.keyFromTouchLayoutKey', function() {
       it('should convert a touch layout key into a KMX+ key', async function() {
         const kmxRow = new KMXPlus.LayrRow();

docs/file-formats/kmx-plus-file-format.md

@@ -472,8 +472,8 @@ Elements are ordered by the `flicks.keyId`, and secondarily by the directions li
 
 #### `key2.kmap` key map subtable
 
-This table (formerly the `keys` section) The keys are sorted in ascending order
-based on the `vkey`, `mod` fields. Note that there may be multiple `keys`
+In this table (formerly the `keys` section), the keys are sorted in ascending
+order based on the `vkey`, `mod` fields. Note that there may be multiple `keys`
 that resolve to a single `vkey`+`mod` pair.
 
 For each key: