From 3d28b101cf924dfee4f83621b857b7f39cab8b51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Paul=20Mar=C3=A9chal?= Date: Thu, 8 Aug 2019 16:49:58 -0400 Subject: [PATCH] core: catch command handler errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../quick-open/quick-command-service.ts | 4 +-- packages/core/src/common/command.ts | 34 +++++++++++++------ .../menu/electron-main-menu-factory.ts | 4 +-- 3 files changed, 27 insertions(+), 15 deletions(-) diff --git a/packages/core/src/browser/quick-open/quick-command-service.ts b/packages/core/src/browser/quick-open/quick-command-service.ts index 83a5b942793d4..8d749fac4bb09 100644 --- a/packages/core/src/browser/quick-open/quick-command-service.ts +++ b/packages/core/src/browser/quick-open/quick-command-service.ts @@ -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(); diff --git a/packages/core/src/common/command.ts b/packages/core/src/common/command.ts index 42456d4a94abe..2dff471b9f696 100644 --- a/packages/core/src/common/command.ts +++ b/packages/core/src/common/command.ts @@ -244,7 +244,7 @@ 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'; } /** @@ -252,7 +252,7 @@ export class CommandRegistry implements CommandService { */ // 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'; } /** @@ -260,8 +260,7 @@ export class CommandRegistry implements CommandService { */ // 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'; } /** @@ -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); } } } @@ -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); } } } @@ -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); } } } diff --git a/packages/core/src/electron-browser/menu/electron-main-menu-factory.ts b/packages/core/src/electron-browser/menu/electron-main-menu-factory.ts index 118da9318f7b0..2e624292ab7df 100644 --- a/packages/core/src/electron-browser/menu/electron-main-menu-factory.ts +++ b/packages/core/src/electron-browser/menu/electron-main-menu-factory.ts @@ -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); } }