fix: Add isCopyable to the ICopyable interface and use it for cut/copy preconditions

Merge pull request #9134 from RoboErikG/is-copyable

core/block.ts

@@ -791,6 +791,7 @@ export class Block {
   isDeletable(): boolean {
     return (
       this.deletable &&
+      !this.isInFlyout &&
       !this.shadow &&
       !this.isDeadOrDying() &&
       !this.workspace.isReadOnly()
@@ -824,6 +825,7 @@ export class Block {
   isMovable(): boolean {
     return (
       this.movable &&
+      !this.isInFlyout &&
       !this.shadow &&
       !this.isDeadOrDying() &&
       !this.workspace.isReadOnly()

core/block_svg.ts

@@ -1721,6 +1721,11 @@ export class BlockSvg
     this.dragStrategy = dragStrategy;
   }
 
+  /** Returns whether this block is copyable or not. */
+  isCopyable(): boolean {
+    return this.isOwnDeletable() && this.isOwnMovable();
+  }
+
   /** Returns whether this block is movable or not. */
   override isMovable(): boolean {
     return this.dragStrategy.isMovable();

core/comments/rendered_workspace_comment.ts

@@ -244,6 +244,11 @@ export class RenderedWorkspaceComment
     }
   }
 
+  /** Returns whether this comment is copyable or not */
+  isCopyable(): boolean {
+    return this.isOwnMovable() && this.isOwnDeletable();
+  }
+
   /** Returns whether this comment is movable or not. */
   isMovable(): boolean {
     return this.dragStrategy.isMovable();

core/comments/workspace_comment.ts

@@ -165,7 +165,11 @@ export class WorkspaceComment {
    * workspace is read-only.
    */
   isMovable() {
-    return this.isOwnMovable() && !this.workspace.isReadOnly();
+    return (
+      this.isOwnMovable() &&
+      !this.workspace.isReadOnly() &&
+      !this.workspace.isFlyout
+    );
   }
 
   /**
@@ -189,7 +193,8 @@ export class WorkspaceComment {
     return (
       this.isOwnDeletable() &&
       !this.isDeadOrDying() &&
-      !this.workspace.isReadOnly()
+      !this.workspace.isReadOnly() &&
+      !this.workspace.isFlyout
     );
   }
 

core/interfaces/i_copyable.ts

@@ -15,6 +15,14 @@ export interface ICopyable<T extends ICopyData> extends ISelectable {
    * @returns Copy metadata.
    */
   toCopyData(): T | null;
+
+  /**
+   * Whether this instance is currently copyable. The standard implementation
+   * is to return true if isOwnDeletable and isOwnMovable return true.
+   *
+   * @returns True if it can currently be copied.
+   */
+  isCopyable?(): boolean;
 }
 
 export namespace ICopyable {
@@ -25,7 +33,7 @@ export namespace ICopyable {
 
 export type ICopyData = ICopyable.ICopyData;
 
-/** @returns true if the given object is copyable. */
+/** @returns true if the given object is an ICopyable. */
 export function isCopyable(obj: any): obj is ICopyable<ICopyData> {
   return obj.toCopyData !== undefined;
 }

core/shortcut_items.ts

@@ -8,19 +8,13 @@
 
 import {BlockSvg} from './block_svg.js';
 import * as clipboard from './clipboard.js';
+import {RenderedWorkspaceComment} from './comments.js';
 import * as eventUtils from './events/utils.js';
 import {getFocusManager} from './focus_manager.js';
 import {Gesture} from './gesture.js';
-import {
+import {ICopyData, isCopyable as isICopyable} from './interfaces/i_copyable.js';
-  ICopyable,
+import {isDeletable as isIDeletable} from './interfaces/i_deletable.js';
-  ICopyData,
+import {isDraggable} from './interfaces/i_draggable.js';
-  isCopyable as isICopyable,
-} from './interfaces/i_copyable.js';
-import {
-  IDeletable,
-  isDeletable as isIDeletable,
-} from './interfaces/i_deletable.js';
-import {IDraggable, isDraggable} from './interfaces/i_draggable.js';
 import {IFocusableNode} from './interfaces/i_focusable_node.js';
 import {KeyboardShortcut, ShortcutRegistry} from './shortcut_registry.js';
 import {Coordinate} from './utils/coordinate.js';
@@ -100,74 +94,43 @@ export function registerDelete() {
 }
 
 let copyData: ICopyData | null = null;
-let copyWorkspace: WorkspaceSvg | null = null;
 let copyCoords: Coordinate | null = null;
 
 /**
  * Determine if a focusable node can be copied.
  *
- * Unfortunately the ICopyable interface doesn't include an isCopyable
+ * This will use the isCopyable method if the node implements it, otherwise
- * method, so we must use some other criteria to make the decision.
+ * it will fall back to checking if the node is deletable and draggable not
- * Specifically,
+ * considering the workspace's edit state.
- *
- * - It must be an ICopyable.
- * - So that a pasted copy can be manipluated and/or disposed of, it
- *   must be both an IDraggable and an IDeletable.
- * - Additionally, both .isOwnMovable() and .isOwnDeletable() must return
- *   true (i.e., the copy could be moved and deleted).
- *
- * TODO(#9098): Revise these criteria.  The latter criteria prevents
- * shadow blocks from being copied; additionally, there are likely to
- * be other circumstances were it is desirable to allow movable /
- * copyable copies of a currently-unmovable / -copyable block to be
- * made.
  *
  * @param focused The focused object.
  */
-function isCopyable(
+function isCopyable(focused: IFocusableNode): boolean {
-  focused: IFocusableNode,
+  if (!isICopyable(focused) || !isIDeletable(focused) || !isDraggable(focused))
-): focused is ICopyable<ICopyData> & IDeletable & IDraggable {
+    return false;
-  if (!(focused instanceof BlockSvg)) return false;
+  if (focused.isCopyable) {
-  return (
+    return focused.isCopyable();
-    isICopyable(focused) &&
+  } else if (
-    isIDeletable(focused) &&
+    focused instanceof BlockSvg ||
-    focused.isOwnDeletable() &&
+    focused instanceof RenderedWorkspaceComment
-    isDraggable(focused) &&
+  ) {
-    focused.isOwnMovable()
+    return focused.isOwnDeletable() && focused.isOwnMovable();
-  );
+  }
+  // This isn't a class Blockly knows about, so fall back to the stricter
+  // checks for deletable and movable.
+  return focused.isDeletable() && focused.isMovable();
 }
 
 /**
  * Determine if a focusable node can be cut.
  *
- * Unfortunately the ICopyable interface doesn't include an isCuttable
+ * This will check if the node can be both copied and deleted in its current
- * method, so we must use some other criteria to make the decision.
+ * workspace.
- * Specifically,
- *
- * - It must be an ICopyable.
- * - So that a pasted copy can be manipluated and/or disposed of, it
- *   must be both an IDraggable and an IDeletable.
- * - Additionally, both .isMovable() and .isDeletable() must return
- *   true (i.e., can currently be moved and deleted). This is the main
- *   difference with isCopyable.
- *
- * TODO(#9098): Revise these criteria.  The latter criteria prevents
- * shadow blocks from being copied; additionally, there are likely to
- * be other circumstances were it is desirable to allow movable /
- * copyable copies of a currently-unmovable / -copyable block to be
- * made.
  *
  * @param focused The focused object.
  */
 function isCuttable(focused: IFocusableNode): boolean {
-  if (!(focused instanceof BlockSvg)) return false;
+  return isCopyable(focused) && isIDeletable(focused) && focused.isDeletable();
-  return (
-    isICopyable(focused) &&
-    isIDeletable(focused) &&
-    focused.isDeletable() &&
-    isDraggable(focused) &&
-    focused.isMovable()
-  );
 }
 
 /**
@@ -185,7 +148,6 @@ export function registerCopy() {
     name: names.COPY,
     preconditionFn(workspace, scope) {
       const focused = scope.focusedNode;
-      if (!(focused instanceof BlockSvg)) return false;
 
       const targetWorkspace = workspace.isFlyout
         ? workspace.targetWorkspace
@@ -193,7 +155,6 @@ export function registerCopy() {
       return (
         !!focused &&
         !!targetWorkspace &&
-        !targetWorkspace.isReadOnly() &&
         !targetWorkspace.isDragging() &&
         !getFocusManager().ephemeralFocusTaken() &&
         isCopyable(focused)
@@ -205,21 +166,17 @@ export function registerCopy() {
       e.preventDefault();
 
       const focused = scope.focusedNode;
-      if (!focused || !isCopyable(focused)) return false;
+      if (!focused || !isICopyable(focused) || !isCopyable(focused))
-      let targetWorkspace: WorkspaceSvg | null =
+        return false;
-        focused.workspace instanceof WorkspaceSvg
+      const targetWorkspace = workspace.isFlyout
-          ? focused.workspace
+        ? workspace.targetWorkspace
-          : workspace;
+        : workspace;
-      targetWorkspace = targetWorkspace.isFlyout
-        ? targetWorkspace.targetWorkspace
-        : targetWorkspace;
       if (!targetWorkspace) return false;
 
       if (!focused.workspace.isFlyout) {
         targetWorkspace.hideChaff();
       }
       copyData = focused.toCopyData();
-      copyWorkspace = targetWorkspace;
       copyCoords =
         isDraggable(focused) && focused.workspace == targetWorkspace
           ? focused.getRelativeToSurfaceXY()
@@ -256,27 +213,20 @@ export function registerCut() {
     },
     callback(workspace, e, shortcut, scope) {
       const focused = scope.focusedNode;
+      if (!focused || !isCuttable(focused) || !isICopyable(focused)) {
+        return false;
+      }
+      copyData = focused.toCopyData();
+      copyCoords = isDraggable(focused)
+        ? focused.getRelativeToSurfaceXY()
+        : null;
 
       if (focused instanceof BlockSvg) {
-        copyData = focused.toCopyData();
-        copyWorkspace = workspace;
-        copyCoords = focused.getRelativeToSurfaceXY();
         focused.checkAndDelete();
-        return true;
+      } else if (isIDeletable(focused)) {
-      } else if (
-        isIDeletable(focused) &&
-        focused.isDeletable() &&
-        isICopyable(focused)
-      ) {
-        copyData = focused.toCopyData();
-        copyWorkspace = workspace;
-        copyCoords = isDraggable(focused)
-          ? focused.getRelativeToSurfaceXY()
-          : null;
         focused.dispose();
-        return true;
       }
-      return false;
+      return !!copyData;
     },
     keyCodes: [ctrlX, metaX],
   };
@@ -310,7 +260,11 @@ export function registerPaste() {
       );
     },
     callback(workspace: WorkspaceSvg, e: Event) {
-      if (!copyData || !copyWorkspace) return false;
+      if (!copyData) return false;
+      const targetWorkspace = workspace.isFlyout
+        ? workspace.targetWorkspace
+        : workspace;
+      if (!targetWorkspace || targetWorkspace.isReadOnly()) return false;
 
       if (e instanceof PointerEvent) {
         // The event that triggers a shortcut would conventionally be a KeyboardEvent.
@@ -319,19 +273,19 @@ export function registerPaste() {
         // at the mouse coordinates where the menu was opened, and this PointerEvent
         // is where the menu was opened.
         const mouseCoords = svgMath.screenToWsCoordinates(
-          copyWorkspace,
+          targetWorkspace,
           new Coordinate(e.clientX, e.clientY),
         );
-        return !!clipboard.paste(copyData, copyWorkspace, mouseCoords);
+        return !!clipboard.paste(copyData, targetWorkspace, mouseCoords);
       }
 
       if (!copyCoords) {
         // If we don't have location data about the original copyable, let the
         // paster determine position.
-        return !!clipboard.paste(copyData, copyWorkspace);
+        return !!clipboard.paste(copyData, targetWorkspace);
       }
 
-      const {left, top, width, height} = copyWorkspace
+      const {left, top, width, height} = targetWorkspace
         .getMetricsManager()
         .getViewMetrics(true);
       const viewportRect = new Rect(top, top + height, left, left + width);
@@ -339,12 +293,12 @@ export function registerPaste() {
       if (viewportRect.contains(copyCoords.x, copyCoords.y)) {
         // If the original copyable is inside the viewport, let the paster
         // determine position.
-        return !!clipboard.paste(copyData, copyWorkspace);
+        return !!clipboard.paste(copyData, targetWorkspace);
       }
 
       // Otherwise, paste in the middle of the viewport.
       const centerCoords = new Coordinate(left + width / 2, top + height / 2);
-      return !!clipboard.paste(copyData, copyWorkspace, centerCoords);
+      return !!clipboard.paste(copyData, targetWorkspace, centerCoords);
     },
     keyCodes: [ctrlV, metaV],
   };

tests/mocha/shortcut_items_test.js

@@ -47,6 +47,16 @@ suite('Keyboard Shortcut Items', function () {
       .returns(block.nextConnection);
   }
 
+  /**
+   * Creates a workspace comment and set it as the focused node.
+   * @param {Blockly.Workspace} workspace The workspace to create a new comment on.
+   */
+  function setSelectedComment(workspace) {
+    const comment = workspace.newComment();
+    sinon.stub(Blockly.getFocusManager(), 'getFocusedNode').returns(comment);
+    return comment;
+  }
+
   /**
    * Creates a test for not running keyDown events when the workspace is in read only mode.
    * @param {Object} keyEvent Mocked key down event. Use createKeyDownEvent.
@@ -173,12 +183,17 @@ suite('Keyboard Shortcut Items', function () {
         });
       });
     });
-    // Do not copy a block if a workspace is in readonly mode.
+    // Allow copying a block if a workspace is in readonly mode.
-    suite('Not called when readOnly is true', function () {
+    suite('Called when readOnly is true', function () {
       testCases.forEach(function (testCase) {
         const testCaseName = testCase[0];
         const keyEvent = testCase[1];
-        runReadOnlyTest(keyEvent, testCaseName);
+        test(testCaseName, function () {
+          this.workspace.setIsReadOnly(true);
+          this.injectionDiv.dispatchEvent(keyEvent);
+          sinon.assert.calledOnce(this.copySpy);
+          sinon.assert.calledOnce(this.hideChaffSpy);
+        });
       });
     });
     // Do not copy a block if a drag is in progress.
@@ -236,6 +251,152 @@ suite('Keyboard Shortcut Items', function () {
       sinon.assert.notCalled(this.copySpy);
       sinon.assert.notCalled(this.hideChaffSpy);
     });
+    // Copy a comment.
+    test('Workspace comment', function () {
+      testCases.forEach(function (testCase) {
+        const testCaseName = testCase[0];
+        const keyEvent = testCase[1];
+        test(testCaseName, function () {
+          Blockly.getFocusManager().getFocusedNode.restore();
+          this.comment = setSelectedComment(this.workspace);
+          this.copySpy = sinon.spy(this.comment, 'toCopyData');
+
+          this.injectionDiv.dispatchEvent(keyEvent);
+          sinon.assert.calledOnce(this.copySpy);
+          sinon.assert.calledOnce(this.hideChaffSpy);
+        });
+      });
+    });
+  });
+
+  suite('Cut', function () {
+    setup(function () {
+      this.block = setSelectedBlock(this.workspace);
+      this.copySpy = sinon.spy(this.block, 'toCopyData');
+      this.disposeSpy = sinon.spy(this.block, 'dispose');
+      this.hideChaffSpy = sinon.spy(
+        Blockly.WorkspaceSvg.prototype,
+        'hideChaff',
+      );
+    });
+    const testCases = [
+      [
+        'Control X',
+        createKeyDownEvent(Blockly.utils.KeyCodes.X, [
+          Blockly.utils.KeyCodes.CTRL,
+        ]),
+      ],
+      [
+        'Meta X',
+        createKeyDownEvent(Blockly.utils.KeyCodes.X, [
+          Blockly.utils.KeyCodes.META,
+        ]),
+      ],
+    ];
+    // Cut a block.
+    suite('Simple', function () {
+      testCases.forEach(function (testCase) {
+        const testCaseName = testCase[0];
+        const keyEvent = testCase[1];
+        test(testCaseName, function () {
+          this.injectionDiv.dispatchEvent(keyEvent);
+          sinon.assert.calledOnce(this.copySpy);
+          sinon.assert.calledOnce(this.disposeSpy);
+          sinon.assert.calledOnce(this.hideChaffSpy);
+        });
+      });
+    });
+    // Do not cut a block if a workspace is in readonly mode.
+    suite('Not called when readOnly is true', function () {
+      testCases.forEach(function (testCase) {
+        const testCaseName = testCase[0];
+        const keyEvent = testCase[1];
+        test(testCaseName, function () {
+          this.workspace.setIsReadOnly(true);
+          this.injectionDiv.dispatchEvent(keyEvent);
+          sinon.assert.notCalled(this.copySpy);
+          sinon.assert.notCalled(this.disposeSpy);
+          sinon.assert.notCalled(this.hideChaffSpy);
+        });
+      });
+    });
+    // Do not cut a block if a drag is in progress.
+    suite('Drag in progress', function () {
+      testCases.forEach(function (testCase) {
+        const testCaseName = testCase[0];
+        const keyEvent = testCase[1];
+        test(testCaseName, function () {
+          sinon.stub(this.workspace, 'isDragging').returns(true);
+          this.injectionDiv.dispatchEvent(keyEvent);
+          sinon.assert.notCalled(this.copySpy);
+          sinon.assert.notCalled(this.disposeSpy);
+          sinon.assert.notCalled(this.hideChaffSpy);
+        });
+      });
+    });
+    // Do not cut a block if is is not deletable.
+    suite('Block is not deletable', function () {
+      testCases.forEach(function (testCase) {
+        const testCaseName = testCase[0];
+        const keyEvent = testCase[1];
+        test(testCaseName, function () {
+          sinon
+            .stub(Blockly.common.getSelected(), 'isOwnDeletable')
+            .returns(false);
+          this.injectionDiv.dispatchEvent(keyEvent);
+          sinon.assert.notCalled(this.copySpy);
+          sinon.assert.notCalled(this.disposeSpy);
+          sinon.assert.notCalled(this.hideChaffSpy);
+        });
+      });
+    });
+    // Do not cut a block if it is not movable.
+    suite('Block is not movable', function () {
+      testCases.forEach(function (testCase) {
+        const testCaseName = testCase[0];
+        const keyEvent = testCase[1];
+        test(testCaseName, function () {
+          sinon
+            .stub(Blockly.common.getSelected(), 'isOwnMovable')
+            .returns(false);
+          this.injectionDiv.dispatchEvent(keyEvent);
+          sinon.assert.notCalled(this.copySpy);
+          sinon.assert.notCalled(this.disposeSpy);
+          sinon.assert.notCalled(this.hideChaffSpy);
+        });
+      });
+    });
+    test('Not called when connection is focused', function () {
+      // Restore the stub behavior called during setup
+      Blockly.getFocusManager().getFocusedNode.restore();
+
+      setSelectedConnection(this.workspace);
+      const event = createKeyDownEvent(Blockly.utils.KeyCodes.C, [
+        Blockly.utils.KeyCodes.CTRL,
+      ]);
+      this.injectionDiv.dispatchEvent(event);
+      sinon.assert.notCalled(this.copySpy);
+      sinon.assert.notCalled(this.disposeSpy);
+      sinon.assert.notCalled(this.hideChaffSpy);
+    });
+
+    // Cut a comment.
+    suite('Workspace comment', function () {
+      testCases.forEach(function (testCase) {
+        const testCaseName = testCase[0];
+        const keyEvent = testCase[1];
+        test(testCaseName, function () {
+          Blockly.getFocusManager().getFocusedNode.restore();
+          this.comment = setSelectedComment(this.workspace);
+          this.copySpy = sinon.spy(this.comment, 'toCopyData');
+          this.disposeSpy = sinon.spy(this.comment, 'dispose');
+
+          this.injectionDiv.dispatchEvent(keyEvent);
+          sinon.assert.calledOnce(this.copySpy);
+          sinon.assert.calledOnce(this.disposeSpy);
+        });
+      });
+    });
   });
 
   suite('Undo', function () {