fix: bumping copied objects (#7349)

* fix: add logic for bumping pasted blocks

* chore: add tests for bumping pasted blocks to the correct location

* fix: add logic for bumping pasted comments

* chore: add tests for bumping pasted comments

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<BlockCopyData, BlockSvg> {
   static TYPE = 'block';
@@ -26,12 +28,90 @@ export class BlockPaster implements IPaster<BlockCopyData, BlockSvg> {
       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) 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.
+ *
+ * Exported for testing.
+ *
+ * @param block The block to move to an unambiguous location.
+ * @internal
+ */
+export 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};

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,

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);
 

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) {
@@ -1441,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 {

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,101 @@ 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),
+        );
+      });
+    });
+  });
+
+  suite('pasting comments', function () {
+    test('pasted comments are bumped to not overlap', function () {
+      Blockly.Xml.domToWorkspace(
+        Blockly.utils.xml.textToDom(
+          '<xml><comment id="test" x=10 y=10/></xml>',
+        ),
+        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),
+      );
+    });
+  });
 });

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));