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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
change(web): facilitates differentiated configuration objects for KMW variants 🧩 #8458
change(web): facilitates differentiated configuration objects for KMW variants 🧩 #8458
Changes from 10 commits
2f6871f
ebf1002
8dedc5c
9c8c426
c9a4c19
18b6c83
3732834
236ebc2
581babe
da06b23
20567d4
d2f4e98
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't you just search for
::
? Although wouldn't it be better for the namespace to be separated fromKI
rather than mutating input data?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could see an argument for that, but the pre-modular namespacing system worked by prepending the package name as part of the ID. I opted to maintain that.
There's then the issue of
Package_id::Keyboard_kbd_id
vsKeyboard_Package_id::kbd_id
due to the wholeKeyboard_
ID-prefixing prepend bit that's been in KMW since forever. The function highlighted in the code block above is designed to ensure that it's always the former pattern and never the latter by detecting the latter and converting it over while leaving the former intact.Though... I'm not 100% sure that Package-prepended keyboards actually use the
Keyboard_
bit too in the existing implementation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmation for that last point...
(from the current
master
)keyman/web/src/engine/main/keyboards/kmwkeyboards.ts
Line 470 in 706cc07
Contrast with...
(also from the current
master
)keyman/web/src/app/embed/kmwembedded.ts
Lines 168 to 174 in 706cc07
Any incoming keyboard IDs are already mutated to have a prepended package ID as part of the keyboard's ID itself.
We need to make sure that the
Keyboard_
applies at the location we want, hence the if-condition seen above.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block best showcases the main reason for the change in constructor parameter ordering:
app/webview
(and eventuallyapp/browser
) will construct their ownConfiguration
object in their respectiveKeymanEngine
constructor. Each derived-class constructor may take additional parameters, but the first parameter - forworker
- will be consistent for both.The reason:
Worker
can't be specified at this level if we wish to provide separatedebug
vsrelease
build products. The non-minified + sourcemapped variant of the worker should be used fordebug
, while the minified + unsourcemapped variant would be used forrelease
. These will be specified by the top-level entry point for bothdebug
andrelease
esbuild
bundling configurations.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay, a
keyman.init
for the modularizedapp/webview
! Well, the start of one, anyway...