refactor(web): clarify function scope and update some function comments - #16532
Conversation
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
| } else { | ||
| languageCode == ''; | ||
| languageCode = ''; | ||
| } |
There was a problem hiding this comment.
Good catch. However, if !keyboardId, we always (line 333) return quickly anyway:
return {
keyboard: Promise.resolve(null),
metadata: null
}So it has no impact because it was never used...
It'd be better to move the parameter nullish check to the top of the function and simplify the logic:
protected prepareKeyboardForActivation(
keyboardId: string,
languageCode?: string
): {keyboard: Promise<Keyboard>, metadata: KeyboardStub} {
if(!keyboardId) {
return {
keyboard: Promise.resolve(null),
metadata: null
}
}
// Set default language code
languageCode ||= '';
// Check that the saved keyboard is currently registered
const requestedStub: KeyboardStub = this.keyboardCache.getStub(keyboardId, languageCode);
if(!requestedStub) {
const availableStubList = this.keyboardCache.getStubList().map(stub => `${stub.KI}@${stub.KLC}`);
throw new Error(`No matching stub has been registered for keyboard ${keyboardId}. Available stubs: ${JSON.stringify(availableStubList)}`);
}
// Check if current keyboard matches requested keyboard, but not (necessarily) stub
if (keyboardId === this.activeKeyboard?.metadata?.id) {
const {keyboard} = this.activeKeyboard;
// In this case, the keyboard is loaded; just update the stub.
return {
keyboard: Promise.resolve(keyboard),
metadata: requestedStub
};
}Secondary question, why would we ever call prepareKeyboardForActivation with a nullish keyboardId? That smells buggy in itself.
There was a problem hiding this comment.
Secondary question, why would we ever call
prepareKeyboardForActivationwith a nullishkeyboardId? That smells buggy in itself.
KeymanEngine.removeKeyboards calls activateKeyboard (which then calls prepareKeyboardForActivation) with an empty string: this.contextManager.activateKeyboard('', '');
Build-bot: skip build:web Test-bot: skip
5640599 to
04a5cd1
Compare
Co-authored-by: Marc Durdin <marc@durdin.net>
| keyboard: Promise.resolve(keyboard), | ||
| metadata: requestedStub | ||
| }; | ||
| } else { |
There was a problem hiding this comment.
The rest of the changes in this file are just removing the else (since the if returns), and adjusting the indentation. You might want to ignore the whitespace changes when reviewing.
mcdurdin
left a comment
There was a problem hiding this comment.
LGTM.
I think we should refactor the callers to prepareKeyboardForActivation() such that they never call with a nullish keyboardId because it seems like that is really out of scope for the purpose of this function, but it's good to keep this refactor small.
|
Changes in this pull request will be available for download in Keyman version 19.0.282-alpha |
Build-bot: skip build:web
Test-bot: skip