From e53fb7c1c5544d772674416dae793d73dabde4b4 Mon Sep 17 00:00:00 2001 From: Erik Pasternak Date: Wed, 28 May 2025 12:56:43 -0700 Subject: [PATCH 1/2] Cleanup and fixes to cut/copy/paste --- src/actions/clipboard.ts | 186 +++++++++++++-------------------------- 1 file changed, 62 insertions(+), 124 deletions(-) diff --git a/src/actions/clipboard.ts b/src/actions/clipboard.ts index f89ae3b1..0c0780f2 100644 --- a/src/actions/clipboard.ts +++ b/src/actions/clipboard.ts @@ -7,21 +7,15 @@ import { ContextMenuRegistry, ShortcutRegistry, - ICopyData, isCopyable, - isDeletable, - isDraggable, Msg, ShortcutItems, - Flyout, - getMainWorkspace, + WorkspaceSvg, } from 'blockly'; import * as Constants from '../constants'; -import {WorkspaceSvg} from 'blockly'; import {Navigation} from '../navigation'; import {getShortActionShortcut} from '../shortcut_formatting'; import {clearPasteHints, showCopiedHint, showCutHint} from '../hints'; -import {IFocusableNode} from 'blockly/core'; /** * Weight for the first of these three items in the context menu. @@ -31,29 +25,18 @@ import {IFocusableNode} from 'blockly/core'; */ const BASE_WEIGHT = 12; -/** Type of the callback function for keyboard shortcuts. */ -type ShortcutCallback = ( - workspace: WorkspaceSvg, - e: Event, - shortcut: ShortcutRegistry.KeyboardShortcut, - scope: ContextMenuRegistry.Scope, -) => boolean; - /** * Logic and state for cut/copy/paste actions as both keyboard shortcuts * and context menu items. * In the long term, this will likely merge with the clipboard code in core. */ export class Clipboard { - /** Data copied by the copy or cut keyboard shortcuts. */ - private copyData: ICopyData | null = null; - /** The workspace a copy or cut keyboard shortcut happened in. */ private copyWorkspace: WorkspaceSvg | null = null; - private oldCutCallback: ShortcutCallback | undefined; - private oldCopyCallback: ShortcutCallback | undefined; - private oldPasteCallback: ShortcutCallback | undefined; + private oldCutShortcut: ShortcutRegistry.KeyboardShortcut | undefined; + private oldCopyShortcut: ShortcutRegistry.KeyboardShortcut | undefined; + private oldPasteShortcut: ShortcutRegistry.KeyboardShortcut | undefined; constructor(private navigation: Navigation) {} @@ -92,20 +75,18 @@ export class Clipboard { * Identical to the one in core but adds a toast after successful cut. */ private registerCutShortcut() { - const oldCutShortcut = + this.oldCutShortcut = ShortcutRegistry.registry.getRegistry()[ShortcutItems.names.CUT]; - if (!oldCutShortcut) + if (!this.oldCutShortcut) throw new Error('No cut keyboard shortcut registered initially'); - this.oldCutCallback = oldCutShortcut.callback; - const cutShortcut: ShortcutRegistry.KeyboardShortcut = { name: Constants.SHORTCUT_NAMES.CUT, - preconditionFn: oldCutShortcut.preconditionFn, + preconditionFn: this.oldCutShortcut.preconditionFn, callback: this.cutCallback.bind(this), // The registry gives back keycodes as an object instead of an array // See https://github.com/google/blockly/issues/9008 - keyCodes: oldCutShortcut.keyCodes, + keyCodes: this.oldCutShortcut.keyCodes, allowCollision: false, }; @@ -127,7 +108,7 @@ export class Clipboard { '%1', getShortActionShortcut(Constants.SHORTCUT_NAMES.CUT), ), - preconditionFn: (scope) => this.cutCopyPrecondition(scope), + preconditionFn: (scope) => this.cutPrecondition(scope), callback: (scope, menuOpenEvent) => { if (!isCopyable(scope.focusedNode)) return false; const ws = scope.focusedNode.workspace; @@ -142,31 +123,47 @@ export class Clipboard { ContextMenuRegistry.registry.register(cutAction); } - /** - * Precondition for cut and copy context menus. These are similar to the - * ones in core but they don't check if a gesture is in progress, - * because a gesture will always be in progress if the context menu - * is open. - * - * @param scope scope on which the menu was opened. - * @returns 'enabled', 'disabled', or 'hidden' as appropriate - */ - private cutCopyPrecondition(scope: ContextMenuRegistry.Scope): string { + private cutPrecondition(scope: ContextMenuRegistry.Scope): string { const focused = scope.focusedNode; + if (!focused || !isCopyable(focused)) return 'hidden'; + const workspace = focused.workspace; + if (!(workspace instanceof WorkspaceSvg)) return 'hidden'; + + if ( + this.oldCutShortcut?.preconditionFn && + this.oldCutShortcut.preconditionFn(workspace, scope) + ) { + return 'enabled'; + } + return 'disabled'; + } + + private copyPrecondition(scope: ContextMenuRegistry.Scope): string { + const focused = scope.focusedNode; if (!focused || !isCopyable(focused)) return 'hidden'; const workspace = focused.workspace; + if (!(workspace instanceof WorkspaceSvg)) return 'hidden'; + if ( - !workspace.isReadOnly() && - isDeletable(focused) && - focused.isDeletable() && - isDraggable(focused) && - focused.isMovable() && - !focused.workspace.isFlyout - ) + this.oldCopyShortcut?.preconditionFn && + this.oldCopyShortcut.preconditionFn(workspace, scope) + ) { return 'enabled'; + } + return 'disabled'; + } + + private pastePrecondition(scope: ContextMenuRegistry.Scope): string { + if (!this.copyWorkspace) return 'disabled'; + if ( + this.oldPasteShortcut?.preconditionFn && + this.oldPasteShortcut.preconditionFn(this.copyWorkspace, scope) + ) { + return 'enabled'; + } return 'disabled'; } @@ -189,9 +186,10 @@ export class Clipboard { scope: ContextMenuRegistry.Scope, ) { const didCut = - !!this.oldCutCallback && - this.oldCutCallback(workspace, e, shortcut, scope); + !!this.oldCutShortcut?.callback && + this.oldCutShortcut.callback(workspace, e, shortcut, scope); if (didCut) { + this.copyWorkspace = workspace; showCutHint(workspace); } return didCut; @@ -202,20 +200,18 @@ export class Clipboard { * Identical to the one in core but pops a toast after succesful copy. */ private registerCopyShortcut() { - const oldCopyShortcut = + this.oldCopyShortcut = ShortcutRegistry.registry.getRegistry()[ShortcutItems.names.COPY]; - if (!oldCopyShortcut) + if (!this.oldCopyShortcut) throw new Error('No copy keyboard shortcut registered initially'); - this.oldCopyCallback = oldCopyShortcut.callback; - const copyShortcut: ShortcutRegistry.KeyboardShortcut = { name: Constants.SHORTCUT_NAMES.COPY, - preconditionFn: oldCopyShortcut.preconditionFn, + preconditionFn: this.oldCopyShortcut.preconditionFn, callback: this.copyCallback.bind(this), // The registry gives back keycodes as an object instead of an array // See https://github.com/google/blockly/issues/9008 - keyCodes: oldCopyShortcut.keyCodes, + keyCodes: this.oldCopyShortcut.keyCodes, allowCollision: false, }; @@ -237,7 +233,7 @@ export class Clipboard { '%1', getShortActionShortcut(Constants.SHORTCUT_NAMES.COPY), ), - preconditionFn: (scope) => this.cutCopyPrecondition(scope), + preconditionFn: (scope) => this.copyPrecondition(scope), callback: (scope, menuOpenEvent) => { if (!isCopyable(scope.focusedNode)) return false; const ws = scope.focusedNode.workspace; @@ -271,9 +267,10 @@ export class Clipboard { scope: ContextMenuRegistry.Scope, ) { const didCopy = - !!this.oldCopyCallback && - this.oldCopyCallback(workspace, e, shortcut, scope); + !!this.oldCopyShortcut?.callback && + this.oldCopyShortcut.callback(workspace, e, shortcut, scope); if (didCopy) { + this.copyWorkspace = workspace; showCopiedHint(workspace); } return didCopy; @@ -284,38 +281,18 @@ export class Clipboard { * Identical to the one in core but clears any paste toasts after. */ private registerPasteShortcut() { - const oldPasteShortcut = + this.oldPasteShortcut = ShortcutRegistry.registry.getRegistry()[ShortcutItems.names.PASTE]; - if (!oldPasteShortcut) + if (!this.oldPasteShortcut) throw new Error('No paste keyboard shortcut registered initially'); - this.oldPasteCallback = oldPasteShortcut.callback; - const pasteShortcut: ShortcutRegistry.KeyboardShortcut = { name: Constants.SHORTCUT_NAMES.PASTE, - preconditionFn: ( - workspace: WorkspaceSvg, - scope: ContextMenuRegistry.Scope, - ) => { - // Don't use the workspace given as we don't want to paste in the flyout, for example - const pasteWorkspace = this.getPasteWorkspace(scope); - if (!pasteWorkspace || pasteWorkspace.isReadOnly()) return false; - return true; - }, - callback: ( - workspace: WorkspaceSvg, - e: Event, - shortcut: ShortcutRegistry.KeyboardShortcut, - scope: ContextMenuRegistry.Scope, - ) => { - // Don't use the workspace given as we don't want to paste in the flyout, for example - const pasteWorkspace = this.getPasteWorkspace(scope); - if (!pasteWorkspace) return false; - return this.pasteCallback(pasteWorkspace, e, shortcut, scope); - }, + preconditionFn: this.oldPasteShortcut.preconditionFn, + callback: this.pasteCallback.bind(this), // The registry gives back keycodes as an object instead of an array // See https://github.com/google/blockly/issues/9008 - keyCodes: oldPasteShortcut.keyCodes, + keyCodes: this.oldPasteShortcut.keyCodes, allowCollision: false, }; @@ -337,17 +314,9 @@ export class Clipboard { '%1', getShortActionShortcut(Constants.SHORTCUT_NAMES.PASTE), ), - preconditionFn: (scope: ContextMenuRegistry.Scope) => { - const workspace = this.getPasteWorkspace(scope); - if (!workspace) return 'hidden'; - - // Unfortunately, this will return enabled even if nothing is in the clipboard - // This is because the clipboard data is not actually exposed in core - // so there's no way to check - return workspace.isReadOnly() ? 'disabled' : 'enabled'; - }, + preconditionFn: (scope) => this.pastePrecondition(scope), callback: (scope: ContextMenuRegistry.Scope, menuOpenEvent: Event) => { - const workspace = this.getPasteWorkspace(scope); + const workspace = this.copyWorkspace; if (!workspace) return; return this.pasteCallback(workspace, menuOpenEvent, undefined, scope); }, @@ -358,37 +327,6 @@ export class Clipboard { ContextMenuRegistry.registry.register(pasteAction); } - /** - * Gets the workspace where something should be pasted. - * Tries to get the workspace the focusable item is on, - * or the target workspace if the focusable item is in a flyout, - * or falls back to the main workspace. - * - * @param scope scope from the action that initiated the paste - * @returns a workspace to paste into if possible, otherwise null - */ - private getPasteWorkspace(scope: ContextMenuRegistry.Scope) { - const focusTree = (scope.focusedNode as IFocusableNode).getFocusableTree(); - let workspace; - if (focusTree instanceof WorkspaceSvg) { - workspace = focusTree; - } else if (focusTree instanceof Flyout) { - // Seems like this case doesn't actually happen and a - // (flyout) Workspace is returned instead, but it's possible - workspace = focusTree.targetWorkspace; - } else { - // Give up and just paste in the main workspace - workspace = getMainWorkspace() as WorkspaceSvg; - } - - if (!workspace) return null; - // If we're trying to paste in a flyout, paste in the target workspace instead - if (workspace.isFlyout) - workspace = workspace.targetWorkspace as WorkspaceSvg; - - return workspace; - } - /** * The callback for the paste action. Uses the registered version of the paste callback * to perform the paste logic, then clears any toasts about pasting. @@ -408,8 +346,8 @@ export class Clipboard { scope: ContextMenuRegistry.Scope, ) { const didPaste = - !!this.oldPasteCallback && - this.oldPasteCallback(workspace, e, shortcut, scope); + !!this.oldPasteShortcut?.callback && + this.oldPasteShortcut.callback(workspace, e, shortcut, scope); // Clear the paste hints regardless of whether something was pasted // Some implementations of paste are async and we should clear the hint From cb2bf76a7da660f138d9a6f5d8118e0379f1ee08 Mon Sep 17 00:00:00 2001 From: Erik Pasternak Date: Wed, 28 May 2025 14:04:47 -0700 Subject: [PATCH 2/2] Add comments --- src/actions/clipboard.ts | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/actions/clipboard.ts b/src/actions/clipboard.ts index 0c0780f2..30c04af7 100644 --- a/src/actions/clipboard.ts +++ b/src/actions/clipboard.ts @@ -123,6 +123,13 @@ export class Clipboard { ContextMenuRegistry.registry.register(cutAction); } + /** + * Precondition function for the cut context menu. This wraps the core cut + * precondition to support context menus. + * + * @param scope scope of the shortcut or context menu item + * @returns 'enabled' if the node can be cut, 'disabled' otherwise. + */ private cutPrecondition(scope: ContextMenuRegistry.Scope): string { const focused = scope.focusedNode; if (!focused || !isCopyable(focused)) return 'hidden'; @@ -139,6 +146,13 @@ export class Clipboard { return 'disabled'; } + /** + * Precondition function for the copy context menu. This wraps the core copy + * precondition to support context menus. + * + * @param scope scope of the shortcut or context menu item + * @returns 'enabled' if the node can be copied, 'disabled' otherwise. + */ private copyPrecondition(scope: ContextMenuRegistry.Scope): string { const focused = scope.focusedNode; if (!focused || !isCopyable(focused)) return 'hidden'; @@ -155,6 +169,13 @@ export class Clipboard { return 'disabled'; } + /** + * Precondition function for the paste context menu. This wraps the core + * paste precondition to support context menus. + * + * @param scope scope of the shortcut or context menu item + * @returns 'enabled' if the node can be pasted, 'disabled' otherwise. + */ private pastePrecondition(scope: ContextMenuRegistry.Scope): string { if (!this.copyWorkspace) return 'disabled';