Skip to content

Commit

Permalink
core: catch command handler errors
Browse files Browse the repository at this point in the history
If a contribution throws an error during basic lifecycle steps, it might
kill whatever is trying to iterate over all the contributions.

A better strategy would be to catch errorred handlers and keep looking
for other contributions.

Also fix `.getToggledHandler` to not stop prematurely and actually test
if the handler is toggled or not.

Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>
  • Loading branch information
paul-marechal committed Aug 9, 2019
1 parent 3a541f2 commit 11badc8
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 15 deletions.
4 changes: 2 additions & 2 deletions packages/core/src/browser/quick-open/quick-command-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -221,8 +221,8 @@ export class CommandQuickOpenItem extends QuickOpenGroupItem {
}

getIconClass(): string | undefined {
const toggleHandler = this.commands.getToggledHandler(this.command.id);
if (toggleHandler && toggleHandler.isToggled && toggleHandler.isToggled()) {
const toggledHandler = this.commands.getToggledHandler(this.command.id);
if (toggledHandler) {
return 'fa fa-check';
}
return super.getIconClass();
Expand Down
34 changes: 23 additions & 11 deletions packages/core/src/common/command.ts
Original file line number Diff line number Diff line change
Expand Up @@ -244,24 +244,23 @@ export class CommandRegistry implements CommandService {
*/
// tslint:disable-next-line:no-any
isEnabled(command: string, ...args: any[]): boolean {
return this.getActiveHandler(command, ...args) !== undefined;
return typeof this.getActiveHandler(command, ...args) !== 'undefined';
}

/**
* Test whether there is a visible handler for the given command.
*/
// tslint:disable-next-line:no-any
isVisible(command: string, ...args: any[]): boolean {
return this.getVisibleHandler(command, ...args) !== undefined;
return typeof this.getVisibleHandler(command, ...args) !== 'undefined';
}

/**
* Test whether there is a toggled handler for the given command.
*/
// tslint:disable-next-line:no-any
isToggled(command: string, ...args: any[]): boolean {
const handler = this.getToggledHandler(command);
return handler && handler.isToggled ? handler.isToggled(...args) : false;
return typeof this.getToggledHandler(command, ...args) !== 'undefined';
}

/**
Expand Down Expand Up @@ -297,8 +296,12 @@ export class CommandRegistry implements CommandService {
const handlers = this._handlers[commandId];
if (handlers) {
for (const handler of handlers) {
if (!handler.isVisible || handler.isVisible(...args)) {
return handler;
try {
if (!handler.isVisible || handler.isVisible(...args)) {
return handler;
}
} catch (error) {
console.error(error);
}
}
}
Expand All @@ -313,8 +316,12 @@ export class CommandRegistry implements CommandService {
const handlers = this._handlers[commandId];
if (handlers) {
for (const handler of handlers) {
if (!handler.isEnabled || handler.isEnabled(...args)) {
return handler;
try {
if (!handler.isEnabled || handler.isEnabled(...args)) {
return handler;
}
} catch (error) {
console.error(error);
}
}
}
Expand All @@ -324,12 +331,17 @@ export class CommandRegistry implements CommandService {
/**
* Get a toggled handler for the given command or `undefined`.
*/
getToggledHandler(commandId: string): CommandHandler | undefined {
// tslint:disable-next-line:no-any
getToggledHandler(commandId: string, ...args: any[]): CommandHandler | undefined {
const handlers = this._handlers[commandId];
if (handlers) {
for (const handler of handlers) {
if (handler.isToggled) {
return handler;
try {
if (handler.isToggled && handler.isToggled(...args)) {
return handler;
}
} catch (error) {
console.error(error);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,14 +144,14 @@ export class ElectronMainMenuFactory {
items.push({
id: menu.id,
label: menu.label,
type: this.commandRegistry.getToggledHandler(commandId) ? 'checkbox' : 'normal',
type: this.commandRegistry.getToggledHandler(commandId, ...args) ? 'checkbox' : 'normal',
checked: this.commandRegistry.isToggled(commandId, ...args),
enabled: true, // https://github.com/theia-ide/theia/issues/446
visible: true,
click: () => this.execute(commandId, args),
accelerator
});
if (this.commandRegistry.getToggledHandler(commandId)) {
if (this.commandRegistry.getToggledHandler(commandId, ...args)) {
this._toggledCommands.add(commandId);
}
}
Expand Down

0 comments on commit 11badc8

Please sign in to comment.