From b94ccd8e944f08674cd5d20142621053c0443f2e Mon Sep 17 00:00:00 2001 From: Beka Westberg Date: Tue, 1 Aug 2023 18:36:20 +0000 Subject: [PATCH 1/4] fix: add logic for bumping pasted blocks --- core/clipboard/block_paster.ts | 85 ++++++++++++++++++++++++++++++++-- core/workspace_svg.ts | 4 ++ 2 files changed, 86 insertions(+), 3 deletions(-) diff --git a/core/clipboard/block_paster.ts b/core/clipboard/block_paster.ts index 3ed8383db25..3f3ba690be6 100644 --- a/core/clipboard/block_paster.ts +++ b/core/clipboard/block_paster.ts @@ -11,6 +11,8 @@ import {IPaster} from '../interfaces/i_paster.js'; import {State, append} from '../serialization/blocks.js'; import {Coordinate} from '../utils/coordinate.js'; import {WorkspaceSvg} from '../workspace_svg.js'; +import * as eventUtils from '../events/utils.js'; +import {config} from '../config.js'; export class BlockPaster implements IPaster { static TYPE = 'block'; @@ -26,12 +28,89 @@ export class BlockPaster implements IPaster { copyData.blockState['x'] = coordinate.x; copyData.blockState['y'] = coordinate.y; } - return append(copyData.blockState, workspace, { - recordUndo: true, - }) as BlockSvg; + + eventUtils.disable(); + let block; + try { + block = append(copyData.blockState, workspace, { + recordUndo: true, + }) as BlockSvg; + moveBlockToNotConflict(block); + } finally { + eventUtils.enable(); + } + + if (!block) return block; + + if (eventUtils.isEnabled() && !block.isShadow()) { + eventUtils.fire(new (eventUtils.get(eventUtils.BLOCK_CREATE))(block)); + } + block.select(); + return block; } } +/** + * Moves the given block to a location where it does not: (1) overlap exactly + * with any other blocks, or (2) look like it is connected to any other blocks. + * + * @param block The block to move to an unambiguous location. + */ +function moveBlockToNotConflict(block: BlockSvg) { + const workspace = block.workspace; + const snapRadius = config.snapRadius; + const coord = block.getRelativeToSurfaceXY(); + const offset = new Coordinate(0, 0); + // getRelativeToSurfaceXY is really expensive, so we want to cache this. + const otherCoords = workspace + .getAllBlocks(false) + .filter((otherBlock) => otherBlock.id != block.id) + .map((b) => b.getRelativeToSurfaceXY()); + + while ( + blockOverlapsOtherExactly(Coordinate.sum(coord, offset), otherCoords) || + blockIsInSnapRadius(block, offset, snapRadius) + ) { + if (workspace.RTL) { + offset.translate(-snapRadius, snapRadius * 2); + } else { + offset.translate(snapRadius, snapRadius * 2); + } + } + + block!.moveTo(Coordinate.sum(coord, offset)); +} + +/** + * @returns true if the given block coordinates are less than a delta of 1 from + * any of the other coordinates. + */ +function blockOverlapsOtherExactly( + coord: Coordinate, + otherCoords: Coordinate[], +): boolean { + return otherCoords.some( + (otherCoord) => + Math.abs(otherCoord.x - coord.x) <= 1 && + Math.abs(otherCoord.y - coord.y) <= 1, + ); +} + +/** + * @returns true if the given block (when offset by the given amount) is close + * enough to any other connections (within the snap radius) that it looks + * like they could connect. + */ +function blockIsInSnapRadius( + block: BlockSvg, + offset: Coordinate, + snapRadius: number, +): boolean { + return block + .getConnections_(false) + .some((connection) => !!connection.closest(snapRadius, offset).connection); +} + export interface BlockCopyData extends ICopyData { blockState: State; typeCounts: {[key: string]: number}; diff --git a/core/workspace_svg.ts b/core/workspace_svg.ts index 16c84c91eb7..af845d83ba5 100644 --- a/core/workspace_svg.ts +++ b/core/workspace_svg.ts @@ -1388,6 +1388,10 @@ export class WorkspaceSvg extends Workspace implements IASTNodeLocationSvg { for (let i = 0, connection; (connection = connections[i]); i++) { const neighbour = connection.closest( config.snapRadius, + // TODO: This code doesn't work because it's passing an absolute + // coordinate instead of a relative coordinate. Need to + // figure out if I'm deprecating this function or if I + // need to fix this. new Coordinate(blockX, blockY), ); if (neighbour.connection) { From c8a69d05ad75f686f4ac1f6daebb48a8c4c5437a Mon Sep 17 00:00:00 2001 From: Beka Westberg Date: Tue, 1 Aug 2023 19:11:15 +0000 Subject: [PATCH 2/4] chore: add tests for bumping pasted blocks to the correct location --- core/clipboard/block_paster.ts | 9 ++-- core/serialization/blocks.ts | 4 +- tests/mocha/clipboard_test.js | 86 +++++++++++++++++++++++++++++- tests/mocha/test_helpers/events.js | 11 ++-- 4 files changed, 96 insertions(+), 14 deletions(-) diff --git a/core/clipboard/block_paster.ts b/core/clipboard/block_paster.ts index 3f3ba690be6..0bb707c3654 100644 --- a/core/clipboard/block_paster.ts +++ b/core/clipboard/block_paster.ts @@ -32,9 +32,7 @@ export class BlockPaster implements IPaster { eventUtils.disable(); let block; try { - block = append(copyData.blockState, workspace, { - recordUndo: true, - }) as BlockSvg; + block = append(copyData.blockState, workspace) as BlockSvg; moveBlockToNotConflict(block); } finally { eventUtils.enable(); @@ -54,9 +52,12 @@ export class BlockPaster implements IPaster { * Moves the given block to a location where it does not: (1) overlap exactly * with any other blocks, or (2) look like it is connected to any other blocks. * + * Exported for testing. + * * @param block The block to move to an unambiguous location. + * @internal */ -function moveBlockToNotConflict(block: BlockSvg) { +export function moveBlockToNotConflict(block: BlockSvg) { const workspace = block.workspace; const snapRadius = config.snapRadius; const coord = block.getRelativeToSurfaceXY(); diff --git a/core/serialization/blocks.ts b/core/serialization/blocks.ts index c9c6395acf1..f11a7d8cd34 100644 --- a/core/serialization/blocks.ts +++ b/core/serialization/blocks.ts @@ -396,7 +396,9 @@ export function appendInternal( const block = appendPrivate(state, workspace, {parentConnection, isShadow}); eventUtils.enable(); - eventUtils.fire(new (eventUtils.get(eventUtils.BLOCK_CREATE))(block)); + if (eventUtils.isEnabled()) { + eventUtils.fire(new (eventUtils.get(eventUtils.BLOCK_CREATE))(block)); + } eventUtils.setGroup(existingGroup); eventUtils.setRecordUndo(prevRecordUndo); diff --git a/tests/mocha/clipboard_test.js b/tests/mocha/clipboard_test.js index 2db315f6e24..b0040763b31 100644 --- a/tests/mocha/clipboard_test.js +++ b/tests/mocha/clipboard_test.js @@ -1,6 +1,6 @@ /** * @license - * Copyright 2019 Google LLC + * Copyright 2023 Google LLC * SPDX-License-Identifier: Apache-2.0 */ @@ -10,11 +10,15 @@ import { sharedTestSetup, sharedTestTeardown, } from './test_helpers/setup_teardown.js'; +import { + assertEventFired, + createChangeListenerSpy, +} from './test_helpers/events.js'; suite('Clipboard', function () { setup(function () { this.clock = sharedTestSetup.call(this, {fireEventsNow: false}).clock; - this.workspace = new Blockly.WorkspaceSvg(new Blockly.Options({})); + this.workspace = Blockly.inject('blocklyDiv'); }); teardown(function () { @@ -32,4 +36,82 @@ suite('Clipboard', function () { Blockly.clipboard.registry.unregister('test-paster'); }); + + suite('pasting blocks', function () { + test('pasting blocks fires a create event', function () { + const eventSpy = createChangeListenerSpy(this.workspace); + const block = Blockly.serialization.blocks.append( + { + 'type': 'controls_if', + 'id': 'blockId', + }, + this.workspace, + ); + const data = block.toCopyData(); + this.clock.runAll(); + eventSpy.resetHistory(); + + Blockly.clipboard.paste(data, this.workspace); + this.clock.runAll(); + + assertEventFired( + eventSpy, + Blockly.Events.BlockCreate, + {'recordUndo': true, 'type': Blockly.Events.BLOCK_CREATE}, + this.workspace.id, + ); + }); + + suite('pasted blocks are placed in unambiguous locations', function () { + test('pasted blocks are bumped to not overlap', function () { + const block = Blockly.serialization.blocks.append( + { + 'type': 'controls_if', + 'x': 38, + 'y': 13, + }, + this.workspace, + ); + const data = block.toCopyData(); + + const newBlock = Blockly.clipboard.paste(data, this.workspace); + chai.assert.deepEqual( + newBlock.getRelativeToSurfaceXY(), + new Blockly.utils.Coordinate(66, 69), + ); + }); + + test('pasted blocks are bumped to be outside the connection snap radius', function () { + Blockly.serialization.workspaces.load( + { + 'blocks': { + 'languageVersion': 0, + 'blocks': [ + { + 'type': 'controls_if', + 'id': 'sourceBlockId', + 'x': 38, + 'y': 13, + }, + { + 'type': 'logic_compare', + 'x': 113, + 'y': 63, + }, + ], + }, + }, + this.workspace, + ); + this.clock.runAll(); // Update the connection DB. + const data = this.workspace.getBlockById('sourceBlockId').toCopyData(); + + const newBlock = Blockly.clipboard.paste(data, this.workspace); + chai.assert.deepEqual( + newBlock.getRelativeToSurfaceXY(), + new Blockly.utils.Coordinate(94, 125), + ); + }); + }); + }); }); diff --git a/tests/mocha/test_helpers/events.js b/tests/mocha/test_helpers/events.js index 46ce328306e..ff0b5fb3c48 100644 --- a/tests/mocha/test_helpers/events.js +++ b/tests/mocha/test_helpers/events.js @@ -149,13 +149,10 @@ export function assertEventFired( expectedWorkspaceId, expectedBlockId, ) { - expectedProperties = Object.assign( - { - workspaceId: expectedWorkspaceId, - blockId: expectedBlockId, - }, - expectedProperties, - ); + const baseProps = {}; + if (expectedWorkspaceId) baseProps.workspaceId = expectedWorkspaceId; + if (expectedBlockId) baseProps.blockId = expectedBlockId; + expectedProperties = Object.assign(baseProps, expectedProperties); const expectedEvent = sinon.match .instanceOf(instanceType) .and(sinon.match(expectedProperties)); From 83df6309052708416b34f300102d139d52841258 Mon Sep 17 00:00:00 2001 From: Beka Westberg Date: Wed, 2 Aug 2023 20:20:03 +0000 Subject: [PATCH 3/4] fix: add logic for bumping pasted comments --- core/clipboard/workspace_comment_paster.ts | 10 ++++++++-- core/workspace_svg.ts | 3 +++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/core/clipboard/workspace_comment_paster.ts b/core/clipboard/workspace_comment_paster.ts index b9933f5a18c..aeedbfb2b77 100644 --- a/core/clipboard/workspace_comment_paster.ts +++ b/core/clipboard/workspace_comment_paster.ts @@ -21,9 +21,15 @@ export class WorkspaceCommentPaster workspace: WorkspaceSvg, coordinate?: Coordinate, ): WorkspaceCommentSvg { + const state = copyData.commentState; if (coordinate) { - copyData.commentState.setAttribute('x', `${coordinate.x}`); - copyData.commentState.setAttribute('y', `${coordinate.y}`); + state.setAttribute('x', `${coordinate.x}`); + state.setAttribute('y', `${coordinate.y}`); + } else { + const x = parseInt(state.getAttribute('x') ?? '0') + 50; + const y = parseInt(state.getAttribute('y') ?? '0') + 50; + state.setAttribute('x', `${x}`); + state.setAttribute('y', `${y}`); } return WorkspaceCommentSvg.fromXmlRendered( copyData.commentState, diff --git a/core/workspace_svg.ts b/core/workspace_svg.ts index af845d83ba5..5978c3197db 100644 --- a/core/workspace_svg.ts +++ b/core/workspace_svg.ts @@ -1445,6 +1445,9 @@ export class WorkspaceSvg extends Workspace implements IASTNodeLocationSvg { // with any blocks. commentX += 50; commentY += 50; + // TODO: This code doesn't work because it's using absolute coords + // where relative coords are expected. Need to figure out what I'm + // doing with this function and if I need to fix it. comment.moveBy(commentX, commentY); } } finally { From b3ee3ba9fe1af0211abee88e52117730fcc43ff8 Mon Sep 17 00:00:00 2001 From: Beka Westberg Date: Wed, 2 Aug 2023 20:20:14 +0000 Subject: [PATCH 4/4] chore: add tests for bumping pasted comments --- tests/mocha/clipboard_test.js | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/tests/mocha/clipboard_test.js b/tests/mocha/clipboard_test.js index b0040763b31..f6cf10a2551 100644 --- a/tests/mocha/clipboard_test.js +++ b/tests/mocha/clipboard_test.js @@ -114,4 +114,23 @@ suite('Clipboard', function () { }); }); }); + + suite('pasting comments', function () { + test('pasted comments are bumped to not overlap', function () { + Blockly.Xml.domToWorkspace( + Blockly.utils.xml.textToDom( + '', + ), + this.workspace, + ); + const comment = this.workspace.getTopComments(false)[0]; + const data = comment.toCopyData(); + + const newComment = Blockly.clipboard.paste(data, this.workspace); + chai.assert.deepEqual( + newComment.getRelativeToSurfaceXY(), + new Blockly.utils.Coordinate(60, 60), + ); + }); + }); });