fix(developer): bring kmc-package inline with file format spec 🗜 #9210
Conversation
Fixes #9208. Fixes #9209. Fixes #9203. Fixes #9198. The changes here overlap in kmp-compiler.ts. Addresses several issues with the package compiler: * refactors the reading of the metadata from keyboards to make use of it when refreshing the keyboard metadata in kmp.json (#9208). This is the bulk of the changes. * Fixes case on some fields in the kps file format (#9209) * Removes code which emitted strings table to kmp.json, as these were only ever used for package installer executables (#9203) * Cleans up code which emitted Start Menu items to make it match spec (#9198)
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
| @@ -71,7 +71,11 @@ export class KmpCompiler { | |||
| if(kps.options) { | |||
| for (let [src,dst] of keys) { | |||
| if (kps.options[src]) { | |||
| kmp.options[dst] = kps.options[src]; | |||
| if(dst == 'graphicFile' || dst == 'readmeFile') { | |||
| kmp.options[dst] = /[/\\]?([^/\\]*)$/.exec(kps.options[src])[1]; | |||
There was a problem hiding this comment.
So... why are we stripping the path selectively here? I assume there are other dst values that should remain unstripped?
There was a problem hiding this comment.
Those are the only two options that are paths
| @@ -94,7 +94,7 @@ export interface KpsFileKeyboard { | |||
| version: string; | |||
| oSKFont?: string; | |||
| displayFont?: string; | |||
| rtl?: boolean; | |||
| rTL?: string; | |||
There was a problem hiding this comment.
What sort of convention is this? I'd rather keep it purely lower-cased. Also not a fan of oSKFont. (Title-case for an abbrevation is... okay if necessary. I'm generally not a fan of mixed-case within a coding abbreviation unless it causes issues parsing something in camelCase or similar.)
I checked the referenced issue, but there's no detail there for me to compare against.
There was a problem hiding this comment.
It sounds like there may be some pre-established spec that mandates this... but there was no link for me to follow and verify that. I'll definitely acquiesce if this is the case.
There was a problem hiding this comment.
We've discussed this before. It's a "feature" of the xml2js implementation and options selected which I am still considering winding back, see #7353. I reckon we should call it dONKEYCase (or aSSCase but that's a little dangerous).
There was a problem hiding this comment.
Ah, so not exactly a "spec", but it's a pre-established thing we have to put up with for now b/c triage. 👍 Good to have the reference, at least.
Fixes #9198.
Fixes #9203.
Fixes #9204.
Fixes #9205.
Fixes #9207.
Fixes #9208.
Fixes #9209.
Seven fixes. Four of these are in a single commit, as they were too tangled to easily do as separate commits. A thousand apologies.
306934e addresses several issues with the package compiler:
3773522 adds kmc-package support for writing kmp.inf, fixing feat(developer): kmc-package needs to support kmp.inf 🗜 #9205.
2085c26 trims fields when emitting kmp.json, fixing bug(developer): kmc-package needs to trim fields in kmp.json 🗜 #9207.
84c04d5 strips paths from
readmeFileandgraphicFile, fixing bug(developer): kmc-package did not strip paths fromreadmeFileorgraphicFile🗜 #9204.@keymanapp-test-bot skip