Skip to content

refactor(web): clarify function scope and update some function comments #16532

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.

Sign up for GitHub

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

Merged
ermshiperete merged 3 commits into from
Sep 8, 2026
Merged

refactor(web): clarify function scope and update some function comments #16532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bd827f8
refactor(web): clarify function scope and update some function comments
ermshiperete Sep 4, 2026
04a5cd1
chore(web): minor improvements and fixes
ermshiperete Sep 4, 2026
d2abc84
refactor(web): simplify logic of `prepareKeyboardForActivation`
ermshiperete Sep 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion web/src/app/browser/src/contextManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ export class ContextManager extends ContextManagerBase<BrowserConfiguration> {
}
}

public setActiveTextStore(textStore: AbstractElementTextStore<any>, sendEvents?: boolean) {
public setActiveTextStore(textStore: AbstractElementTextStore<any>, sendEvents?: boolean): void {
const previousTextStore = this.mostRecentTextStore;
const originalTextStore = this.activeTextStore; // may differ, depending on focus state.

Expand Down Expand Up @@ -410,6 +410,11 @@ export class ContextManager extends ContextManagerBase<BrowserConfiguration> {
* activates the keyboard if the specified control represents the
* currently-active context.
*
* If kbdId and langId are both null, the control will use the global
* keyboard. If both are the empty string, the control will use the
* system keyboard (on desktop), or the first installed keyboard (on
* touch devices).
*
* This is the core method that backs
* https://help.keyman.com/developer/engine/web/current-version/reference/core/setKeyboardForControl.
* @param textStore
Expand Down
23 changes: 12 additions & 11 deletions web/src/app/browser/src/keymanEngine.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,14 +315,14 @@ export class KeymanEngine extends KeymanEngineBase<BrowserConfiguration, Context
}

/**
* Function getKeyboardForControl
* Scope Public
* @param {Element} Pelem Control element
* @return {string|null} The independently-managed keyboard for the control.
* Description Returns the keyboard ID of the current independently-managed keyboard for this control.
* If it is currently following the global keyboard setting, returns null instead.
* Returns the keyboard ID of the current independently-managed keyboard for this control.
* If it is currently following the global keyboard setting, returns null instead.
*
* See https://help.keyman.com/developer/engine/web/current-version/reference/core/getKeyboardForControl
*
* @param {Element} Pelem Control element
* @return {string|null} The independently-managed keyboard for the control,
* or null if it is following the global keyboard setting.
*/
public getKeyboardForControl(Pelem: HTMLElement): string | null{
const textStore = textStoreForElement(Pelem);
Expand All @@ -331,12 +331,13 @@ export class KeymanEngine extends KeymanEngineBase<BrowserConfiguration, Context

// Is not currently published API... but it exists.
/**
* Function getLanguageForControl
* Scope Public
* Returns the language code used with the current independently-managed keyboard
* for this control. If it is currently following the global keyboard setting,
* returns null instead.
*
* @param {Element} Pelem Control element
* @return {string|null} The independently-managed keyboard for the control.
* Description Returns the language code used with the current independently-managed keyboard for this control.
* If it is currently following the global keyboard setting, returns null instead.
* @return {string|null} The independently-managed keyboard for the control,
* or null if it is following the global keyboard setting.
*/
public getLanguageForControl(Pelem: HTMLElement): string | null {
const textStore = textStoreForElement(Pelem);
Expand Down
36 changes: 14 additions & 22 deletions web/src/app/ui/kmwuitoggle.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ if(!keyman) {
* // and the like are defined on individual instances later.
* // It thinks they're always null.
**/
button(_src: string, _caption: string, _selected: boolean) {
private button(_src: string, _caption: string, _selected: boolean) {
/**
* Only ui.controllerHovered is referenced here: it'd be easy enough to toggle it via closure
* and extract this inner class into its own definition outside of `class ToggleUI`.
Expand Down Expand Up @@ -401,11 +401,9 @@ if(!keyman) {
};

/**
* Function Initialize
* Scope Private
* Description Initialize Toggle User Interface
* Initialize Toggle User Interface
**/
initialize() {
public initialize() {
//Never initialize before KMW!
if(!keyman.initialized || util.isTouchDevice()) {
return;
Expand Down Expand Up @@ -482,12 +480,10 @@ if(!keyman) {
}

/**
* Function updateKeyboardList
* Scope Private
* Description Rebuild the UI and keyboard list
* Rebuild the UI and keyboard list
**/
readonly updateKeyboardList = () => {
if(!(keyman.initialized || this.initialized)) {
public readonly updateKeyboardList = () => {
if (!(keyman.initialized || this.initialized)) {
return; //TODO: may want to restart the timer??
}

Expand Down Expand Up @@ -562,10 +558,9 @@ if(!keyman) {
// var _SelectedMenuItem;

/**
* Function selecKbd
* Scope Private
* Select a keyboard from the drop down menu
*
* @param {number} kbdIndex
* Description Select a keyboard from the drop down menu
**/
private async selectKbd(kbdIndex: number): Promise<boolean> {
let name: string, languageCode: string;
Expand All @@ -588,13 +583,12 @@ if(!keyman) {
};

/**
* Function updateMenu
* Scope Private
* Updates the menu selection when a change is required
*
* @param {string} kbdName
* @param {?string=} lgCode
* Description Updates the menu selection when a change is required
**/
updateMenu(kbdName: string, lgCode: string) {
public updateMenu(kbdName: string, lgCode: string) {
let _k=document.getElementById('KMWSel_$');

for(let i=0; i < this.keyboards.length; i++) {
Expand Down Expand Up @@ -629,7 +623,7 @@ if(!keyman) {
}
}

get stylingCSS() {
private get stylingCSS() {
return `
#KeymanWeb_KbdList {
display: block;
Expand Down Expand Up @@ -713,11 +707,9 @@ if(!keyman) {
}

/**
* Function createMenu
* Scope Private
* Description Create the drop down menu and populate with loaded KeymanWeb keyboards
* Create the drop down menu and populate with loaded KeymanWeb keyboards
**/
createMenu() {
private createMenu() {
if(typeof(this.keyboardMenu) == 'undefined') { // I2403 - Allow toggle design to be loaded twice
this.keyboardMenu = util.createElement('ul');
this.keyboardMenu.id='KeymanWeb_KbdList';
Expand Down
2 changes: 1 addition & 1 deletion web/src/app/webview/src/contextManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ export class ContextManager extends ContextManagerBase<WebviewConfiguration> {
return this._activeKeyboard;
}

activateKeyboardForTextStore(kbd: { keyboard: Keyboard, metadata: KeyboardStub }, textStore: TextStore) {
protected activateKeyboardForTextStore(kbd: { keyboard: Keyboard, metadata: KeyboardStub }, textStore: TextStore) {
// `textStore` is irrelevant for `app/webview`, as it'll only ever use 'global' keyboard settings.

// Clone the object to prevent accidental by-reference changes.
Expand Down
128 changes: 61 additions & 67 deletions web/src/engine/src/main/contextManagerBase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ export abstract class ContextManagerBase<MainConfig extends EngineConfiguration>
const wasNull = !this.activeKeyboard;

// If there was a previous activation attempt set and still active for the specified keyboard textStore,
// cancel it. For exmaple, if the user selects a preloaded keyboard after having tried to select one
// cancel it. For example, if the user selects a preloaded keyboard after having tried to select one
// still async-loading, we should go with the later setting - the preloaded one.
this.findAndPopActivation(this.currentKeyboardSrcTextStore());

Expand Down Expand Up @@ -314,31 +314,25 @@ export abstract class ContextManagerBase<MainConfig extends EngineConfiguration>
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
let requestedStub: KeyboardStub = null;
if(keyboardId) {
requestedStub = this.keyboardCache.getStub(keyboardId, languageCode);
} else {
languageCode == '';
}

const requestedStub: KeyboardStub = this.keyboardCache.getStub(keyboardId, languageCode);
if(!requestedStub) {
if(keyboardId) {
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)}`);
} else {
return {
keyboard: Promise.resolve(null),
metadata: null
}
}
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 (this.activeKeyboard?.metadata && keyboardId == this.activeKeyboard.metadata.id) {
if (keyboardId == this.activeKeyboard?.metadata?.id) {
const {keyboard} = this.activeKeyboard;
// In this case, the keyboard is loaded; just update the stub.

Expand All @@ -349,61 +343,61 @@ export abstract class ContextManagerBase<MainConfig extends EngineConfiguration>
}

// Determine if the keyboard was previously loaded but is not active; use the cached, pre-loaded version if so.
let keyboard: Keyboard;
if(keyboard = this.keyboardCache.getKeyboardForStub(requestedStub)) {
const keyboard: Keyboard = this.keyboardCache.getKeyboardForStub(requestedStub);
if (keyboard) {
return {
keyboard: Promise.resolve(keyboard),
metadata: requestedStub
};
} else {

@ermshiperete ermshiperete Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// It's async time - the keyboard is not preloaded within the cache. Use the stub's data to load it.

// `beforeKeyboardChange` - first call
this.emit('beforekeyboardchange', requestedStub);

const defermentPromise = this.engineConfig.deferForInitialization.then(() => {
// Provide a Promise for completion of the async load process.
const completionPromise = new ManagedPromise<Error>();
this.emit('keyboardasyncload', requestedStub, completionPromise.corePromise);

const keyboardPromise = this.keyboardCache.fetchKeyboard(requestedStub.KI);
const timeoutPromise = new Promise<Keyboard>((resolve, reject) => {
const timeoutMsg = `Download of ${requestedStub.KI} for language ${requestedStub.langId} timed out.`;
window.setTimeout(() => reject(new Error(timeoutMsg)), ContextManagerBase.TIMEOUT_THRESHOLD);
});

const combinedPromise = Promise.race([keyboardPromise, timeoutPromise]);

// Ensure the async-load Promise completes properly.
combinedPromise.then(() => {
completionPromise.resolve(null);
// Prevent any 'unhandled Promise rejection' events that may otherwise occur from the timeout promise.
timeoutPromise.catch(() => {});
});
combinedPromise.catch((err) => {
completionPromise.resolve(err);
throw err;
});

return combinedPromise;
}

// It's async time - the keyboard is not preloaded within the cache. Use the stub's data to load it.

// `beforeKeyboardChange` - first call
this.emit('beforekeyboardchange', requestedStub);

const defermentPromise = this.engineConfig.deferForInitialization.then(() => {
// Provide a Promise for completion of the async load process.
const completionPromise = new ManagedPromise<Error>();
this.emit('keyboardasyncload', requestedStub, completionPromise.corePromise);

const keyboardPromise = this.keyboardCache.fetchKeyboard(requestedStub.KI);
const timeoutPromise = new Promise<Keyboard>((resolve, reject) => {
const timeoutMsg = `Download of ${requestedStub.KI} for language ${requestedStub.langId} timed out.`;
window.setTimeout(() => reject(new Error(timeoutMsg)), ContextManagerBase.TIMEOUT_THRESHOLD);
});

// Now the fun part: note the original call's parameters as a pending activation.
const promise = this.deferredKeyboardActivation(defermentPromise, requestedStub, this.currentKeyboardSrcTextStore());
return {
keyboard: promise.then(async (activation) => {
// Is the activation we requested still pending, or was it cancelled in favor of a
// different activation in some manner?
if(!activation) {
// If the user chose to load a different keyboard afterward that would affect the same
// textStore, the activation is no longer valid.
return Promise.resolve(null);
} else {
return defermentPromise;
}
}),
metadata: requestedStub
}
const combinedPromise = Promise.race([keyboardPromise, timeoutPromise]);

// Ensure the async-load Promise completes properly.
combinedPromise.then(() => {
completionPromise.resolve(null);
// Prevent any 'unhandled Promise rejection' events that may otherwise occur from the timeout promise.
timeoutPromise.catch(() => {});
});
combinedPromise.catch((err) => {
completionPromise.resolve(err);
throw err;
});

return combinedPromise;
});

// Now the fun part: note the original call's parameters as a pending activation.
const promise = this.deferredKeyboardActivation(defermentPromise, requestedStub, this.currentKeyboardSrcTextStore());
return {
keyboard: promise.then(async (activation) => {
// Is the activation we requested still pending, or was it cancelled in favor of a
// different activation in some manner?
if(!activation) {
// If the user chose to load a different keyboard afterward that would affect the same
// textStore, the activation is no longer valid.
return Promise.resolve(null);
} else {
return defermentPromise;
}
}),
metadata: requestedStub
}
}
}
6 changes: 3 additions & 3 deletions web/src/engine/src/main/keymanEngineBase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -543,8 +543,8 @@ export class KeymanEngineBase<
/**
* Allow to change active keyboard by (internal) keyboard name
*
* @param {string} PInternalName Internal name
* @param {string} PLgCode Language code
* @param {string} keyboardId Keyboard name
* @param {string} languageCode Language code
*
* See https://help.keyman.com/developer/engine/web/current-version/reference/core/setActiveKeyboard
*/
Expand Down Expand Up @@ -632,4 +632,4 @@ export class KeymanEngineBase<
};
}

// Intent: define common behaviors for both primary app types; each then subclasses & extends where needed.
// Intent: define common behaviors for both primary app types; each then subclasses & extends where needed.