fix(common): ldml: updates per code review 🙀

- build-key2.ts: comments, and treat the kmxplus data as const
- build-layr.ts: comments, reordering of structs, and treat the kmxplus data as const
- build-list.ts: comments, reordering
- string-list.ts: comments
- kmx-plus.ts: comments
- ldml-keyboard-xml.ts: update definition of LKKey.transform
-

For #7532

common/web/types/src/kmx/kmx-plus-builder/build-key2.ts

@@ -1,6 +1,6 @@
 
 import { constants } from "@keymanapp/ldml-keyboard-constants";
-import { KMXPlusData } from "../kmx-plus.js";
+import { Key2Flick, KMXPlusData, StrsItem } from "../kmx-plus.js";
 import { build_strs_index, BUILDER_STRS } from "./build-strs.js";
 import { build_list_index, BUILDER_LIST } from "./build-list.js";
 import { BUILDER_SECTION } from "./builder-section.js";
@@ -9,30 +9,41 @@ import { BUILDER_SECTION } from "./builder-section.js";
  * key2 section
    ------------------------------------------------------------------ */
 
+/**
+ * This struct is a single <key> in the key2 keybag
+ */
 interface BUILDER_KEY2_KEY {
-  vkey: number;
-  to: number; // str
+  vkey: number; // Scan code for the key
+  to: number; // str or single codepoint
   flags: number;
-  id: number; // str
-  switch: number; // str
-  width: number; // width*10
-  longPress: number; // list
-  longPressDefault: number; // str
-  multiTap: number; // list
-  flicks: number; // index into flicks[]
-};
-
-interface BUILDER_KEY2_FLICK {
-  directions: number; // list
-  flags: number;
-  to: number; // str
+  id: number; // str with original key id
+  _id: string; // original key id, for sorting
+  switch: number; // str with layer of new l
+  width: number; // ceil((width||1)*10), so 12 for width 1.2
+  longPress: number; // list of longPress sequences
+  longPressDefault: number; // str with the default longPress target
+  multiTap: number; // list of multiTap sequences
+  flicks: number; // index into the flicks[] subtable for this flick list
 };
 
+/**
+ * This is a <flicks>, a list of <flick> elements.
+ */
 interface BUILDER_KEY2_FLICKS {
-  count: number;
-  flick: number; // index into flick[]
-  id: number; //str
-  _id: string;
+  count: number; // number of BUILDER_KEY2_FLICK entries in this flick list
+  flick: number; // index into the flick[] subtable of the first flick in the list
+  id: number; // str with the original id of this flicks
+  _id: string; // copy of the flicks id, used for sorting during build
+  _flicks: Key2Flick[]; // temporary copy of Key2Flick object
+};
+
+/**
+ * This is a single <flick> element.
+ */
+interface BUILDER_KEY2_FLICK {
+  directions: number; // list of cardinal/intercardinal directions
+  flags: number; //
+  to: number; // str or single codepoint
 };
 
 /**
@@ -73,18 +84,24 @@ export function build_key2(kmxplus: KMXPlusData, sect_strs: BUILDER_STRS, sect_l
     _offset: 0,
   };
 
-  // flicks first
-  // sort the input, to simplify bookkeeping later
-  kmxplus.key2.flicks.sort((a, b) => a.compareTo(b));
-  // we always need a flicks=0 to mean 'no flicks'
+  // flicks first: the keys will need to index into the flicks table.
+
+  // Note that per the Key2 class and spec, there is always a flicks=0 meaning 'no flicks'
   key2.flicks = kmxplus.key2.flicks.map((flicks) => {
     let result : BUILDER_KEY2_FLICKS = {
       count: flicks.flicks.length,
       flick: key2.flick.length, // index of first flick
       id: build_strs_index(sect_strs, flicks.id),
       _id: flicks.id.value,
+      _flicks: flicks.flicks,
     };
-    flicks.flicks.forEach((flick) => {
+    return result;
+  });
+  // Sort the flicks array by id
+  key2.flicks.sort((a, b) => StrsItem.binaryStringCompare(a._id, b._id));
+  // now, allocate 'flick' entries for each 'flicks'
+  key2.flicks.forEach((flicks) => {
+    flicks._flicks.forEach((flick) => {
       key2.flick.push({
         directions: build_list_index(sect_list, flick.directions),
         flags: flick.flags,
@@ -92,17 +109,16 @@ export function build_key2(kmxplus: KMXPlusData, sect_strs: BUILDER_STRS, sect_l
       });
       key2.flickCount++;
     });
-    return result;
   });
 
-  // now keys
-  kmxplus.key2.keys.sort((a, b) => a.id.compareTo(b.id));
+  // now, keys
   key2.keys = kmxplus.key2.keys.map((key) => {
     let result : BUILDER_KEY2_KEY = {
       vkey: key.vkey,
       to: build_strs_index(sect_strs, key.to),
       flags: key.flags,
       id: build_strs_index(sect_strs, key.id),
+      _id: key.id.value,
       switch: build_strs_index(sect_strs, key.switch),
       width: key.width,
       longPress: build_list_index(sect_list, key.longPress),
@@ -116,6 +132,8 @@ export function build_key2(kmxplus: KMXPlusData, sect_strs: BUILDER_STRS, sect_l
     }
     return result;
   });
+  // sort the keys by id
+  key2.keys.sort((a, b) => StrsItem.binaryStringCompare(a._id, b._id));
 
   let offset = constants.length_key2 +
     (constants.length_key2_key * key2.keyCount) +

common/web/types/src/kmx/kmx-plus-builder/build-layr.ts

@@ -1,132 +1,157 @@
 
 import { constants } from "@keymanapp/ldml-keyboard-constants";
-import { KMXPlusData } from "../kmx-plus.js";
+import { KMXPlusData, LayrEntry, LayrRow, StrsItem } from "../kmx-plus.js";
 import { build_strs_index, BUILDER_STRS } from "./build-strs.js";
 import { BUILDER_LIST } from "./build-list.js";
 import { BUILDER_SECTION } from "./builder-section.js";
 
 /* ------------------------------------------------------------------
- * layr section
-   ------------------------------------------------------------------ */
-
+ * layr section -
+ ------------------------------------------------------------------ */
 
+/**
+ * List of layers, the <layers> element
+ */
 interface BUILDER_LAYR_LIST {
-    flags: number;
-    hardware: number; // str
-    layer: number; // index
-    count: number;
-    minDeviceWidth: number;
+  flags: number;
+  hardware: number; // str - hardware name, see #7986
+  layer: number; // index of first layer in the list, in the
+  count: number; // number of layer entries in the list
+  minDeviceWidth: number; // width in millimeters
+  _layers: LayrEntry[]; // original layer entry, for in-memory only
 };
 
+/**
+ * <layer> element
+ */
 interface BUILDER_LAYR_LAYER {
-    id: number; // str
-    modifier: number; // str
-    row: number; // row index
-    count: number;
+  id: number; // str of layer id
+  _id: string; // original layer id, for sorting
+  modifier: number; // str of modifier string
+  row: number; // row index into row subtable
+  _rows: LayrRow[]; // original rows, for in-memory only
+  count: number; // number of row entries
 };
 
+/**
+ * <row> element
+ */
 interface BUILDER_LAYR_ROW {
-    key: number;
-    count: number;
+  key: number; // index into key subtable
+  count: number; // number of keys
 };
 
+/**
+ * portion of keys attribute of <row>
+ */
 interface BUILDER_LAYR_KEY {
-    key: number;
+  key: number;
 };
 
 /**
  * Builder for the 'keys' section
  */
 export interface BUILDER_LAYR extends BUILDER_SECTION {
-  listCount: number,
-  layerCount: number,
-  rowCount: number,
-  keyCount: number,
-  reserved0: number,
-  reserved1: number,
-  lists: BUILDER_LAYR_LIST[],
-  layers: BUILDER_LAYR_LAYER[],
-  rows: BUILDER_LAYR_ROW[],
-  keys: BUILDER_LAYR_KEY[],
+  listCount: number, // number of entries in lists subtable
+  layerCount: number, // number of entries in layers subtable
+  rowCount: number, // number of entries in rows subtable
+  keyCount: number, // number of entries in keys subtable
+  reserved0: number, // padding
+  reserved1: number, // padding
+  lists: BUILDER_LAYR_LIST[], // subtable of <layers> elements
+  layers: BUILDER_LAYR_LAYER[], // subtable of <layer> elements
+  rows: BUILDER_LAYR_ROW[], // subtable of <row> elements
+  keys: BUILDER_LAYR_KEY[], // subtable of key entries
 };
 
-
 export function build_layr(kmxplus: KMXPlusData, sect_strs: BUILDER_STRS, sect_list: BUILDER_LIST): BUILDER_LAYR {
   if (!kmxplus.layr?.lists) {
-    return null;
+    return null;  // if there aren't any layers at all (which should be an invalid keyboard)
   }
 
   let layr: BUILDER_LAYR = {
-      ident: constants.hex_section_id(constants.section.layr),
-      size: constants.length_layr,
-      _offset: 0,
-      listCount: kmxplus.layr.lists.length,
-      layerCount: 0,
-      rowCount: 0,
-      keyCount: 0,
-      reserved0: 0,
-      reserved1: 0,
-      lists: [],
-      layers: [],
-      rows: [],
-      keys: []
+    ident: constants.hex_section_id(constants.section.layr),
+    size: constants.length_layr,
+    _offset: 0,
+    listCount: kmxplus.layr.lists.length,
+    layerCount: 0, // calculated below
+    rowCount: 0, // calculated below
+    keyCount: 0, // calculated below
+    reserved0: 0,
+    reserved1: 0,
+    lists: [],
+    layers: [],
+    rows: [],
+    keys: []
   };
 
-  // pre-sort layers
-  kmxplus.layr.lists.sort((a, b) => {
+  layr.lists = kmxplus.layr.lists.map((list) => {
+    const blist: BUILDER_LAYR_LIST = {
+      flags: list.flags,
+      hardware: build_strs_index(sect_strs, list.hardware),
+      layer: null, // to be set below
+      _layers: list.layers,
+      count: list.layers.length,
+      minDeviceWidth: list.minDeviceWidth,
+    };
+    return blist;
+  });
+  // now sort the lists
+  layr.lists.sort((a, b) => {
     const aform = a.flags & constants.layr_list_flags_mask_form;
     const bform = b.flags & constants.layr_list_flags_mask_form;
     if (aform < bform) {
-        return -1;
-    } else if(aform > bform) {
-        return 1;
+      return -1;
+    } else if (aform > bform) {
+      return 1;
     }
     if (a.minDeviceWidth < b.minDeviceWidth) {
-        return -1;
-    } else if(a.minDeviceWidth > b.minDeviceWidth) {
-        return 1;
+      return -1;
+    } else if (a.minDeviceWidth > b.minDeviceWidth) {
+      return 1;
     } else {
-        return 0; // same
+      return 0; // same
     }
   });
-
-  layr.lists = kmxplus.layr.lists.map((list) => {
-    const blist : BUILDER_LAYR_LIST = {
-        flags: list.flags,
-        hardware: build_strs_index(sect_strs, list.hardware),
-        layer: layr.layers.length,
-        count: list.layers.length,
-        minDeviceWidth: list.minDeviceWidth,
-    };
-    list.layers.forEach((layer) => {
-        const blayer : BUILDER_LAYR_LAYER = {
-            id: build_strs_index(sect_strs, layer.id),
-            modifier: build_strs_index(sect_strs, layer.modifier),
-            row: layr.rows.length,
-            count: layer.rows.length,
-        };
-        layer.rows.forEach((row) => {
-            const brow : BUILDER_LAYR_ROW = {
-                key: layr.keys.length,
-                count: row.keys.length,
-            };
-            row.keys.forEach((key) => {
-                const bkey : BUILDER_LAYR_KEY = {
-                    key: build_strs_index(sect_strs, key),
-                };
-                layr.keys.push(bkey);
-                layr.keyCount++;
-            });
-            layr.rows.push(brow);
-            layr.rowCount++;
-        });
-        layr.layers.push(blayer);
-        layr.layerCount++;
+  // Now allocate the layers, rows, and keys
+  layr.lists.forEach((list) => {
+    list.layer = layr.layers.length; // index to first layer in list
+    const blayers = list._layers.map((layer) => {
+      const blayer: BUILDER_LAYR_LAYER = {
+        _id: layer.id.value, // original id
+        id: build_strs_index(sect_strs, layer.id),
+        modifier: build_strs_index(sect_strs, layer.modifier),
+        row: null, // row ID, to be filled in
+        _rows: layer.rows, // temporary
+        count: layer.rows.length, // number of rows
+      };
+      return blayer;
+    });
+    // sort the new layers
+    blayers.sort((a, b) => StrsItem.binaryStringCompare(a._id, b._id));
+    blayers.forEach((layer) => {
+      layer.row = layr.rows.length; // index to first row in list
+      layer._rows.forEach((row) => {
+        const brow: BUILDER_LAYR_ROW = {
+          key: layr.keys.length,
+          count: row.keys.length,
+        };
+        row.keys.forEach((key) => {
+          const bkey: BUILDER_LAYR_KEY = {
+            key: build_strs_index(sect_strs, key),
+          };
+          layr.keys.push(bkey);
+        });
+        layr.rows.push(brow);
+      });
+      layr.layers.push(layer);
     });
-
-    return blist;
   });
 
+  layr.layerCount = layr.layers.length;
+  layr.rowCount = layr.rows.length;
+  layr.keyCount = layr.keys.length;
+
   let offset = constants.length_layr +
     (constants.length_layr_list * layr.listCount) +
     (constants.length_layr_entry * layr.layerCount) +

common/web/types/src/kmx/kmx-plus-builder/build-list.ts

@@ -7,23 +7,26 @@ import { BUILDER_SECTION } from "./builder-section.js";
  * list section
    ------------------------------------------------------------------ */
 
+/**
+ * A list entry.
+ */
 interface BUILDER_LIST_LIST {
-  index: number; // index into indices
-  count: number; // number of strings
-  _value: ListItem; // for findability
+  index: number; // index into indices[] subtable
+  count: number; // number of strings in this list
+  _value: ListItem; // for locating the list during finalization
 };
 
 interface BUILDER_LIST_INDEX {
-  str: number; // str
-  _value: string; // for findability?
+  str: number; // str for this string
+  _value: string; // for locating this string during finalization
 };
 
 /**
  * Builder for the 'list' section
  */
 export interface BUILDER_LIST extends BUILDER_SECTION {
-  listCount: number;
-  indexCount: number;
+  listCount: number; // Number of lists total in the subtable
+  indexCount: number; // Total number of indices in the subtable
   lists: BUILDER_LIST_LIST[];
   indices: BUILDER_LIST_INDEX[];
 };
@@ -35,13 +38,13 @@ export function build_list(source_list: List, sect_strs: BUILDER_STRS): BUILDER_
   }
 
   let result: BUILDER_LIST = {
+    ident: constants.hex_section_id(constants.section.list),
+    size: 0,
+    _offset: 0,
     listCount: source_list.lists.length,
     indexCount: 0,
     lists: [],
     indices: [],
-    ident: constants.hex_section_id(constants.section.list),
-    size: 0,
-    _offset: 0
   };
 
   result.lists = source_list.lists.map(array => {

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

@@ -111,6 +111,10 @@ export class Ordr extends Section {
 
 // 'strs'
 
+/**
+ * A string item in memory. This will be replaced with an index
+ * into the string table at finalization.
+ */
 export class StrsItem {
   readonly value: string;
   constructor(value: string) {

common/web/types/src/kmx/string-list.ts

@@ -1,5 +1,9 @@
 import { Strs, StrsItem } from './kmx-plus.js';
 
+/**
+ * A single entry in a ListItem.
+ * Contains a StrsItem as its value.
+ */
 export class ListIndex {
   readonly value: StrsItem; // will become index into Strs table
   constructor(value: StrsItem) {
@@ -14,7 +18,18 @@ export class ListIndex {
   }
 };
 
+/**
+ * A string list in memory. This will be replaced with an index
+ * into the string table at finalization.
+ */
 export class ListItem extends Array<ListIndex> {
+  /**
+   * Construct a new list from an array of strings.
+   * Use List. This is meant to be called by the List.allocString*() functions.
+   * @param strs the Strs section is needed to construct this object.
+   * @param source array of strings
+   * @returns
+   */
   constructor(strs: Strs, source: Array<string>) {
     super();
     if(!source) {

common/web/types/src/ldml-keyboard/ldml-keyboard-xml.ts

@@ -78,10 +78,7 @@ export interface LKKey {
   longPress?: string;
   longPressDefault?: string;
   multiTap?: string;
-  /**
-   * "no" or falsy
-   */
-  transform?: string;
+  transform?: "no";
   width?: number;
 };