fix: Make cut/copy/paste work as expected (#556)

* Cleanup and fixes to cut/copy/paste

* Add comments

Author: RoboErikG

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;
@@ -143,30 +124,67 @@ export class Clipboard {
   }
 
   /**
-   * 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.
+   * Precondition function for the cut context menu. This wraps the core cut
+   * precondition to support context menus.
    *
-   * @param scope scope on which the menu was opened.
-   * @returns 'enabled', 'disabled', or 'hidden' as appropriate
+   * @param scope scope of the shortcut or context menu item
+   * @returns 'enabled' if the node can be cut, 'disabled' otherwise.
    */
-  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';
+  }
 
+  /**
+   * 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';
 
     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';
+  }
 
+  /**
+   * 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';
+
+    if (
+      this.oldPasteShortcut?.preconditionFn &&
+      this.oldPasteShortcut.preconditionFn(this.copyWorkspace, scope)
+    ) {
+      return 'enabled';
+    }
     return 'disabled';
   }
 
@@ -189,9 +207,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 +221,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 +254,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 +288,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 +302,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 +335,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 +348,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 +367,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