Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,23 +1,26 @@
//
// .keyman-touch-layout JSON format
//
// Follows /common/schemas/keyman-touch-layout/keyman-touch-layout.spec.json for
// reading and
// /common/schemas/keyman-touch-layout/keyman-touch-layout.clean.spec.json for
// writing
//
/*
* Keyman is copyright (C) SIL Global. MIT License.
*
* .keyman-touch-layout JSON format definitions
*
* Follows scheams in /common/schemas/keyman-touch-layout/, using
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Follows scheams in /common/schemas/keyman-touch-layout/, using
* Follows schemas in /common/schemas/keyman-touch-layout/, using

* keyman-touch-layout.spec.json for reading and
* keyman-touch-layout.clean.spec.json for writing
*/

/**
* On screen keyboard description consisting of specific layouts for tablet, phone,
* and desktop. Despite its name, this format is used for both touch layouts and
* hardware-style layouts.
*/
export interface TouchLayoutFile {
export type TouchLayoutFile = {
tablet?: TouchLayoutPlatform;
phone?: TouchLayoutPlatform;
desktop?: TouchLayoutPlatform;
};

export type TouchLayoutPlatformName = keyof TouchLayoutFile;

export type TouchLayoutFont = string;
export type TouchLayoutFontSize = string;
export type TouchLayoutDefaultHint = "none"|"dot"|"longpress"|"multitap"|"flick"|"flick-n"|"flick-ne"|"flick-e"|"flick-se"|"flick-s"|"flick-sw"|"flick-w"|"flick-nw";
Expand Down
261 changes: 261 additions & 0 deletions developer/src/kmc-kmn/src/compiler/embed-osk/embed-osk-touch-layout.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,261 @@
/*
* Keyman is copyright (C) SIL Global. MIT License.
*
* Convert .keyman-touch-layout data into KMXPlus data for embedding into .kmx
*/
import { TouchLayout, KMXPlus } from "@keymanapp/common-types";
import { CompilerCallbacks, TouchLayoutFileReader, oskFontMagicToken } from "@keymanapp/developer-utils";
import { KmnCompilerMessages } from "../kmn-compiler-messages.js";

export class EmbedOskTouchLayoutInKmx {

constructor(private callbacks: CompilerCallbacks) {
}

public loadTouchLayoutFile(filename: string): TouchLayout.TouchLayoutFile {
const data = this.callbacks.loadFile(filename);
if(!data) {
this.callbacks.reportMessage(KmnCompilerMessages.Error_FileNotFound({filename}));
return null;
}

const reader = new TouchLayoutFileReader();
try {
const touchLayout = reader.read(data);
reader.validate(touchLayout);
return touchLayout;
} catch(e) {
// TODO-EMBED-OSK-IN-KMX report on e
this.callbacks.reportMessage(KmnCompilerMessages.Error_InvalidTouchLayoutFile({filename}));
return null;
}
}

public transformTouchLayoutToKmxPlus(kmx: KMXPlus.KMXPlusFile, tl: TouchLayout.TouchLayoutFile): void {
// empty the keys into layer bags
// build the layers
// don't forget all the gestures
// what about the VKDictionary?

// transformTouchLayoutPlatform(kmx, tl, 'desktop', tl.desktop); // probably not needed

this.addPlatformFromTouchLayoutPlatform(kmx.kmxplus, 'tablet', tl.tablet, 200); // anything larger than 200mm width
this.addPlatformFromTouchLayoutPlatform(kmx.kmxplus, 'phone', tl.phone, 1); // anything larger than 1mm width
}

private addPlatformFromTouchLayoutPlatform(kmxplus: KMXPlus.KMXPlusData, platformName: TouchLayout.TouchLayoutPlatformName, platform: TouchLayout.TouchLayoutPlatform, deviceWidth: number): void {
if(!platform) {
// The platform may not be used in the touch layout; this is fine
return;
}

const form = new KMXPlus.LayrForm();
form.baseLayout = kmxplus.strs.allocString('en-us'); // TODO-EMBED-OSK-IN-KMX: should this be null for touch?
form.hardware = kmxplus.strs.allocString(KMXPlus.LayrFormHardware.touch);
form.fontFaceName = kmxplus.strs.allocString(oskFontMagicToken);
form.fontSizePct = 100;
form.flags = platform.displayUnderlying ? KMXPlus.LayrFormFlags.showBaseLayout : 0;
form.minDeviceWidth = deviceWidth;
form.layers = [];

// platform.defaultHint; TODO-EMBED-OSK-IN-KMX
// platform.font; [ignore]
// platform.fontsize; [ignore]

for(const layer of platform.layer) {
this.addLayerFromTouchLayoutLayer(kmxplus, form, platformName, layer);
}

kmxplus.layr.forms.push(form);
}

private addLayerFromTouchLayoutLayer(kmxplus: KMXPlus.KMXPlusData, form: KMXPlus.LayrForm, platformName: TouchLayout.TouchLayoutPlatformName, layer: TouchLayout.TouchLayoutLayer): void {
const entry = new KMXPlus.LayrEntry();
entry.id = kmxplus.strs.allocString(layer.id); // TODO-EMBED-OSK-IN-KMX: is this correct?
entry.mod = 0;
for(const row of layer.row) {
this.addRowFromTouchLayoutRow(kmxplus, entry, this.keyIdPrefix(platformName, layer.id), row);
}
form.layers.push(entry);
}

private keyIdPrefix(platformName: TouchLayout.TouchLayoutPlatformName, layerId: string) {
return platformName + '-' + layerId + '-';
}

private addRowFromTouchLayoutRow(kmxplus: KMXPlus.KMXPlusData, entry: KMXPlus.LayrEntry, idPrefix: string, row: TouchLayout.TouchLayoutRow): void {
const er = new KMXPlus.LayrRow();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const er = new KMXPlus.LayrRow();
const layrRow = new KMXPlus.LayrRow();


for(const key of row.key) {
this.addKeyFromTouchLayoutKey(kmxplus, er, idPrefix, key);
}

entry.rows.push(er);
}

private addKeyFromTouchLayoutKey(kmxplus: KMXPlus.DependencySections, er: KMXPlus.LayrRow, idPrefix: string, key: TouchLayout.TouchLayoutKey): void {
const lk = new KMXPlus.KeysKeys();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use a better variable name than lk? keyskeys isn't ideal either but would at least have a connection to the type.

The same "problem" exists in other functions, but they are shorter so it's easier to see the definition of the variable.


lk.id = kmxplus.strs.allocString(idPrefix + key.id);
lk.to = this.getKeyCap(kmxplus, key.id, key.text); // kmxplus.strs.allocString(subKey.text);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

devin.ai identified this problem - I can't tell, but it looks like it might be a real problem. We should probably add a unit test.

DispItem.toId references unprefixed key ID for main keys with special text patterns

When addKeyFromTouchLayoutKey processes a main key whose text matches the *...* pattern (e.g. *Enter*), the getKeyCap method creates a DispItem with toId set to the unprefixed key ID, while the key itself is stored with a prefixed ID.

Root Cause

At embed-osk-touch-layout.ts:99, the key's actual id is set to the prefixed form:

lk.id = kmxplus.strs.allocString(idPrefix + key.id); // e.g. "phone-default-K_ENTER"

But at embed-osk-touch-layout.ts:100, getKeyCap is called with the unprefixed key.id:

lk.to = this.getKeyCap(kmxplus, key.id, key.text); // passes "K_ENTER"

Inside getKeyCap at embed-osk-touch-layout.ts:208, when the text matches *...*, the DispItem.toId is allocated from the unprefixed id:

toId: kmxplus.strs.allocString(id), // allocates "K_ENTER" instead of "phone-default-K_ENTER"

Contrast this with the hint DispItem at embed-osk-touch-layout.ts:141, which correctly uses the prefixed lk.id:

toId: lk.id, // correctly references "phone-default-K_Q"

For subkeys (processed via keyFromSubKey), this is not a problem because subkey IDs are not prefixed — subKey.id matches lk.id. But for main keys, the mismatch means the runtime will fail to look up the display for these keys because the DispItem.toId won't match any key's actual ID.

Impact: Any main key (not subkey) with a special text pattern like *Enter*, *Shift*, etc. will have a broken display mapping in the compiled KMX+ output. The runtime won't find the DispItem for these keys, causing incorrect rendering of special key caps.

Suggested change
lk.to = this.getKeyCap(kmxplus, key.id, key.text); // kmxplus.strs.allocString(subKey.text);
lk.to = this.getKeyCap(kmxplus, idPrefix + key.id, key.text); // kmxplus.strs.allocString(subKey.text);


lk.flags = 0; // TODO-EMBED-OSK-IN-KMX: extend,gap // KMXPlus.KeysKeysFlags.

if(key.flick) {
const flicks = new KMXPlus.KeysFlicks(lk.id);
for(const direction of Object.keys(key.flick) as (keyof TouchLayout.TouchLayoutFlick)[]) {
this.addFlickFromTouchLayoutFlick(kmxplus, flicks, direction, key.flick[direction]);
}
kmxplus.keys.flicks.push(flicks);
lk.flicks = flicks.id.value;
} else {
lk.flicks = null;
}

if(key.sk && key.sk.length) {
const { listItem, defaultId } = this.addKeysFromSubKeys(kmxplus, key.sk);
lk.longPress = listItem;
lk.longPressDefault = defaultId;
} else {
lk.longPress = null;
lk.longPressDefault = kmxplus.strs.allocString('');
}

if(key.multitap && key.multitap.length) {
const { listItem } = this.addKeysFromSubKeys(kmxplus, key.multitap);
lk.multiTap = listItem;
} else {
lk.multiTap = null;
}

lk.switch = kmxplus.strs.allocString(key.nextlayer || '');


lk.width = key.width ?? 100;

if(key.hint) {
const hintDisp: KMXPlus.DispItem = {
to: null, // not used in v19
id: null, // not used in v19
display: kmxplus.strs.allocString(key.hint),
toId: lk.id,
flags:
KMXPlus.DispItemFlags.hintNE |
KMXPlus.DispItemFlags.isId, // TODO-EMBED-OSK-IN-KMX: Do we need to support special key caps here?
};
kmxplus.disp.disps.push(hintDisp);
}

// TODO-EMBED-OSK-IN-KMX key.pad
// TODO-EMBED-OSK-IN-KMX key.sp

kmxplus.keys.keys.push(lk);
er.keys.push(lk.id);
}

private addKeysFromSubKeys(kmxplus: KMXPlus.DependencySections, sk: TouchLayout.TouchLayoutSubKey[]) {
let defaultId: KMXPlus.StrsItem = null;
const ids: string[] = [];
for(const subKey of sk) {
const lk = this.keyFromSubKey(kmxplus, subKey);
kmxplus.keys.keys.push(lk);
if(subKey.default) {
defaultId = lk.id;
}
ids.push(lk.id.value);
}

if(defaultId === null) {
defaultId = kmxplus.strs.allocString(ids.length ? ids[0] : '');
}

const listItem = kmxplus.list.allocList(ids, {}, kmxplus);
return { listItem, defaultId };
}

private addFlickFromTouchLayoutFlick(kmxplus: KMXPlus.DependencySections, flicks: KMXPlus.KeysFlicks, direction: string, subKey: TouchLayout.TouchLayoutSubKey): void {
const flick = new KMXPlus.KeysFlick();
flick.directions = kmxplus.list.allocList([direction], {}, kmxplus);
flick.keyId = kmxplus.strs.allocString(subKey.id);

const lk = this.keyFromSubKey(kmxplus, subKey);
kmxplus.keys.keys.push(lk);
flicks.flicks.push(flick);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another section that devin.ai flagged:

Subkey IDs are not deduplicated, risking duplicate KeysKeys entries

The keyFromSubKey method at embed-osk-touch-layout.ts:186-198 creates a new KeysKeys and pushes it unconditionally at lines 161 and 182. If the same subkey ID (e.g. K_1) is referenced by both a longpress (sk) and a flick on the same or different keys, duplicate KeysKeys entries with the same id will be added to kmxplus.keys.keys. Compare this with the LDML compiler in developer/src/kmc-ldml/src/compiler/keys.ts:314, which tracks used keys in a Set<string> to avoid duplicates. Depending on how the KMX+ builder serializes and the runtime looks up keys, this may cause issues or merely waste space. Worth investigating whether deduplication is needed here.

private keyFromSubKey(kmxplus: KMXPlus.DependencySections, subKey: TouchLayout.TouchLayoutSubKey) {
const lk = new KMXPlus.KeysKeys();
lk.id = kmxplus.strs.allocString(subKey.id);
lk.flags = 0;
lk.flicks = null;
lk.longPress = null;
lk.longPressDefault = kmxplus.strs.allocString('');
lk.multiTap = null;
lk.switch = kmxplus.strs.allocString(subKey.nextlayer || '');
// TODO: Disp
lk.to = this.getKeyCap(kmxplus, subKey.id, subKey.text); // kmxplus.strs.allocString(subKey.text);
lk.width = 100;
return lk;
}

private getKeyCap(kmxplus: KMXPlus.DependencySections, id: string, text: string) {
text = text ?? '';
if(text.match(/^\*(.+)\*$/)) {
const disp: KMXPlus.DispItem = {
to: null, // not used in v19
id: null, // not used in v19
display: kmxplus.strs.allocString(''),
toId: kmxplus.strs.allocString(id),
flags:
KMXPlus.DispItemFlags.hintPrimary |
KMXPlus.DispItemFlags.isId |
KMXPlus.DispItemFlags.keyCap123, //TODO-EMBED-OSK-IN-KMX
}
kmxplus.disp.disps.push(disp);
return kmxplus.strs.allocString('');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this right that we return an empty string for *key* ?

} else if(text.trim() === '' && id.startsWith('U_')) {
// if key cap == U_xxxx[_yyyy], then we generate key cap from that
return kmxplus.strs.allocString(this.unicodeKeyIdToString(id));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If id has an invalid U_ id this will result in an empty key cap and not produce a warning. I don't know if this can happen or if we know here that id always contains a valid U_ sequence. (Flagged by devin.ai)

} else {
return kmxplus.strs.allocString(text);
}
}

private unicodeKeyIdToString(id: string) {
// Test for fall back to U_xxxxxx key id
// For this first test, we ignore the keyCode and use the keyName
if(!id || id.substring(0, 2) != 'U_') {
return null;
}

let result = '';
const codePoints = id.substring(2).split('_');
for(const codePoint of codePoints) {
const codePointValue = parseInt(codePoint, 16);
if (isNaN(codePointValue) || (0x0 <= codePointValue && codePointValue <= 0x1F) || (0x80 <= codePointValue && codePointValue <= 0x9F)) {
// Code points [U_0000 - U_001F] and [U_0080 - U_009F] refer to Unicode C0 and C1 control codes.
// Check the codePoint number and do not allow output of these codes via U_xxxxxx shortcuts.
// Also handles invalid identifiers (e.g. `U_ghij`) for which parseInt returns NaN

// We'll still attempt to add valid chars
continue;
} else {
result += String.fromCodePoint(codePointValue);
}
}
return result ? result : null;
}

public unitTestEndpoints = {
// public loadTouchLayoutFile: this.loadTouchLayoutFile.bind(this),
// public transformTouchLayoutToKmxPlus: this.transformTouchLayoutToKmxPlus.bind(this),
addPlatformFromTouchLayoutPlatform: this.addPlatformFromTouchLayoutPlatform.bind(this),
addLayerFromTouchLayoutLayer: this.addLayerFromTouchLayoutLayer.bind(this),
addRowFromTouchLayoutRow: this.addRowFromTouchLayoutRow.bind(this),
addKeyFromTouchLayoutKey: this.addKeyFromTouchLayoutKey.bind(this),
addKeysFromSubKeys: this.addKeysFromSubKeys.bind(this),
addFlickFromTouchLayoutFlick: this.addFlickFromTouchLayoutFlick.bind(this),
keyFromSubKey: this.keyFromSubKey.bind(this),
}

}
14 changes: 11 additions & 3 deletions developer/src/kmc-kmn/src/compiler/embed-osk/embed-osk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,7 @@ import { KMXPlusVersion } from "@keymanapp/ldml-keyboard-constants";
import { KmnCompilerOptions } from "../compiler.js";
import { PuaMap, loadKvkFile } from "../osk.js";
import { EmbedOskKvkInKmx } from "./embed-osk-kvk.js";

// import { EmbedOskTouchLayoutInKmx } from "./embed-osk-touch-layout.js";
import { EmbedOskTouchLayoutInKmx } from "./embed-osk-touch-layout.js";

export class EmbedOskInKmx {
constructor(
Expand All @@ -31,7 +30,7 @@ export class EmbedOskInKmx {
kmx.kmxplus.elem = new KMXPlus.Elem(kmx.kmxplus);
kmx.kmxplus.disp = new KMXPlus.Disp();
kmx.kmxplus.keys = new KMXPlus.Keys(strs);
// list?
kmx.kmxplus.list = new KMXPlus.List(strs);
kmx.kmxplus.loca = new KMXPlus.Loca();
kmx.kmxplus.meta = new KMXPlus.Meta();
kmx.kmxplus.meta.author = strs.allocString();
Expand Down Expand Up @@ -68,6 +67,15 @@ export class EmbedOskInKmx {
embedKvk.transformVisualKeyboardToKmxPlus(kmxPlus, vk);
}

if(touchLayoutFilename) {
const embedTouchLayout = new EmbedOskTouchLayoutInKmx(this.callbacks);
const tl = embedTouchLayout.loadTouchLayoutFile(touchLayoutFilename);
if(!tl) {
// error will have been reported by loadTouchLayoutFile
return null;
}
embedTouchLayout.transformTouchLayoutToKmxPlus(kmxPlus, tl);
}

// TODO-EMBED-OSK-IN-KMX: touch layout to ldml
// TODO-EMBED-OSK-IN-KMX: display map remapping
Expand Down
Loading