From 9685498d219aa796a5e1a1bfafb3cf64834625e5 Mon Sep 17 00:00:00 2001 From: Erik Pasternak Date: Mon, 9 Jun 2025 15:13:43 -0700 Subject: [PATCH 01/10] Add isCopyable and isCuttable as optional methods on ICopyable --- core/block_svg.ts | 10 +++ core/comments/rendered_workspace_comment.ts | 10 +++ core/interfaces/i_copyable.ts | 16 ++++- core/shortcut_items.ts | 77 ++++++++------------- 4 files changed, 62 insertions(+), 51 deletions(-) diff --git a/core/block_svg.ts b/core/block_svg.ts index 8ea26e354..501be1b59 100644 --- a/core/block_svg.ts +++ b/core/block_svg.ts @@ -1721,6 +1721,16 @@ 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 cuttable or not. */ + isCuttable(): boolean { + return this.isDeletable() && this.isMovable(); + } + /** Returns whether this block is movable or not. */ override isMovable(): boolean { return this.dragStrategy.isMovable(); diff --git a/core/comments/rendered_workspace_comment.ts b/core/comments/rendered_workspace_comment.ts index 3a3d57a44..569905518 100644 --- a/core/comments/rendered_workspace_comment.ts +++ b/core/comments/rendered_workspace_comment.ts @@ -244,6 +244,16 @@ export class RenderedWorkspaceComment } } + /** Returns whether this comment is copyable or not */ + isCopyable(): boolean { + return this.isOwnMovable() && this.isOwnDeletable(); + } + + /** Returns whether this comment is cuttable or not */ + isCuttable(): boolean { + return this.isMovable() && this.isDeletable(); + } + /** Returns whether this comment is movable or not. */ isMovable(): boolean { return this.dragStrategy.isMovable(); diff --git a/core/interfaces/i_copyable.ts b/core/interfaces/i_copyable.ts index b653bd20a..4f5e4ab9a 100644 --- a/core/interfaces/i_copyable.ts +++ b/core/interfaces/i_copyable.ts @@ -15,6 +15,20 @@ export interface ICopyable extends ISelectable { * @returns Copy metadata. */ toCopyData(): T | null; + + /** + * Whether this instance is currently copyable. + * + * @returns True if it can currently be copied. + */ + isCopyable?(): boolean; + + /** + * Whether this instance is currently cuttable. + * + * @returns True if it can currently be cut. + */ + isCuttable?(): boolean; } export namespace ICopyable { @@ -25,7 +39,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 { return obj.toCopyData !== undefined; } diff --git a/core/shortcut_items.ts b/core/shortcut_items.ts index 161d5fceb..0da30de62 100644 --- a/core/shortcut_items.ts +++ b/core/shortcut_items.ts @@ -8,6 +8,7 @@ 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'; @@ -106,68 +107,44 @@ 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 & 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 !== undefined) { + 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 use the isCuttable method if the node implements it, otherwise + * it will fall back to checking if the node can be moved 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() - ); + if (!isICopyable(focused) || !isIDeletable(focused) || !isDraggable(focused)) + return false; + if (focused.isCuttable !== undefined) { + return focused.isCuttable(); + } + return focused.isMovable() && focused.isDeletable(); } /** From 46078369c2e61dd9040d5a8bb9c4598927409e96 Mon Sep 17 00:00:00 2001 From: Erik Pasternak Date: Mon, 9 Jun 2025 15:33:45 -0700 Subject: [PATCH 02/10] Fix build errors --- core/shortcut_items.ts | 41 ++++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/core/shortcut_items.ts b/core/shortcut_items.ts index 0da30de62..8d67321d7 100644 --- a/core/shortcut_items.ts +++ b/core/shortcut_items.ts @@ -8,20 +8,13 @@ import {BlockSvg} from './block_svg.js'; import * as clipboard from './clipboard.js'; -import { RenderedWorkspaceComment } from './comments.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'; @@ -182,17 +175,27 @@ 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 + if (!focused || !isICopyable(focused) || !isCopyable(focused)) + return false; + let targetWorkspace: WorkspaceSvg | null; + let hideChaff = false; + if (focused instanceof BlockSvg) { + hideChaff = !focused.workspace.isFlyout; + targetWorkspace = + focused.workspace instanceof WorkspaceSvg + ? focused.workspace + : workspace; + targetWorkspace = targetWorkspace.isFlyout + ? targetWorkspace.targetWorkspace + : targetWorkspace; + } else { + targetWorkspace = workspace.isFlyout + ? workspace.targetWorkspace : workspace; - targetWorkspace = targetWorkspace.isFlyout - ? targetWorkspace.targetWorkspace - : targetWorkspace; + } if (!targetWorkspace) return false; - if (!focused.workspace.isFlyout) { + if (hideChaff) { targetWorkspace.hideChaff(); } copyData = focused.toCopyData(); From e1441d5308b668f3c6e73b43a51e1a4176ce63f4 Mon Sep 17 00:00:00 2001 From: Erik Pasternak Date: Tue, 10 Jun 2025 11:09:12 -0700 Subject: [PATCH 03/10] Remove isCuttable api --- core/block_svg.ts | 5 ----- core/comments/rendered_workspace_comment.ts | 5 ----- core/interfaces/i_copyable.ts | 7 ------- core/shortcut_items.ts | 12 +++--------- 4 files changed, 3 insertions(+), 26 deletions(-) diff --git a/core/block_svg.ts b/core/block_svg.ts index 501be1b59..a30cc34ed 100644 --- a/core/block_svg.ts +++ b/core/block_svg.ts @@ -1726,11 +1726,6 @@ export class BlockSvg return this.isOwnDeletable() && this.isOwnMovable(); } - /** Returns whether this block is cuttable or not. */ - isCuttable(): boolean { - return this.isDeletable() && this.isMovable(); - } - /** Returns whether this block is movable or not. */ override isMovable(): boolean { return this.dragStrategy.isMovable(); diff --git a/core/comments/rendered_workspace_comment.ts b/core/comments/rendered_workspace_comment.ts index 569905518..42fb1fda4 100644 --- a/core/comments/rendered_workspace_comment.ts +++ b/core/comments/rendered_workspace_comment.ts @@ -249,11 +249,6 @@ export class RenderedWorkspaceComment return this.isOwnMovable() && this.isOwnDeletable(); } - /** Returns whether this comment is cuttable or not */ - isCuttable(): boolean { - return this.isMovable() && this.isDeletable(); - } - /** Returns whether this comment is movable or not. */ isMovable(): boolean { return this.dragStrategy.isMovable(); diff --git a/core/interfaces/i_copyable.ts b/core/interfaces/i_copyable.ts index 4f5e4ab9a..246dd4dd5 100644 --- a/core/interfaces/i_copyable.ts +++ b/core/interfaces/i_copyable.ts @@ -22,13 +22,6 @@ export interface ICopyable extends ISelectable { * @returns True if it can currently be copied. */ isCopyable?(): boolean; - - /** - * Whether this instance is currently cuttable. - * - * @returns True if it can currently be cut. - */ - isCuttable?(): boolean; } export namespace ICopyable { diff --git a/core/shortcut_items.ts b/core/shortcut_items.ts index 8d67321d7..302308fd6 100644 --- a/core/shortcut_items.ts +++ b/core/shortcut_items.ts @@ -125,19 +125,13 @@ function isCopyable(focused: IFocusableNode): boolean { /** * Determine if a focusable node can be cut. * - * This will use the isCuttable method if the node implements it, otherwise - * it will fall back to checking if the node can be moved and deleted in its - * current workspace. + * 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 (!isICopyable(focused) || !isIDeletable(focused) || !isDraggable(focused)) - return false; - if (focused.isCuttable !== undefined) { - return focused.isCuttable(); - } - return focused.isMovable() && focused.isDeletable(); + return isCopyable(focused) && isIDeletable(focused) && focused.isDeletable(); } /** From 1d4e531ebed00e1d3e210e2c1ab6c93244fb3d4e Mon Sep 17 00:00:00 2001 From: Erik Pasternak Date: Tue, 10 Jun 2025 11:24:42 -0700 Subject: [PATCH 04/10] Don't allow things in a flyout to be deleted or moved. --- core/block.ts | 2 ++ core/comments/workspace_comment.ts | 9 +++++++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/core/block.ts b/core/block.ts index 43bc6bbc5..9f7c11d4f 100644 --- a/core/block.ts +++ b/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() diff --git a/core/comments/workspace_comment.ts b/core/comments/workspace_comment.ts index 190efd64d..b5dc3023c 100644 --- a/core/comments/workspace_comment.ts +++ b/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 ); } From 428e4475bfce90b1bc5d050d95b4535f160a40cc Mon Sep 17 00:00:00 2001 From: Erik Pasternak Date: Tue, 10 Jun 2025 13:32:36 -0700 Subject: [PATCH 05/10] Simplify cut/copy logic --- core/shortcut_items.ts | 48 ++++++++++++------------------------------ 1 file changed, 14 insertions(+), 34 deletions(-) diff --git a/core/shortcut_items.ts b/core/shortcut_items.ts index 302308fd6..826cef285 100644 --- a/core/shortcut_items.ts +++ b/core/shortcut_items.ts @@ -149,7 +149,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 @@ -171,25 +170,12 @@ export function registerCopy() { const focused = scope.focusedNode; if (!focused || !isICopyable(focused) || !isCopyable(focused)) return false; - let targetWorkspace: WorkspaceSvg | null; - let hideChaff = false; - if (focused instanceof BlockSvg) { - hideChaff = !focused.workspace.isFlyout; - targetWorkspace = - focused.workspace instanceof WorkspaceSvg - ? focused.workspace - : workspace; - targetWorkspace = targetWorkspace.isFlyout - ? targetWorkspace.targetWorkspace - : targetWorkspace; - } else { - targetWorkspace = workspace.isFlyout - ? workspace.targetWorkspace - : workspace; - } + const targetWorkspace = workspace.isFlyout + ? workspace.targetWorkspace + : workspace; if (!targetWorkspace) return false; - if (hideChaff) { + if (focused.workspace.isFlyout) { targetWorkspace.hideChaff(); } copyData = focused.toCopyData(); @@ -230,27 +216,21 @@ export function registerCut() { }, callback(workspace, e, shortcut, scope) { const focused = scope.focusedNode; + if (!focused || !isCuttable(focused) || !isICopyable(focused)) { + return false; + } + copyData = focused.toCopyData(); + copyWorkspace = workspace; + 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], }; From f1b44db6f4925b8880ba2182cb73d2cf05f68cce Mon Sep 17 00:00:00 2001 From: Erik Pasternak Date: Tue, 10 Jun 2025 13:52:14 -0700 Subject: [PATCH 06/10] Add missing bang --- core/shortcut_items.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/shortcut_items.ts b/core/shortcut_items.ts index 826cef285..615f1edc2 100644 --- a/core/shortcut_items.ts +++ b/core/shortcut_items.ts @@ -175,7 +175,7 @@ export function registerCopy() { : workspace; if (!targetWorkspace) return false; - if (focused.workspace.isFlyout) { + if (!focused.workspace.isFlyout) { targetWorkspace.hideChaff(); } copyData = focused.toCopyData(); From 32bb84ec8fbb9a23295ce5bb6328548ce55e3db2 Mon Sep 17 00:00:00 2001 From: Erik Pasternak Date: Fri, 13 Jun 2025 11:57:03 -0700 Subject: [PATCH 07/10] Allow copying from readonly workspace and add cut tests Also cleans up logic a bit --- core/shortcut_items.ts | 22 +++--- tests/mocha/shortcut_items_test.js | 123 ++++++++++++++++++++++++++++- 2 files changed, 131 insertions(+), 14 deletions(-) diff --git a/core/shortcut_items.ts b/core/shortcut_items.ts index 615f1edc2..dca2d7366 100644 --- a/core/shortcut_items.ts +++ b/core/shortcut_items.ts @@ -94,7 +94,6 @@ export function registerDelete() { } let copyData: ICopyData | null = null; -let copyWorkspace: WorkspaceSvg | null = null; let copyCoords: Coordinate | null = null; /** @@ -156,7 +155,6 @@ export function registerCopy() { return ( !!focused && !!targetWorkspace && - !targetWorkspace.isReadOnly() && !targetWorkspace.isDragging() && !getFocusManager().ephemeralFocusTaken() && isCopyable(focused) @@ -179,7 +177,6 @@ export function registerCopy() { targetWorkspace.hideChaff(); } copyData = focused.toCopyData(); - copyWorkspace = targetWorkspace; copyCoords = isDraggable(focused) && focused.workspace == targetWorkspace ? focused.getRelativeToSurfaceXY() @@ -220,7 +217,6 @@ export function registerCut() { return false; } copyData = focused.toCopyData(); - copyWorkspace = workspace; copyCoords = isDraggable(focused) ? focused.getRelativeToSurfaceXY() : null; @@ -264,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. @@ -273,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); @@ -293,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], }; diff --git a/tests/mocha/shortcut_items_test.js b/tests/mocha/shortcut_items_test.js index 622df9efc..7667ba387 100644 --- a/tests/mocha/shortcut_items_test.js +++ b/tests/mocha/shortcut_items_test.js @@ -173,12 +173,17 @@ suite('Keyboard Shortcut Items', function () { }); }); }); - // Do not copy a block if a workspace is in readonly mode. - suite('Not called when readOnly is true', function () { + // Allow copying a block if a workspace is in readonly mode. + 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. @@ -238,6 +243,118 @@ suite('Keyboard Shortcut Items', function () { }); }); + 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); + }); + }); + suite('Undo', function () { setup(function () { this.undoSpy = sinon.spy(this.workspace, 'undo'); From a88836227c4f0aac1dc9682d55fa2676e7a485f5 Mon Sep 17 00:00:00 2001 From: Erik Pasternak Date: Fri, 13 Jun 2025 13:07:53 -0700 Subject: [PATCH 08/10] Add tests for workspace comments --- tests/mocha/shortcut_items_test.js | 44 ++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/mocha/shortcut_items_test.js b/tests/mocha/shortcut_items_test.js index 7667ba387..4ab83d8e1 100644 --- a/tests/mocha/shortcut_items_test.js +++ b/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. @@ -241,6 +251,22 @@ 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 () { @@ -353,6 +379,24 @@ suite('Keyboard Shortcut Items', function () { 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 () { From f117bbad22b669ad80208d45d7552ed2017bf3dd Mon Sep 17 00:00:00 2001 From: RoboErikG Date: Mon, 16 Jun 2025 12:35:10 -0700 Subject: [PATCH 09/10] Simplify check for existence of isCopyable Co-authored-by: Christopher Allen --- core/shortcut_items.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/shortcut_items.ts b/core/shortcut_items.ts index dca2d7366..25295e417 100644 --- a/core/shortcut_items.ts +++ b/core/shortcut_items.ts @@ -108,7 +108,7 @@ let copyCoords: Coordinate | null = null; function isCopyable(focused: IFocusableNode): boolean { if (!isICopyable(focused) || !isIDeletable(focused) || !isDraggable(focused)) return false; - if (focused.isCopyable !== undefined) { + if (focused.isCopyable) { return focused.isCopyable(); } else if ( focused instanceof BlockSvg || From 2bae8eb377f881abb061e9a45f563a1800725ef2 Mon Sep 17 00:00:00 2001 From: Erik Pasternak Date: Mon, 16 Jun 2025 12:38:46 -0700 Subject: [PATCH 10/10] Update isCopyable comment --- core/interfaces/i_copyable.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/interfaces/i_copyable.ts b/core/interfaces/i_copyable.ts index 246dd4dd5..6c354926a 100644 --- a/core/interfaces/i_copyable.ts +++ b/core/interfaces/i_copyable.ts @@ -17,7 +17,8 @@ export interface ICopyable extends ISelectable { toCopyData(): T | null; /** - * Whether this instance is currently copyable. + * 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. */