Skip to content
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

execute already registered command in the same tick #7851

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions .gitpod.dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ ENV TRIGGER_REBUILD 3

# Install custom tools, runtime, etc.
RUN sudo apt-get update \
# firefox for testing via VNC
&& sudo apt-get install -y firefox \
# window manager
&& sudo apt-get install -y jwm \
# electron
Expand Down
4 changes: 4 additions & 0 deletions packages/core/src/browser/keybinding.ts
Original file line number Diff line number Diff line change
Expand Up @@ -434,6 +434,8 @@ export class KeybindingRegistry {
/**
* Tries to execute a keybinding.
*
* **IMPORTANT:** This method has to be synchronous.
*
* @param binding to execute
* @param event keyboard event.
*/
Expand Down Expand Up @@ -502,6 +504,8 @@ export class KeybindingRegistry {

/**
* Run the command matching to the given keyboard event.
*
* **IMPORTANT:** This method has to be synchronous.
*/
run(event: KeyboardEvent): void {
if (event.defaultPrevented) {
Expand Down
31 changes: 21 additions & 10 deletions packages/core/src/common/command.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { injectable, inject, named } from 'inversify';
import { Event, Emitter, WaitUntilEvent } from './event';
import { Disposable, DisposableCollection } from './disposable';
import { ContributionProvider } from './contribution-provider';
import { MaybePromise } from './types';

/**
* A command is a unique identifier of a function
Expand Down Expand Up @@ -285,24 +286,34 @@ export class CommandRegistry implements CommandService {
* Execute the active handler for the given command and arguments.
*
* Reject if a command cannot be executed.
*
* **IMPORTANT:** This method has to be synchronous.
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
async executeCommand<T>(commandId: string, ...args: any[]): Promise<T | undefined> {
executeCommand<T>(commandId: string, ...args: any[]): Promise<T | undefined> {
const handler = this.getActiveHandler(commandId, ...args);
if (handler) {
await this.fireWillExecuteCommand(commandId, args);
const result = await handler.execute(...args);
if (!handler) {
const argsMessage = args && args.length > 0 ? ` (args: ${JSON.stringify(args)})` : '';
Copy link
Member Author

Choose a reason for hiding this comment

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

(off-topic) but serializing of random args won't work, just imagine widget

we have to remove this code

Copy link
Contributor

@vzhukovs vzhukovs May 20, 2020

Choose a reason for hiding this comment

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

as I understand, the args count might be infinite and one of this arg can represent a widget, am I right?

// eslint-disable-next-line max-len
throw Object.assign(new Error(`The command '${commandId}' cannot be executed. There are no active handlers available for the command.${argsMessage}`), { code: 'NO_ACTIVE_HANDLER' });
}
let invocation: MaybePromise<T | undefined>;
const waitForCommand = this.fireWillExecuteCommand(commandId, args);
if (waitForCommand) {
invocation = waitForCommand.then(() => handler.execute(...args));
} else {
// execute already registered command in the same tick, see https://github.com/eclipse-theia/theia/issues/7542#issuecomment-631452852
invocation = handler.execute(...args);
}
return Promise.resolve(invocation).then(result => {
this.onDidExecuteCommandEmitter.fire({ commandId, args });
return result;
}
const argsMessage = args && args.length > 0 ? ` (args: ${JSON.stringify(args)})` : '';
// eslint-disable-next-line max-len
throw Object.assign(new Error(`The command '${commandId}' cannot be executed. There are no active handlers available for the command.${argsMessage}`), { code: 'NO_ACTIVE_HANDLER' });
});
}

// eslint-disable-next-line @typescript-eslint/no-explicit-any
protected async fireWillExecuteCommand(commandId: string, args: any[] = []): Promise<void> {
await WaitUntilEvent.fire(this.onWillExecuteCommandEmitter, { commandId, args }, 30000);
protected fireWillExecuteCommand(commandId: string, args: any[] = []): MaybePromise<void> {
return WaitUntilEvent.fire(this.onWillExecuteCommandEmitter, { commandId, args }, 30000);
}

/**
Expand Down
17 changes: 11 additions & 6 deletions packages/core/src/common/event.ts
Original file line number Diff line number Diff line change
Expand Up @@ -199,8 +199,8 @@ export class Emitter<T = any> {

return result;
}, {
maxListeners: Emitter.LEAK_WARNING_THRESHHOLD
}
maxListeners: Emitter.LEAK_WARNING_THRESHHOLD
}
);
}
return this._event;
Expand Down Expand Up @@ -308,11 +308,14 @@ export interface WaitUntilEvent {
/* eslint-enable @typescript-eslint/no-explicit-any */
}
export namespace WaitUntilEvent {
export async function fire<T extends WaitUntilEvent>(
/**
* **IMPORTANT:** This method has to be synchronous.
*/
export function fire<T extends WaitUntilEvent>(
emitter: Emitter<T>,
event: Pick<T, Exclude<keyof T, 'waitUntil'>>,
timeout: number | undefined = undefined
): Promise<void> {
): MaybePromise<void> {
const waitables: Promise<void>[] = [];
const asyncEvent = Object.assign(event, {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
Expand All @@ -333,10 +336,12 @@ export namespace WaitUntilEvent {
if (!waitables.length) {
return;
}
let invocation;
if (timeout !== undefined) {
await Promise.race([Promise.all(waitables), new Promise(resolve => setTimeout(resolve, timeout))]);
invocation = Promise.race([Promise.all(waitables), new Promise(resolve => setTimeout(resolve, timeout))]);
} else {
await Promise.all(waitables);
invocation = Promise.all(waitables);
}
return invocation.then(() => Promise.resolve());
}
}