feat(developer): kmc-convert: new conversion .keylayout->kmn 😎#12564
Open
SabineSIL wants to merge 286 commits intoepic/kmc-convertfrom
Open
feat(developer): kmc-convert: new conversion .keylayout->kmn 😎#12564SabineSIL wants to merge 286 commits intoepic/kmc-convertfrom
SabineSIL wants to merge 286 commits intoepic/kmc-convertfrom
Conversation
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
keymanapp-test-bot
Bot
changed the title
…napp/keyman into feat/developer/kmc-convert # Conflicts: # developer/src/kmc-convert/test/keylayout-to-kmn-converter.tests.ts
adapt writing use new keylayout.xsd without "@__" change name MAX_KEY_COUNT, USE_KEY_COUNT remove init()
…CharacterOrUnicode()
SabineSIL
commented
Apr 9, 2026
Contributor
Author
There was a problem hiding this comment.
All comments have been addressed:
- uses KeymanXMLReader now
- removeWhitespace() is not neccessary any more when using KeymanXMLReader
- KeylayoutToKmnConverter uses constructor/no init()
- uses MAX_KEY_IDENTIFIER and dismisses USED_KEY_IDENTIFIER
- unknown modifiers and other messages:
…boxXmlArray, filter function, remove USED_KEY_IDENTIFIER and function to calculate it
mcdurdin
reviewed
Apr 22, 2026
Member
mcdurdin
left a comment
mcdurdin
left a comment
There was a problem hiding this comment.
I'm going to stop here on this code review -- I've looked mainly at the source file format for keylayout and compared to the Apple docs at TN2056.
Once you apply the Keylayout.KeylayoutXMLSourceFile type to the convert() jsonObj parameter, I suspect that a number of other compile errors will emerge -- these should help to tighten up the code.
Comment on lines
+43
to
+65
| boxXmlArray(source.layouts, 'layout'); | ||
| boxXmlArray(source?.modifierMap, 'keyMapSelect'); | ||
|
|
||
| for (const keyMapSelect of source?.modifierMap?.keyMapSelect) { | ||
| boxXmlArray(keyMapSelect, 'modifier'); | ||
| } | ||
|
|
||
| for (const keyMapSet of source?.keyMapSet) { | ||
| boxXmlArray(keyMapSet, 'keyMap'); | ||
| for (const keyMap of keyMapSet.keyMap) { | ||
| boxXmlArray(keyMap, 'key'); | ||
| } | ||
| } | ||
|
|
||
| boxXmlArray(source?.actions, 'action'); | ||
| for (const action of source?.actions?.action) { | ||
| boxXmlArray(action, 'when'); | ||
| } | ||
|
|
||
| boxXmlArray(source.terminators, 'when'); | ||
| for (const action of source?.actions?.action) { | ||
| boxXmlArray(action, 'when'); | ||
| } |
Member
There was a problem hiding this comment.
I think the boxing is not quite right. Any element that can be repeated needs to be boxed, in case there are only 1 or 0 of them. Once boxed, the property will become an array, so it's safe to reference.
This matches more closely to the spec, I think:
Suggested change
| boxXmlArray(source.layouts, 'layout'); | |
| boxXmlArray(source?.modifierMap, 'keyMapSelect'); | |
| for (const keyMapSelect of source?.modifierMap?.keyMapSelect) { | |
| boxXmlArray(keyMapSelect, 'modifier'); | |
| } | |
| for (const keyMapSet of source?.keyMapSet) { | |
| boxXmlArray(keyMapSet, 'keyMap'); | |
| for (const keyMap of keyMapSet.keyMap) { | |
| boxXmlArray(keyMap, 'key'); | |
| } | |
| } | |
| boxXmlArray(source?.actions, 'action'); | |
| for (const action of source?.actions?.action) { | |
| boxXmlArray(action, 'when'); | |
| } | |
| boxXmlArray(source.terminators, 'when'); | |
| for (const action of source?.actions?.action) { | |
| boxXmlArray(action, 'when'); | |
| } | |
| boxXmlArray(source, 'modifierMap'); | |
| boxXmlArray(source, 'keyMapSet'); | |
| if(source.layouts) { | |
| boxXmlArray(source.layouts, 'layout'); | |
| } | |
| if(source.modifierMap) { | |
| for(const modifierMap of source.modifierMap) { | |
| boxXmlArray(modifierMap, 'keyMapSelect'); | |
| if(modifierMap.keyMapSelect) { | |
| for (const keyMapSelect of source.modifierMap.keyMapSelect) { | |
| boxXmlArray(keyMapSelect, 'modifier'); | |
| } | |
| } | |
| } | |
| } | |
| for (const keyMapSet of source.keyMapSet) { | |
| boxXmlArray(keyMapSet, 'keyMap'); | |
| for (const keyMap of keyMapSet.keyMap) { | |
| boxXmlArray(keyMap, 'key'); | |
| } | |
| } | |
| if(source.actions) { | |
| boxXmlArray(source.actions, 'action'); | |
| for (const action of source.actions.action) { | |
| boxXmlArray(action, 'when'); | |
| } | |
| } | |
| boxXmlArray(source?.terminators, 'when'); |
| try { | ||
| const data = new TextDecoder().decode(source); | ||
| const jsonObj = new KeymanXMLReader('keylayout').parse(data) as Keylayout.KeylayoutXMLSourceFile; | ||
| this.boxArray(jsonObj.keyboard); // jsonObj now contains arrays; no single fields |
Member
There was a problem hiding this comment.
Suggested change
| this.boxArray(jsonObj.keyboard); // jsonObj now contains arrays; no single fields | |
| if(!jsonObj?.keyboard) { | |
| // TODO: report an error | |
| return null; | |
| } | |
| this.boxArray(jsonObj.keyboard); // jsonObj now contains arrays; no single fields |
| * the 5 main elements. | ||
| */ | ||
| //layoutsMM: KL_Layouts[]; | ||
| layouts?: KL_Layouts[]; |
Member
There was a problem hiding this comment.
This is created as an array here but the array appears actually to be the child element layouts.layout? It seems it is unused so slipping through but should be corrected.
Also, per TN2056 it is not optional:
Suggested change
| layouts?: KL_Layouts[]; | |
| layouts: KL_Layouts[]; |
| * <layouts>, <modifierMap>, <keyMapSet>, <actions>, <terminators> | ||
| * the 5 main elements. | ||
| */ | ||
| //layoutsMM: KL_Layouts[]; |
Member
There was a problem hiding this comment.
Suggested change
| //layoutsMM: KL_Layouts[]; |
Comment on lines
+26
to
+28
| /** | ||
| * attributes of the root element <keyboard> | ||
| */ |
Member
There was a problem hiding this comment.
This comment is attached to the group property as a javadoc-style comment. This is probably not what you want. As it is a structural comment, you could either go with (my preference):
Suggested change
| /** | |
| * attributes of the root element <keyboard> | |
| */ | |
| // attributes of the root element <keyboard> |
or:
Suggested change
| /** | |
| * attributes of the root element <keyboard> | |
| */ | |
| /* | |
| * attributes of the root element <keyboard> | |
| */ |
This needs to be corrected throughout this file.
You have the option to add javadoc-style comments above the interface itself and/or for each property.
| /** | ||
| * <action> the sub element of <actions> | ||
| */ | ||
| action?: KL_Action[]; |
Member
There was a problem hiding this comment.
Suggested change
| action?: KL_Action[]; | |
| action: KL_Action[]; |
|
|
||
|
|
||
|
|
||
| export interface ProcessedData { |
Member
There was a problem hiding this comment.
The doc comment needs to go above the export interface ProcessedData { line!
| * @param jsonObj containing filename, behaviorand rules of a json object | ||
| * @return an ProcessedData containing all data ready to print out | ||
| */ | ||
| private convert(jsonObj: any, inputfilename: string, outputFilename?: string): ProcessedData { |
Member
There was a problem hiding this comment.
Suggested change
| private convert(jsonObj: any, inputfilename: string, outputFilename?: string): ProcessedData { | |
| private convert(jsonObj: Keylayout.KeylayoutXMLSourceFile, inputfilename: string, outputFilename?: string): ProcessedData { |
We have the type, we need to use it!
Comment on lines
+9
to
+31
| <!ELEMENT keyboard (layouts, keyMapSet) > | ||
| <!ELEMENT keyboard (modifierMap)* > | ||
| <!ATTLIST keyboard (group, id, name, maxout) #REQUIRED > | ||
|
|
||
| <!ELEMENT layouts ( layout)* > | ||
| <!ATTLIST layout (first, last, mapSet, modifiers) #REQUIRED > | ||
|
|
||
| <!ELEMENT modifierMap (keyMapSelect)*> | ||
| <!ATTLIST modifierMap (id, defaultIndex) #REQUIRED > | ||
|
|
||
| <!ELEMENT keyMapSelect (modifier)*> | ||
| <!ATTLIST keyMapSelect (mapIndex) #REQUIRED > | ||
|
|
||
| <!ATTLIST modifier (keys) #REQUIRED > | ||
|
|
||
| <!ELEMENT keyMapSet ( keyMap)* > | ||
| <!ATTLIST keyMapSet (id) #REQUIRED > | ||
|
|
||
| <!ELEMENT keyMap (key)* > | ||
| <!ATTLIST keyMap (index) #REQUIRED > | ||
|
|
||
| <!ATTLIST key (code) #REQUIRED > | ||
| <!ATTLIST key (action #REQUIRED output #IMPLIED | output #REQUIRED action #IMPLIED)> |
Member
There was a problem hiding this comment.
We should use the TN2056 DTD if possible, unless it doesn't match reality:
<!-- Overall structure -->
<!ELEMENT keyboard (layouts+, modifierMap+, keyMapSet+, actions*, terminators*)>
<!ATTLIST keyboard group NMTOKEN #REQUIRED >
<!ATTLIST keyboard id NMTOKEN #REQUIRED >
<!ATTLIST keyboard name CDATA #REQUIRED >
<!ATTLIST keyboard maxout NMTOKEN #IMPLIED >
<!-- Hardware layout elements -->
<!ELEMENT layouts (layout+) >
<!ELEMENT layout EMPTY >
<!ATTLIST layout first NMTOKEN #REQUIRED >
<!ATTLIST layout last NMTOKEN #REQUIRED >
<!ATTLIST layout modifiers IDREF #REQUIRED >
<!ATTLIST layout mapSet IDREF #REQUIRED >
<!-- Modifier descriptions -->
<!ELEMENT modifierMap (keyMapSelect+) >
<!ATTLIST modifierMap id ID #REQUIRED >
<!ATTLIST modifierMap defaultIndex NMTOKEN #REQUIRED >
<!ELEMENT keyMapSelect (modifier+) >
<!ATTLIST keyMapSelect mapIndex NMTOKEN #REQUIRED >
<!ELEMENT modifier EMPTY >
<!ATTLIST modifier keys CDATA #REQUIRED >
<!-- Keyboard mapping -->
<!ELEMENT keyMapSet (keyMap+) >
<!ATTLIST keyMapSet id ID #REQUIRED >
<!ELEMENT keyMap (key+) >
<!ATTLIST keyMap index NMTOKEN #REQUIRED >
<!ATTLIST keyMap baseMapSet IDREF #IMPLIED >
<!ATTLIST keyMap baseIndex NMTOKEN #IMPLIED >
<!ELEMENT key (action*) >
<!ATTLIST key code NMTOKEN #REQUIRED >
<!ATTLIST key output CDATA #IMPLIED >
<!ATTLIST key action IDREF #IMPLIED >
<!-- Actions (state records) -->
<!ELEMENT actions (action+) >
<!ELEMENT action (when+) >
<!ATTLIST action id ID #IMPLIED >
<!ELEMENT when EMPTY >
<!ATTLIST when state NMTOKEN #REQUIRED >
<!ATTLIST when through NMTOKEN #IMPLIED >
<!ATTLIST when output CDATA #IMPLIED >
<!ATTLIST when multiplier NMTOKEN #IMPLIED >
<!ATTLIST when next NMTOKEN #IMPLIED >
<!-- Terminators -->
<!ELEMENT terminators (when+) >
| <xs:complexType> | ||
| <xs:sequence> | ||
|
|
||
| <xs:element name="layouts"> |
Member
There was a problem hiding this comment.
We can add some constraints, e.g. layouts may be only 1 so minOccurs="1" maxOccurs="1". Need to review each element.
adapt elements of keylayout-xml boxArray parsing ( tests, validate of and error msg for different amount of keymap<->keymapSelect) first version keylayout.dtd, keylayout.xsd, keylayout.schema.json
add checkForCorrespondingElements() to validate() min/maxOccurs added in keylayout.xsd edited keylayout.dtd (possible remaining squiggle lines will be addressed in PR#15860
…napp/keyman into feat/developer/kmc-convert
SabineSIL
commented
Apr 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
kmc-convert provides tooling to convert between various common keyboard description file formats. Each conversion will be implemented in its single, separate module (and each module will done in its own PR)
This PR will address the conversion keylayout → kmn
@keymanapp-test-bot skip