From ce09d7137df61800893e01ebfd315e8d6eed8154 Mon Sep 17 00:00:00 2001 From: Colin Grant Date: Wed, 2 Jun 2021 18:07:37 -0500 Subject: [PATCH 1/3] Add utility to handle profusion of clicks Signed-off-by: Colin Grant --- .../core/src/browser/tree/tree-widget.tsx | 11 ++- .../core/src/browser/widgets/event-utils.ts | 70 +++++++++++++++++++ 2 files changed, 79 insertions(+), 2 deletions(-) create mode 100644 packages/core/src/browser/widgets/event-utils.ts diff --git a/packages/core/src/browser/tree/tree-widget.tsx b/packages/core/src/browser/tree/tree-widget.tsx index 75a8ba539e3eb..c1c2f150b8879 100644 --- a/packages/core/src/browser/tree/tree-widget.tsx +++ b/packages/core/src/browser/tree/tree-widget.tsx @@ -39,6 +39,7 @@ import { TreeWidgetSelection } from './tree-widget-selection'; import { MaybePromise } from '../../common/types'; import { LabelProvider } from '../label-provider'; import { CorePreferences } from '../core-preferences'; +import { createClickEventHandler } from '../widgets/event-utils'; const debounce = require('lodash.debounce'); @@ -886,11 +887,17 @@ export class TreeWidget extends ReactWidget implements StatefulWidget { protected createNodeAttributes(node: TreeNode, props: NodeProps): React.Attributes & React.HTMLAttributes { const className = this.createNodeClassNames(node, props).join(' '); const style = this.createNodeStyle(node, props); + const clickListener = createClickEventHandler>( + event => this.handleClickEvent(node, event), + event => this.handleDblClickEvent(node, event), + // This component can't use invokeSingle because running the handler causes the tree to refresh, replacing the listener and its closure + { timeout: 250, invokeSingle: false } + ); return { className, style, - onClick: event => this.handleClickEvent(node, event), - onDoubleClick: event => this.handleDblClickEvent(node, event), + onClick: clickListener, + onDoubleClick: clickListener, onContextMenu: event => this.handleContextMenuEvent(node, event) }; } diff --git a/packages/core/src/browser/widgets/event-utils.ts b/packages/core/src/browser/widgets/event-utils.ts new file mode 100644 index 0000000000000..e1ec6b0a1b183 --- /dev/null +++ b/packages/core/src/browser/widgets/event-utils.ts @@ -0,0 +1,70 @@ +/******************************************************************************** + * Copyright (C) 2021 Ericsson and others. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License v. 2.0 which is available at + * http://www.eclipse.org/legal/epl-2.0. + * + * This Source Code may also be made available under the following Secondary + * Licenses when the conditions for such availability set forth in the Eclipse + * Public License v. 2.0 are satisfied: GNU General Public License, version 2 + * with the GNU Classpath Exception which is available at + * https://www.gnu.org/software/classpath/license.html. + * + * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 + ********************************************************************************/ + +export interface Handler { (stimulus: T): unknown; } +export const isReactEvent = (event?: object): event is { persist(): unknown } => !!event && 'persist' in event; +export interface MinimalEvent { + type: string; + __superseded?: boolean; +} +export interface ClickHandlerOptions { + /** + * Time in milliseconds to wait for a second click. + */ + timeout: number; + /** + * If true, the single-click handler will be invoked immediately on the first click. + */ + invokeSingle?: boolean; +} + +/** + * @returns a single handler that should be applied to be both 'click' and 'dblclick' events for a given element. + * If the user clicks once in the interval specified, the single-click handler will be invoked. + * If the user clicks twice in the interval, *only* the double-click handler will be invoked. + */ +export function createClickEventHandler( + singleClickHandler: Handler, + doubleClickHandler: Handler, + options: ClickHandlerOptions, +): Handler { + const { timeout, invokeSingle } = options; + let deferredEvent: T | undefined; + return async (event: T): Promise => { + if (!deferredEvent && event.type === 'click') { + deferredEvent = event; + if (isReactEvent(event)) { + event.persist(); + } + if (invokeSingle) { + singleClickHandler(event); + } + await new Promise(resolve => setTimeout(resolve, timeout)); + if (!event.__superseded) { // No double click has cleaned up the old deferred event. + deferredEvent = undefined; + if (!invokeSingle) { // We haven't run it yet. + singleClickHandler(event); + } + } + } else if (event.type === 'dblclick') { + if (deferredEvent) { + deferredEvent.__superseded = true; + deferredEvent = undefined; + } + doubleClickHandler(event); + } + }; +} From 280ef7f6b676f0f883d569ffc51363a96fb39e38 Mon Sep 17 00:00:00 2001 From: Colin Grant Date: Tue, 8 Jun 2021 11:44:52 -0500 Subject: [PATCH 2/3] Maybe invoke single on double? Signed-off-by: Colin Grant --- .../core/src/browser/tree/tree-widget.tsx | 2 +- .../core/src/browser/widgets/event-utils.ts | 34 +++++++++++++------ 2 files changed, 25 insertions(+), 11 deletions(-) diff --git a/packages/core/src/browser/tree/tree-widget.tsx b/packages/core/src/browser/tree/tree-widget.tsx index c1c2f150b8879..b7d6c84755632 100644 --- a/packages/core/src/browser/tree/tree-widget.tsx +++ b/packages/core/src/browser/tree/tree-widget.tsx @@ -891,7 +891,7 @@ export class TreeWidget extends ReactWidget implements StatefulWidget { event => this.handleClickEvent(node, event), event => this.handleDblClickEvent(node, event), // This component can't use invokeSingle because running the handler causes the tree to refresh, replacing the listener and its closure - { timeout: 250, invokeSingle: false } + { timeout: 250, invokeImmediately: false } ); return { className, diff --git a/packages/core/src/browser/widgets/event-utils.ts b/packages/core/src/browser/widgets/event-utils.ts index e1ec6b0a1b183..7996fc5f76e7e 100644 --- a/packages/core/src/browser/widgets/event-utils.ts +++ b/packages/core/src/browser/widgets/event-utils.ts @@ -26,9 +26,16 @@ export interface ClickHandlerOptions { */ timeout: number; /** - * If true, the single-click handler will be invoked immediately on the first click. + * If `true`, the single-click handler will be invoked immediately on the first click. If true, `invokeSingleOnDouble` is ignored. + * Use this option with caution: if the single-click handler triggers a rerender, it may invalidate or ignore the state of the handlers + * created here. */ - invokeSingle?: boolean; + invokeImmediately?: boolean; + /** + * If `true`, the single click handler will be invoked and awaited before the double-click handler is invoked. + * Ignored if `invokeImmediately` is `true`. + */ + invokeSingleOnDouble?: boolean; } /** @@ -41,28 +48,35 @@ export function createClickEventHandler( doubleClickHandler: Handler, options: ClickHandlerOptions, ): Handler { - const { timeout, invokeSingle } = options; + const { timeout, invokeImmediately, invokeSingleOnDouble } = options; + const shouldInvokeSingleOnDouble = invokeSingleOnDouble && !invokeImmediately; let deferredEvent: T | undefined; return async (event: T): Promise => { + if (isReactEvent(event)) { + event.persist(); + } if (!deferredEvent && event.type === 'click') { deferredEvent = event; - if (isReactEvent(event)) { - event.persist(); - } - if (invokeSingle) { + if (invokeImmediately) { singleClickHandler(event); } await new Promise(resolve => setTimeout(resolve, timeout)); - if (!event.__superseded) { // No double click has cleaned up the old deferred event. + if (!event.__superseded) { // No double click has occurred. deferredEvent = undefined; - if (!invokeSingle) { // We haven't run it yet. + if (!invokeImmediately) { // We haven't run it yet. singleClickHandler(event); } } } else if (event.type === 'dblclick') { if (deferredEvent) { deferredEvent.__superseded = true; - deferredEvent = undefined; + if (shouldInvokeSingleOnDouble) { + const eventToHandle = deferredEvent; + deferredEvent = undefined; // Clear state immediately in case of triple click. + try { + await singleClickHandler(eventToHandle); + } catch { } + } } doubleClickHandler(event); } From 0455dff89678bad8286b356e1244248875b2ace1 Mon Sep 17 00:00:00 2001 From: Colin Grant Date: Thu, 26 Aug 2021 01:06:31 +0200 Subject: [PATCH 3/3] Class instead of closure Signed-off-by: Colin Grant --- packages/core/src/browser/core-preferences.ts | 9 +- .../browser/frontend-application-module.ts | 8 ++ .../core/src/browser/tree/tree-widget.tsx | 22 +-- .../core/src/browser/widgets/event-utils.ts | 129 +++++++++++------- 4 files changed, 106 insertions(+), 62 deletions(-) diff --git a/packages/core/src/browser/core-preferences.ts b/packages/core/src/browser/core-preferences.ts index e69a7784a2b15..5be82a0b5fcd8 100644 --- a/packages/core/src/browser/core-preferences.ts +++ b/packages/core/src/browser/core-preferences.ts @@ -97,11 +97,19 @@ export const corePreferenceSchema: PreferenceSchema = { default: 'code', description: 'Whether to interpret keypresses by the `code` of the physical key, or by the `keyCode` provided by the OS.' }, + 'application.clickTime': { + type: 'number', + minimum: 0, + default: 500, // This is Windows' default. + description: 'The number of milliseconds within which a second click should trigger a double-click action' + } } }; export interface CoreConfiguration { 'application.confirmExit': 'never' | 'ifRequired' | 'always'; + 'application.clickTime': number; + 'files.encoding': string 'keyboard.dispatch': 'code' | 'keyCode'; 'workbench.list.openMode': 'singleClick' | 'doubleClick'; 'workbench.commandPalette.history': number; @@ -110,7 +118,6 @@ export interface CoreConfiguration { 'workbench.colorTheme': string; 'workbench.iconTheme': string | null; 'workbench.silentNotifications': boolean; - 'files.encoding': string 'workbench.tree.renderIndentGuides': 'onHover' | 'none' | 'always'; } diff --git a/packages/core/src/browser/frontend-application-module.ts b/packages/core/src/browser/frontend-application-module.ts index b868bbacc4791..c330b752c1430 100644 --- a/packages/core/src/browser/frontend-application-module.ts +++ b/packages/core/src/browser/frontend-application-module.ts @@ -104,6 +104,7 @@ import { } from './quick-input'; import { QuickAccessContribution } from './quick-input/quick-access'; import { QuickCommandService } from './quick-input/quick-command-service'; +import { ClickEventHandler, ClickEventHandlerFactory, ClickEventHandlerOptions } from './widgets/event-utils'; export { bindResourceProvider, bindMessageService, bindPreferenceService }; @@ -353,4 +354,11 @@ export const frontendApplicationModule = new ContainerModule((bind, unbind, isBo bind(CredentialsService).to(CredentialsServiceImpl); bind(ContributionFilterRegistry).to(ContributionFilterRegistryImpl).inSingletonScope(); + + bind(ClickEventHandler).toSelf(); + bind(ClickEventHandlerFactory).toFactory(({ container }) => (options: ClickEventHandlerOptions) => { + const child = container.createChild(); + child.bind(ClickEventHandlerOptions).toConstantValue(options); + return child.get(ClickEventHandler); + }); }); diff --git a/packages/core/src/browser/tree/tree-widget.tsx b/packages/core/src/browser/tree/tree-widget.tsx index b7d6c84755632..293571b0b37f9 100644 --- a/packages/core/src/browser/tree/tree-widget.tsx +++ b/packages/core/src/browser/tree/tree-widget.tsx @@ -39,7 +39,7 @@ import { TreeWidgetSelection } from './tree-widget-selection'; import { MaybePromise } from '../../common/types'; import { LabelProvider } from '../label-provider'; import { CorePreferences } from '../core-preferences'; -import { createClickEventHandler } from '../widgets/event-utils'; +import { ClickEventHandler, ClickEventHandlerFactory } from '../widgets/event-utils'; const debounce = require('lodash.debounce'); @@ -148,7 +148,6 @@ export namespace TreeWidget { */ readonly shiftKey: boolean; } - } @injectable() @@ -156,6 +155,7 @@ export class TreeWidget extends ReactWidget implements StatefulWidget { protected searchBox: SearchBox; protected searchHighlights: Map; + protected nodeClickEventHandler: ClickEventHandler; @inject(TreeDecoratorService) protected readonly decoratorService: TreeDecoratorService; @@ -175,6 +175,8 @@ export class TreeWidget extends ReactWidget implements StatefulWidget { @inject(CorePreferences) protected readonly corePreferences: CorePreferences; + @inject(ClickEventHandlerFactory) protected readonly clickEventHandlerFactory: ClickEventHandlerFactory; + protected shouldScrollToRow = true; constructor( @@ -238,7 +240,14 @@ export class TreeWidget extends ReactWidget implements StatefulWidget { }), ]); } + this.nodeClickEventHandler = this.clickEventHandlerFactory({ + actions: [ + (event: React.MouseEvent, node: TreeNode) => this.handleClickEvent(node, event), + (event: React.MouseEvent, node: TreeNode) => this.handleDblClickEvent(node, event), + ] + }); this.toDispose.pushAll([ + this.nodeClickEventHandler, this.model, this.model.onChanged(() => this.updateRows()), this.model.onSelectionChanged(() => this.updateScrollToRow({ resize: false })), @@ -887,17 +896,10 @@ export class TreeWidget extends ReactWidget implements StatefulWidget { protected createNodeAttributes(node: TreeNode, props: NodeProps): React.Attributes & React.HTMLAttributes { const className = this.createNodeClassNames(node, props).join(' '); const style = this.createNodeStyle(node, props); - const clickListener = createClickEventHandler>( - event => this.handleClickEvent(node, event), - event => this.handleDblClickEvent(node, event), - // This component can't use invokeSingle because running the handler causes the tree to refresh, replacing the listener and its closure - { timeout: 250, invokeImmediately: false } - ); return { className, style, - onClick: clickListener, - onDoubleClick: clickListener, + onClick: event => this.nodeClickEventHandler.invoke(event, node), onContextMenu: event => this.handleContextMenuEvent(node, event) }; } diff --git a/packages/core/src/browser/widgets/event-utils.ts b/packages/core/src/browser/widgets/event-utils.ts index 7996fc5f76e7e..935f0f5643a1d 100644 --- a/packages/core/src/browser/widgets/event-utils.ts +++ b/packages/core/src/browser/widgets/event-utils.ts @@ -14,71 +14,98 @@ * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 ********************************************************************************/ -export interface Handler { (stimulus: T): unknown; } -export const isReactEvent = (event?: object): event is { persist(): unknown } => !!event && 'persist' in event; -export interface MinimalEvent { - type: string; - __superseded?: boolean; +import { Disposable } from '../../common'; +import { inject, injectable } from 'inversify'; +import * as React from 'react'; +import { CorePreferences } from '../core-preferences'; +import { wait } from '../../common/promise-util'; + +export const isReactEvent = (event: MouseEvent | React.MouseEvent): event is React.MouseEvent => 'nativeEvent' in event; + +interface MouseEventHandlerPlus { + (event: T, ...additionalArgs: unknown[]): unknown; } -export interface ClickHandlerOptions { + +export interface ClickEventHandlerOptions { /** - * Time in milliseconds to wait for a second click. + * A function to invoke on every click, regardless of other conditions. */ - timeout: number; + immediateAction?: MouseEventHandlerPlus; /** - * If `true`, the single-click handler will be invoked immediately on the first click. If true, `invokeSingleOnDouble` is ignored. - * Use this option with caution: if the single-click handler triggers a rerender, it may invalidate or ignore the state of the handlers - * created here. + * Functions to invoke on a given number of clicks, and no more. + * E.g. the action at index 0 will be invoked on a single click if no additional click + * comes within a set interval. */ - invokeImmediately?: boolean; + actions: MouseEventHandlerPlus[]; +} + +export const ClickEventHandlerOptions = Symbol('ClickEventHandlerOptions'); +export interface ClickEventHandlerFactory { + (options: ClickEventHandlerOptions): ClickEventHandler; +} +export const ClickEventHandlerFactory = Symbol('ClickEventHandlerFactory'); + +@injectable() +export class ClickEventHandler implements Disposable { + + @inject(CorePreferences) readonly preferences: CorePreferences; + @inject(ClickEventHandlerOptions) protected readonly options: ClickEventHandlerOptions; + + protected disposed = false; + /** - * If `true`, the single click handler will be invoked and awaited before the double-click handler is invoked. - * Ignored if `invokeImmediately` is `true`. + * Setting `.canceled` to true will prevent the handler from running. + * Calling `.run()` will invoke the handler immediately and prevent future invocations. */ - invokeSingleOnDouble?: boolean; -} + protected queuedInvocation: { event: T, canceled: boolean, run: () => unknown } | undefined; -/** - * @returns a single handler that should be applied to be both 'click' and 'dblclick' events for a given element. - * If the user clicks once in the interval specified, the single-click handler will be invoked. - * If the user clicks twice in the interval, *only* the double-click handler will be invoked. - */ -export function createClickEventHandler( - singleClickHandler: Handler, - doubleClickHandler: Handler, - options: ClickHandlerOptions, -): Handler { - const { timeout, invokeImmediately, invokeSingleOnDouble } = options; - const shouldInvokeSingleOnDouble = invokeSingleOnDouble && !invokeImmediately; - let deferredEvent: T | undefined; - return async (event: T): Promise => { + async invoke(event: T, ...additionalArguments: unknown[]): Promise { + if (this.disposed) { + return; + } if (isReactEvent(event)) { event.persist(); } - if (!deferredEvent && event.type === 'click') { - deferredEvent = event; - if (invokeImmediately) { - singleClickHandler(event); + const { immediateAction, actions } = this.options; + if (immediateAction) { + immediateAction(event, ...additionalArguments); + } + const { detail } = event; + const handler = actions[detail - 1]; + if (!!handler) { + if (this.queuedInvocation && detail > this.queuedInvocation.event.detail) { // Click is on same thing as before. Cancel that invocation. + this.queuedInvocation.canceled = true; + } else if (this.queuedInvocation) { // Clicking somewhere else or OS timer has run out. Run handler for last invocation immediately. + this.queuedInvocation.run(); } - await new Promise(resolve => setTimeout(resolve, timeout)); - if (!event.__superseded) { // No double click has occurred. - deferredEvent = undefined; - if (!invokeImmediately) { // We haven't run it yet. - singleClickHandler(event); + const thisCall = { + event, + canceled: false, + run(): void { + if (!this.canceled) { + this.canceled = true; + handler(event, ...additionalArguments); + } } + }; + this.queuedInvocation = thisCall; + // If detail == actions.length, it's the last defined handler. + // In that case, we run it immediately. If >, we don't reach this code. + if (detail < actions.length) { + await wait(this.preferences.get('application.clickTime', 500)); } - } else if (event.type === 'dblclick') { - if (deferredEvent) { - deferredEvent.__superseded = true; - if (shouldInvokeSingleOnDouble) { - const eventToHandle = deferredEvent; - deferredEvent = undefined; // Clear state immediately in case of triple click. - try { - await singleClickHandler(eventToHandle); - } catch { } - } + thisCall.run(); + if (this.queuedInvocation === thisCall) { + this.queuedInvocation = undefined; } - doubleClickHandler(event); } - }; + } + + dispose(): void { + this.disposed = true; + if (this.queuedInvocation) { + this.queuedInvocation.canceled = true; + this.queuedInvocation = undefined; + } + } }