From fdffd6558b7a2618c5aae48070be987cb921478a Mon Sep 17 00:00:00 2001 From: RoboErikG Date: Fri, 30 May 2025 10:49:30 -0700 Subject: [PATCH] fix: Make cut/copy/paste work consistently and as expected (#9107) * Work on cut/copy/paste preconditions * Cleanup and fixes to cut/copy/paste * Fix tests * Remove editable check from isCopyable and isCuttable --- core/shortcut_items.ts | 98 +++++++++++++++++++++++------- tests/mocha/shortcut_items_test.js | 12 ++-- 2 files changed, 82 insertions(+), 28 deletions(-) diff --git a/core/shortcut_items.ts b/core/shortcut_items.ts index fb1ca0fce..161d5fceb 100644 --- a/core/shortcut_items.ts +++ b/core/shortcut_items.ts @@ -104,7 +104,7 @@ let copyWorkspace: WorkspaceSvg | null = null; let copyCoords: Coordinate | null = null; /** - * Determine if a focusable node can be copied using cut or copy. + * 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. @@ -113,8 +113,8 @@ let copyCoords: Coordinate | null = null; * - 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). + * - 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 @@ -127,6 +127,40 @@ let copyCoords: Coordinate | null = null; 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() + ); +} + +/** + * 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. + * + * @param focused The focused object. + */ +function isCuttable(focused: IFocusableNode): boolean { + if (!(focused instanceof BlockSvg)) return false; return ( isICopyable(focused) && isIDeletable(focused) && @@ -151,29 +185,45 @@ 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 ( - !workspace.isReadOnly() && - !Gesture.inProgress() && !!focused && - isCopyable(focused) && - !getFocusManager().ephemeralFocusTaken() + !!targetWorkspace && + !targetWorkspace.isReadOnly() && + !targetWorkspace.isDragging() && + !getFocusManager().ephemeralFocusTaken() && + isCopyable(focused) ); }, callback(workspace, e, shortcut, scope) { // Prevent the default copy behavior, which may beep or otherwise indicate // an error due to the lack of a selection. e.preventDefault(); - workspace.hideChaff(); + const focused = scope.focusedNode; - if (!focused || !isICopyable(focused)) return false; - copyData = focused.toCopyData(); - copyWorkspace = + if (!focused || !isCopyable(focused)) return false; + let targetWorkspace: WorkspaceSvg | null = focused.workspace instanceof WorkspaceSvg ? focused.workspace : workspace; - copyCoords = isDraggable(focused) - ? focused.getRelativeToSurfaceXY() - : null; + 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() + : null; return !!copyData; }, keyCodes: [ctrlC, metaC], @@ -197,14 +247,11 @@ export function registerCut() { preconditionFn(workspace, scope) { const focused = scope.focusedNode; return ( - !workspace.isReadOnly() && - !Gesture.inProgress() && !!focused && - isCopyable(focused) && - // Extra criteria for cut (not just copy): - !focused.workspace.isFlyout && - focused.isDeletable() && - !getFocusManager().ephemeralFocusTaken() + !workspace.isReadOnly() && + !workspace.isDragging() && + !getFocusManager().ephemeralFocusTaken() && + isCuttable(focused) ); }, callback(workspace, e, shortcut, scope) { @@ -251,9 +298,14 @@ export function registerPaste() { const pasteShortcut: KeyboardShortcut = { name: names.PASTE, preconditionFn(workspace) { + const targetWorkspace = workspace.isFlyout + ? workspace.targetWorkspace + : workspace; return ( - !workspace.isReadOnly() && - !Gesture.inProgress() && + !!copyData && + !!targetWorkspace && + !targetWorkspace.isReadOnly() && + !targetWorkspace.isDragging() && !getFocusManager().ephemeralFocusTaken() ); }, diff --git a/tests/mocha/shortcut_items_test.js b/tests/mocha/shortcut_items_test.js index 29487ec58..622df9efc 100644 --- a/tests/mocha/shortcut_items_test.js +++ b/tests/mocha/shortcut_items_test.js @@ -181,13 +181,13 @@ suite('Keyboard Shortcut Items', function () { runReadOnlyTest(keyEvent, testCaseName); }); }); - // Do not copy a block if a gesture is in progress. - suite('Gesture in progress', function () { + // Do not copy 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(Blockly.Gesture, 'inProgress').returns(true); + sinon.stub(this.workspace, 'isDragging').returns(true); this.injectionDiv.dispatchEvent(keyEvent); sinon.assert.notCalled(this.copySpy); sinon.assert.notCalled(this.hideChaffSpy); @@ -201,7 +201,7 @@ suite('Keyboard Shortcut Items', function () { const keyEvent = testCase[1]; test(testCaseName, function () { sinon - .stub(Blockly.common.getSelected(), 'isDeletable') + .stub(Blockly.common.getSelected(), 'isOwnDeletable') .returns(false); this.injectionDiv.dispatchEvent(keyEvent); sinon.assert.notCalled(this.copySpy); @@ -215,7 +215,9 @@ suite('Keyboard Shortcut Items', function () { const testCaseName = testCase[0]; const keyEvent = testCase[1]; test(testCaseName, function () { - sinon.stub(Blockly.common.getSelected(), 'isMovable').returns(false); + sinon + .stub(Blockly.common.getSelected(), 'isOwnMovable') + .returns(false); this.injectionDiv.dispatchEvent(keyEvent); sinon.assert.notCalled(this.copySpy); sinon.assert.notCalled(this.hideChaffSpy);