-
-
Notifications
You must be signed in to change notification settings - Fork 130
refactor(web): rename field core to inputProcessor 🎼
#15411
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
Conversation
The field of `KeymanEngineBase` that holds the `InputProcessor` used to be called `core`. This can be easily confused with Keyman Core, especially now that we're implementing web-core. This change therefore renames the field to `inputProcessor`. Test-bot: skip
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
core to inputProcessorcore to inputProcessor 🎼
mcdurdin
left a comment
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 don't disagree with your assessment, but we do have dependencies on the name core in other modules (first reference only for each file):
keyman.core.languageProcessor.on('statechange', onStateChange); keyman/developer/src/server/src/site/test.js
Line 256 in 7c5064a
keyman.core.contextDevice = targetDevice; // Note: KeymanWeb internal API keyman/ios/engine/KMEI/KeymanEngine/resources/Keyman.bundle/Contents/Resources/ios-host.js
Line 75 in 5b4df8e
if(!keyman.core.activeKeyboard && !keyman.keyboardRequisitioner.cache.defaultStub) { keyman/web/src/test/manual/web/regression-tests/src/test-runner.js
Lines 277 to 278 in 62ff9e6
if(keyman.core) { keyman.core.resetContext();
You should also fixup the unit tests at:
let core = new InputProcessor(device);
This change would need extensive testing across iOS, Android, and Developer web test.
Then, you should also fixup the Engine documentation where the "Keyman Core Module" is something else again:
The KeymanWeb core module is exposed to the developer as `window.keyman`.
This isn't a comprehensive audit of potential stumbling points. We need to be careful with JS/TS because internals can easily leak, like in this case.
If we do go ahead with this, the rename should be highlighted in release notes because even though it was an undocumented API, we used it ourselves to accomplish things, and other KeymanWeb integrators may well have done the same.
|
Some additional references:
|
|
I should have checked the rest of the repo. Thanks! Doesn't seem worth to continue at the moment, so I'm closing this PR. |
The field of
KeymanEngineBasethat holds theInputProcessorused to be calledcore. This can be easily confused with Keyman Core, especially now that we're implementing web-core. This change therefore renames the field toinputProcessor.Test-bot: skip