feat(developer): Support loading XML LDML keyboards in TIKE 🦕 #9963

mcdurdin · 2023-11-08T01:37:24Z

Relates to #9948.

This PR may be more easily reviewed commit-by-commit.

removed obsolete project code: Removes old state load/save from each project file type, and the unused WarnAsError property, in preparation for xmlLdmlProjectFile.
ldmlKeyboard3.dtd
Add basic LDML XML keyboard file support to TIKE projects
Split dmActionsDebugger out of dmActionsKeyboardEditor:
- Because the debugger actions are used only with the kmn keyboard editor, it makes sense to keep them as a separate module, in preparation for supporting XmlLdmlKeyboards.
add xmlLdml types and form, rename ActiveEditor:
- Adds XmlLdml keyboard editor form based on the plain text Editor for now.
- ActiveEditor is a name used for multiple purposes. Rename to ActiveKmnKeyboardEditor and introduce ActiveLdmlKeyboardEditor alongside.
Actions to support LdmlKeyboardEditor:
- Adds actions to support LdmlKeyboardEditor, and refactors and reorganizes the existing actions along the way to make them clearer.
- Compile, Install, and Uninstall supported at this time

A full user test of the new XML keyboard functionality will be included in a subsequent PR; there are a number of incomplete features at present which make test difficult.

Removes old state load/save from each project file type, and the unused WarnAsError property, in preparation for xmlLdmlProjectFile.

…jects

Adds XmlLdml keyboard editor form based on the plain text Editor for now. ActiveEditor is a name used for multiple purposes. Rename to ActiveKmnKeyboardEditor and introduce ActiveLdmlKeyboardEditor alongside.

Because the debugger actions are used only with the kmn keyboard editor, it makes sense to keep them as a separate module, in preparation for supporting XmlLdmlKeyboards.

Adds actions to support LdmlKeyboardEditor, and refactors and reorganizes the existing actions along the way to make them clearer. Compile, Install, and Uninstall supported at this time

keymanapp-test-bot · 2023-11-08T01:37:27Z

User Test Results

Test specification and instructions

User tests are not required

ermshiperete · 2023-11-08T03:50:48Z

GHA packaging builds are failing because the branch is missing the latest changes from master. However, GHA packaging fails on setting the status, so the failure is not visible here.

darcywong00 · 2023-11-14T08:46:22Z

developer/src/tike/project/Keyman.Developer.System.Project.xmlLdmlProjectFile.pas

+  // This appears to be a Delphi compiler bug (RSP-20457)
+  // Workaround is to make a copy of the parameter locally
+  // which fixes the reference counting.
+  FTempFileVersion := FileVersion;


Is this still around? Issue 631 was closed in 2018.

RSP-20457 is still not fixed.

darcywong00 · 2023-11-14T08:58:30Z

developer/src/tike/project/Keyman.Developer.System.Project.xmlLdmlProjectFileAction.pas

+  CleanFile(OutputFileName);
+
+  if Targets * KMXKeymanTargets <> [] then
+    CleanFile(ExtractFilePath(FileName) + ExtractFileName(ChangeFileExt(KVKFileName, '.kvk')), True);


Why is KVKFileName used to establish the file name for CleanFile?

This is removing a build output, the .kvk file, if it exists.

jahorton · 2023-11-14T09:00:10Z

developer/src/tike/project/Keyman.Developer.UI.Project.xmlLdmlProjectFileUI.pas

+
+function TxmlLdmlProjectFileUI.TestKeymanWeb(FSilent: Boolean): Boolean;   // I4409
+(*var
+  FCompiledName: string;


This one of them multi-line comments of dead-code we usually remove?

Granted, I see this is the part where we'd test an LDML keyboard in Web, which isn't going to be supported for this version. But... surely just saying "Hey, just CTRL-F this function from this other class / file when the time comes" should work well enough? Or do we expect significant differences in how it'd work?

This is on my list to address now in #9948.

(Oh yes, there will be significant differences in the kmn debugger and ldml debugger -- they will be completely divergent implementations, I expect)

darcywong00 · 2023-11-14T12:58:52Z

developer/src/tike/project/Keyman.Developer.UI.Project.xmlLdmlProjectFileUI.pas

+      if not modActionsMain.actFileSave.Execute then Exit;
+    end
+    else
+      Exit;


So when ProjecFile is modified and silent is true, this just exists as NOOP?

Correct. It exits with a false return value so the caller knows that things didn't work out.

jahorton

For anything "actions", all I can give is an RSLGTM. I'm not familiar with that abstraction from Core whatsoever, so I can't really give much feedback there.

For the rest of it, a plain ol' LGTM.

jahorton · 2023-11-15T01:28:27Z

developer/src/tike/project/Keyman.Developer.System.Project.xmlLdmlProjectFile.pas

+    procedure Load(node: IXMLNode); override;   // I4698
+    procedure Save(node: IXMLNode); override;   // I4698
+    procedure LoadState(node: IXMLNode); override;   // I4698
+    procedure SaveState(node: IXMLNode); override;   // I4698


While I'm not quite following the inheritance and/or polymorphism patterns used here and in ProjectFile.pas (as noticed via commit 1), I can see a pretty strong correlation between the methods.

I'm mostly confused why these procedures are override here but virtual there; I don't see the inheritance path between the two, and personally would've expected them to be sibling derived classes of a common base. But those might be small nits and partly due to my inexperience with Delphi in particular.

The implementation in ProjectFile.pas is the base class, hence it is virtual. The other classes are the overrides and extend the base functionality.

jahorton · 2023-11-15T01:31:15Z

developer/src/tike/project/Keyman.Developer.UI.Project.xmlLdmlProjectFileUI.pas

+
+function TxmlLdmlProjectFileUI.TestKeymanWeb(FSilent: Boolean): Boolean;   // I4409
+(*var
+  FCompiledName: string;


Granted, I see this is the part where we'd test an LDML keyboard in Web, which isn't going to be supported for this version. But... surely just saying "Hey, just CTRL-F this function from this other class / file when the time comes" should work well enough? Or do we expect significant differences in how it'd work?

jahorton · 2023-11-15T01:37:34Z

developer/src/tike/actions/dmActionsDebugger.pas

+    dlgFont: TFontDialog;
+    actDebugViewState: TAction;
+    actDebugSwitchToDebuggerWindow: TAction;
+    procedure actionsKeyboardEditorUpdate(Action: TBasicAction;


As best as I can tell, the method above is new, but all that follow in this list have been migrated from dmActionsKeyboardEditor within this commit.

keyman-server · 2023-11-15T18:04:17Z

Changes in this pull request will be available for download in Keyman version 17.0.211-alpha

mcdurdin added 6 commits November 7, 2023 14:57

chore(developer): remove obsolete project code

857017f

Removes old state load/save from each project file type, and the unused WarnAsError property, in preparation for xmlLdmlProjectFile.

chore(common): ldmlKeyboard3.dtd

1d3cb5e

feat(developer): Add basic LDML XML keyboard file support to TIKE pro…

7b8fe32

…jects

refactor(developer): add xmlLdml types and form, rename ActiveEditor

befe2b8

Adds XmlLdml keyboard editor form based on the plain text Editor for now. ActiveEditor is a name used for multiple purposes. Rename to ActiveKmnKeyboardEditor and introduce ActiveLdmlKeyboardEditor alongside.

feat(developer): Split dmActionsDebugger out of dmActionsKeyboardEditor

7c937e7

Because the debugger actions are used only with the kmn keyboard editor, it makes sense to keep them as a separate module, in preparation for supporting XmlLdmlKeyboards.

feat(developer): Actions to support LdmlKeyboardEditor

a923537

Adds actions to support LdmlKeyboardEditor, and refactors and reorganizes the existing actions along the way to make them clearer. Compile, Install, and Uninstall supported at this time

mcdurdin requested review from jahorton and darcywong00 as code owners November 8, 2023 01:37

keymanapp-test-bot bot added this to the A17S25 milestone Nov 8, 2023

github-actions bot added developer/ common/ common/resources/ Build infrastructure common/web/ developer/ide/ feat labels Nov 8, 2023

mcdurdin mentioned this pull request Nov 8, 2023

feat(developer): Keyman Developer v2.0 Projects #9948

Closed

69 tasks

mcdurdin changed the title ~~feat(developer): Support loading XML LDML keyboards in TIKE~~ feat(developer): Support loading XML LDML keyboards in TIKE 🦕 Nov 12, 2023

mcdurdin modified the milestones: A17S25, A17S26 Nov 13, 2023

darcywong00 reviewed Nov 14, 2023

View reviewed changes

jahorton reviewed Nov 14, 2023

View reviewed changes

darcywong00 reviewed Nov 14, 2023

View reviewed changes

jahorton approved these changes Nov 15, 2023

View reviewed changes

Base automatically changed from chore/developer/9948-hide-remove-project to master November 15, 2023 02:46

Merge branch 'master' into feat/developer/9948-load-xml-ldml-files

b8dcc1b

mcdurdin merged commit 37edb4f into master Nov 15, 2023
2 checks passed

mcdurdin deleted the feat/developer/9948-load-xml-ldml-files branch November 15, 2023 02:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(developer): Support loading XML LDML keyboards in TIKE 🦕 #9963

feat(developer): Support loading XML LDML keyboards in TIKE 🦕 #9963

mcdurdin commented Nov 8, 2023 •

edited

Loading

keymanapp-test-bot bot commented Nov 8, 2023 •

edited

Loading

ermshiperete commented Nov 8, 2023

darcywong00 Nov 14, 2023

mcdurdin Nov 14, 2023

darcywong00 Nov 14, 2023

mcdurdin Nov 15, 2023

jahorton Nov 14, 2023

jahorton Nov 15, 2023

mcdurdin Nov 15, 2023

mcdurdin Nov 15, 2023

darcywong00 Nov 14, 2023

mcdurdin Nov 15, 2023

jahorton left a comment

jahorton Nov 15, 2023

mcdurdin Nov 15, 2023

jahorton Nov 15, 2023

jahorton Nov 15, 2023

keyman-server commented Nov 15, 2023

feat(developer): Support loading XML LDML keyboards in TIKE 🦕 #9963

feat(developer): Support loading XML LDML keyboards in TIKE 🦕 #9963

Conversation

mcdurdin commented Nov 8, 2023 • edited Loading

keymanapp-test-bot bot commented Nov 8, 2023 • edited Loading

User Test Results

ermshiperete commented Nov 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jahorton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keyman-server commented Nov 15, 2023

mcdurdin commented Nov 8, 2023 •

edited

Loading

keymanapp-test-bot bot commented Nov 8, 2023 •

edited

Loading