feat: improved insertion flow (#455)

* refactor: create findInsertStartPoint

* refactor: pass Navigation into the keyboard drag strategy

* refactor: pass block into startMove

* refactor: set selected and curNode in startDrag

* feat: wire isNewBlock into keyboard drag strategy

* feat: monkeypatch dragger as needed to delete on aborted insertion

* feat: pass insertStartPoint into the drag strategy after all

* feat: make insertion wrap a block immediately

* fix: undo deletes the newly insertedblock

* fix: reference to getCurrentBlock

* chore: fix review feedback

* chore: remove unnecessary unpatch method

Author: rachel-fenichel

src/actions/enter.ts

@@ -128,30 +128,28 @@ export class EnterAction {
    */
   private insertFromFlyout(workspace: WorkspaceSvg) {
     workspace.setResizesEnabled(false);
-    Events.setGroup(true);
+    // Create a new event group or append to the existing group.
+    const existingGroup = Events.getGroup();
+    if (!existingGroup) {
+      Events.setGroup(true);
+    }
 
     const stationaryNode = this.navigation.getStationaryNode(workspace);
     const newBlock = this.createNewBlock(workspace);
     if (!newBlock) return;
-    if (stationaryNode) {
-      if (!this.navigation.tryToConnectBlock(stationaryNode, newBlock)) {
-        console.warn(
-          'Something went wrong while inserting a block from the flyout.',
-        );
-      }
-    }
-
+    const insertStartPoint = stationaryNode
+      ? this.navigation.findInsertStartPoint(stationaryNode, newBlock)
+      : null;
     if (workspace.getTopBlocks().includes(newBlock)) {
       this.positionNewTopLevelBlock(workspace, newBlock);
     }
 
-    Events.setGroup(false);
     workspace.setResizesEnabled(true);
 
     this.navigation.focusWorkspace(workspace);
     // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
     workspace.getCursor()?.setCurNode(ASTNode.createBlockNode(newBlock)!);
-    this.mover.startMove(workspace);
+    this.mover.startMove(workspace, newBlock, insertStartPoint);
 
     const isStartBlock =
       !newBlock.outputConnection &&

src/actions/move.ts

@@ -5,6 +5,7 @@
  */
 
 import {
+  BlockSvg,
   ContextMenuRegistry,
   ShortcutRegistry,
   utils,
@@ -28,8 +29,16 @@ export class MoveActions {
     // Begin and end move.
     {
       name: 'Start move',
-      preconditionFn: (workspace) => this.mover.canMove(workspace),
-      callback: (workspace) => this.mover.startMove(workspace),
+      preconditionFn: (workspace) => {
+        const startBlock = this.getCurrentBlock(workspace);
+        return !!startBlock && this.mover.canMove(workspace, startBlock);
+      },
+      callback: (workspace) => {
+        const startBlock = this.getCurrentBlock(workspace);
+        return (
+          !!startBlock && this.mover.startMove(workspace, startBlock, null)
+        );
+      },
       keyCodes: [KeyCodes.M],
     },
     {
@@ -131,12 +140,19 @@ export class MoveActions {
         const workspace = scope.block?.workspace as WorkspaceSvg | null;
         if (!workspace || menuOpenEvent instanceof PointerEvent)
           return 'hidden';
-        return this.mover.canMove(workspace) ? 'enabled' : 'disabled';
+
+        const startBlock = this.getCurrentBlock(workspace);
+        return !!startBlock && this.mover.canMove(workspace, startBlock)
+          ? 'enabled'
+          : 'disabled';
       },
       callback: (scope) => {
         const workspace = scope.block?.workspace as WorkspaceSvg | null;
         if (!workspace) return false;
-        this.mover.startMove(workspace);
+        const startBlock = this.getCurrentBlock(workspace);
+        return (
+          !!startBlock && this.mover.startMove(workspace, startBlock, null)
+        );
       },
       scopeType: ContextMenuRegistry.ScopeType.BLOCK,
       id: 'move',
@@ -168,4 +184,30 @@ export class MoveActions {
       ContextMenuRegistry.registry.unregister(menuItem.id);
     }
   }
+
+  /**
+   * Get the source block for the cursor location, or undefined if no
+   * source block can be found.
+   * If the cursor is on a shadow block, walks up the tree until it finds
+   * a non-shadow block to drag.
+   *
+   * @param workspace The workspace to inspect for a cursor.
+   * @returns The source block, or undefined if no appropriate block
+   *     could be found.
+   */
+  getCurrentBlock(workspace: WorkspaceSvg): BlockSvg | undefined {
+    const curNode = workspace?.getCursor()?.getCurNode();
+    let block = curNode?.getSourceBlock();
+    if (!block) return undefined;
+    while (block?.isShadow()) {
+      block = block.getParent();
+      if (!block) {
+        throw new Error(
+          'Tried to drag a shadow block with no parent. ' +
+            'Shadow blocks should always have parents.',
+        );
+      }
+    }
+    return block as BlockSvg;
+  }
 }

src/actions/mover.ts

@@ -4,11 +4,17 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import type {BlockSvg, IDragger, IDragStrategy, Gesture} from 'blockly';
+import type {
+  BlockSvg,
+  IDragger,
+  IDragStrategy,
+  Gesture,
+  RenderedConnection,
+} from 'blockly';
 import {
   ASTNode,
-  common,
   Connection,
+  dragging,
   registry,
   utils,
   WorkspaceSvg,
@@ -63,11 +69,10 @@ export class Mover {
    * currently has focus on the given workspace.
    *
    * @param workspace The workspace to move on.
+   * @param block The block to try to drag.
    * @returns True iff we can begin a move.
    */
-  canMove(workspace: WorkspaceSvg) {
-    const block = this.getCurrentBlock(workspace);
-
+  canMove(workspace: WorkspaceSvg, block: BlockSvg) {
     return !!(
       this.navigation.getState(workspace) === Constants.STATE.WORKSPACE &&
       this.navigation.canCurrentlyEdit(workspace) &&
@@ -96,19 +101,18 @@ export class Mover {
    * Should only be called if canMove has returned true.
    *
    * @param workspace The workspace we might be moving on.
+   * @param block The block to start dragging.
+   * @param insertStartPoint Where to insert the block, or null if the block
+   *     already existed.
    * @returns True iff a move has successfully begun.
    */
-  startMove(workspace: WorkspaceSvg) {
-    const cursor = workspace?.getCursor();
-    const block = this.getCurrentBlock(workspace);
-    if (!cursor || !block) throw new Error('precondition failure');
-
-    // Select and focus block.
-    common.setSelected(block);
-    cursor.setCurNode(ASTNode.createBlockNode(block));
-
+  startMove(
+    workspace: WorkspaceSvg,
+    block: BlockSvg,
+    insertStartPoint: RenderedConnection | null,
+  ) {
     this.patchWorkspace(workspace);
-    this.patchDragStrategy(block);
+    this.patchDragStrategy(block, insertStartPoint);
     // Begin dragging block.
     const DraggerClass = registry.getClassFromOptions(
       registry.Type.BLOCK_DRAGGER,
@@ -167,19 +171,34 @@ export class Mover {
     const info = this.moves.get(workspace);
     if (!info) throw new Error('no move info for workspace');
 
-    // Monkey patch dragger to trigger call to draggable.revertDrag.
-    // eslint-disable-next-line @typescript-eslint/no-explicit-any
-    (info.dragger as any).shouldReturnToStart = () => true;
-    const blockSvg = info.block;
+    const dragStrategy = info.block.getDragStrategy() as KeyboardDragStrategy;
+    this.patchDragger(
+      info.dragger as dragging.Dragger,
+      dragStrategy.isNewBlock,
+    );
 
     // Explicitly call `hidePreview` because it is not called in revertDrag.
-    // @ts-expect-error Access to private property dragStrategy.
-    blockSvg.dragStrategy.connectionPreviewer.hidePreview();
+    // @ts-expect-error Access to private property connectionPreviewer.
+    dragStrategy.connectionPreviewer.hidePreview();
+
+    // Save the position so we can put the cursor in a reasonable spot.
+    // @ts-expect-error Access to private property connectionCandidate.
+    const target = dragStrategy.connectionCandidate?.neighbour;
+
+    // Prevent the stragegy connecting the block so we just delete one block.
+    // @ts-expect-error Access to private property connectionCandidate.
+    dragStrategy.connectionCandidate = null;
+
     info.dragger.onDragEnd(
       info.fakePointerEvent('pointerup'),
-      new utils.Coordinate(0, 0),
+      info.startLocation,
     );
 
+    if (dragStrategy.isNewBlock && target) {
+      const newNode = ASTNode.createConnectionNode(target);
+      if (newNode) workspace.getCursor()?.setCurNode(newNode);
+    }
+
     this.unpatchWorkspace(workspace);
     this.unpatchDragStrategy(info.block);
     this.moves.delete(workspace);
@@ -233,32 +252,6 @@ export class Mover {
     return true;
   }
 
-  /**
-   * Get the source block for the cursor location, or undefined if no
-   * source block can be found.
-   * If the cursor is on a shadow block, walks up the tree until it finds
-   * a non-shadow block to drag.
-   *
-   * @param workspace The workspace to inspect for a cursor.
-   * @returns The source block, or undefined if no appropriate block
-   *     could be found.
-   */
-  protected getCurrentBlock(workspace: WorkspaceSvg): BlockSvg | undefined {
-    const curNode = workspace?.getCursor()?.getCurNode();
-    let block = curNode?.getSourceBlock();
-    if (!block) return undefined;
-    while (block && block.isShadow()) {
-      block = block.getParent();
-      if (!block) {
-        throw new Error(
-          'Tried to drag a shadow block with no parent. ' +
-            'Shadow blocks should always have parents.',
-        );
-      }
-    }
-    return block as BlockSvg;
-  }
-
   /**
    * Monkeypatch over workspace functions to consider keyboard drags as
    * well as mouse/pointer drags.
@@ -297,15 +290,21 @@ export class Mover {
       (workspace as any).getGesture = this.oldGetGesture;
     }
   }
+
   /**
    * Monkeypatch: replace the block's drag strategy and cache the old value.
    *
    * @param block The block to patch.
+   * @param insertStartPoint Where to insert the block, or null if the block
+   *     already existed.
    */
-  private patchDragStrategy(block: BlockSvg) {
+  private patchDragStrategy(
+    block: BlockSvg,
+    insertStartPoint: RenderedConnection | null,
+  ) {
     // @ts-expect-error block.dragStrategy is private.
     this.oldDragStrategy = block.dragStrategy;
-    block.setDragStrategy(new KeyboardDragStrategy(block));
+    block.setDragStrategy(new KeyboardDragStrategy(block, insertStartPoint));
   }
 
   /**
@@ -328,14 +327,34 @@ export class Mover {
    *     the workspace's viewport.
    */
   private scrollCurrentBlockIntoView(workspace: WorkspaceSvg, padding = 10) {
-    const blockToView = this.getCurrentBlock(workspace);
+    const blockToView = this.moves.get(workspace)?.block;
     if (blockToView) {
       workspace.scrollBoundsIntoView(
         blockToView.getBoundingRectangleWithoutChildren(),
         padding,
       );
     }
   }
+
+  /**
+   * Monkeypatch: override either wouldDeleteDraggable or shouldReturnToStart,
+   * based on whether this was an insertion of a new block or a movement of
+   * an existing block.
+   *
+   * @param dragger The dragger to patch.
+   * @param isNewBlock Whether the moving block was created for this action.
+   */
+  private patchDragger(dragger: dragging.Dragger, isNewBlock: boolean) {
+    if (isNewBlock) {
+      // Monkey patch dragger to trigger delete.
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      (dragger as any).wouldDeleteDraggable = () => true;
+    } else {
+      // Monkey patch dragger to trigger call to draggable.revertDrag.
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      (dragger as any).shouldReturnToStart = () => true;
+    }
+  }
 }
 
 /**