From f57c3d56c8a8a677aee91bcaacadc6d1d36c2764 Mon Sep 17 00:00:00 2001 From: Marc Durdin Date: Thu, 16 Apr 2026 09:51:18 +0200 Subject: [PATCH] chore(developer): address review comments Co-authored-by: Eberhard Beilharz --- .../types/src/kmx/kmx-plus/kmx-plus-file.ts | 5 ++-- common/web/types/src/kmx/kmx-plus/kmx-plus.ts | 5 ++-- .../src/compiler/embed-osk/embed-osk-kvk.ts | 4 +-- .../embed-osk/embed-osk-touch-layout.ts | 29 +++++++++++-------- .../src/compiler/kmn-compiler-messages.ts | 13 +++++---- .../src/kmc-kmn/test/embed-osk-kvk.tests.ts | 1 + .../test/embed-osk-touch-layout.tests.ts | 3 ++ docs/file-formats/kmx-plus-file-format.md | 4 +-- 8 files changed, 39 insertions(+), 25 deletions(-) diff --git a/common/web/types/src/kmx/kmx-plus/kmx-plus-file.ts b/common/web/types/src/kmx/kmx-plus/kmx-plus-file.ts index 19601099d1..0ccc14fb4f 100644 --- a/common/web/types/src/kmx/kmx-plus/kmx-plus-file.ts +++ b/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; }; diff --git a/common/web/types/src/kmx/kmx-plus/kmx-plus.ts b/common/web/types/src/kmx/kmx-plus/kmx-plus.ts index fb3fccd788..feb0992108 100644 --- a/common/web/types/src/kmx/kmx-plus/kmx-plus.ts +++ b/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 }; diff --git a/developer/src/kmc-kmn/src/compiler/embed-osk/embed-osk-kvk.ts b/developer/src/kmc-kmn/src/compiler/embed-osk/embed-osk-kvk.ts index f1aa382555..f3a1b29218 100644 --- a/developer/src/kmc-kmn/src/compiler/embed-osk/embed-osk-kvk.ts +++ b/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; diff --git a/developer/src/kmc-kmn/src/compiler/embed-osk/embed-osk-touch-layout.ts b/developer/src/kmc-kmn/src/compiler/embed-osk/embed-osk-touch-layout.ts index 5718dc0977..f9bd702ccf 100644 --- a/developer/src/kmc-kmn/src/compiler/embed-osk/embed-osk-touch-layout.ts +++ b/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 = (USVirtualKeyCodes)[id]; + if(keyCode.startsWith('K_')) { + const result: number = (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; diff --git a/developer/src/kmc-kmn/src/compiler/kmn-compiler-messages.ts b/developer/src/kmc-kmn/src/compiler/kmn-compiler-messages.ts index 6f46237b25..6cc6bf1a02 100644 --- a/developer/src/kmc-kmn/src/compiler/kmn-compiler-messages.ts +++ b/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 `); diff --git a/developer/src/kmc-kmn/test/embed-osk-kvk.tests.ts b/developer/src/kmc-kmn/test/embed-osk-kvk.tests.ts index e087db0f31..563dbff515 100644 --- a/developer/src/kmc-kmn/test/embed-osk-kvk.tests.ts +++ b/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); diff --git a/developer/src/kmc-kmn/test/embed-osk-touch-layout.tests.ts b/developer/src/kmc-kmn/test/embed-osk-touch-layout.tests.ts index 0fffd13dcc..2c4c4fd27f 100644 --- a/developer/src/kmc-kmn/test/embed-osk-touch-layout.tests.ts +++ b/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(); diff --git a/docs/file-formats/kmx-plus-file-format.md b/docs/file-formats/kmx-plus-file-format.md index 6324ddefed..83707abfbd 100644 --- a/docs/file-formats/kmx-plus-file-format.md +++ b/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: