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

Merge pull request #9134 from RoboErikG/is-copyable

Author: RoboErikG

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 {
-  ICopyable,
-  ICopyData,
-  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 {ICopyData, isCopyable as isICopyable} from './interfaces/i_copyable.js';
+import {isDeletable as isIDeletable} from './interfaces/i_deletable.js';
+import {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
- * method, so we must use some other criteria to make the decision.
- * 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 .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.
+ * This will use the isCopyable method if the node implements it, otherwise
+ * it will fall back to checking if the node is deletable and draggable not
+ * considering the workspace's edit state.
  *
  * @param focused The focused object.
  */
-function isCopyable(
-  focused: IFocusableNode,
-): focused is ICopyable<ICopyData> & IDeletable & IDraggable {
-  if (!(focused instanceof BlockSvg)) return false;
-  return (
-    isICopyable(focused) &&
-    isIDeletable(focused) &&
-    focused.isOwnDeletable() &&
-    isDraggable(focused) &&
-    focused.isOwnMovable()
-  );
+function isCopyable(focused: IFocusableNode): boolean {
+  if (!isICopyable(focused) || !isIDeletable(focused) || !isDraggable(focused))
+    return false;
+  if (focused.isCopyable) {
+    return focused.isCopyable();
+  } else if (
+    focused instanceof BlockSvg ||
+    focused instanceof RenderedWorkspaceComment
+  ) {
+    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
- * method, so we must use some other criteria to make the decision.
- * 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.
+ * This will check if the node can be both copied and deleted in its current
+ * workspace.
  *
  * @param focused The focused object.
  */
 function isCuttable(focused: IFocusableNode): boolean {
-  if (!(focused instanceof BlockSvg)) return false;
-  return (
-    isICopyable(focused) &&
-    isIDeletable(focused) &&
-    focused.isDeletable() &&
-    isDraggable(focused) &&
-    focused.isMovable()
-  );
+  return isCopyable(focused) && isIDeletable(focused) && focused.isDeletable();
 }
 
 /**
@@ -185,15 +148,13 @@ 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
         : workspace;
       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;
-      let targetWorkspace: WorkspaceSvg | null =
-        focused.workspace instanceof WorkspaceSvg
-          ? focused.workspace
-          : workspace;
-      targetWorkspace = targetWorkspace.isFlyout
-        ? targetWorkspace.targetWorkspace
-        : targetWorkspace;
+      if (!focused || !isICopyable(focused) || !isCopyable(focused))
+        return false;
+      const targetWorkspace = workspace.isFlyout
+        ? workspace.targetWorkspace
+        : workspace;
       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) &&
-        focused.isDeletable() &&
-        isICopyable(focused)
-      ) {
-        copyData = focused.toCopyData();
-        copyWorkspace = workspace;
-        copyCoords = isDraggable(focused)
-          ? focused.getRelativeToSurfaceXY()
-          : null;
+      } else if (isIDeletable(focused)) {
         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,32 +273,32 @@ 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);
 
       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],
   };