Merge branch 'insert-connection-april-ut' into april-ut

Author: microbit-matt-hillsdon

src/actions/enter.ts

@@ -133,14 +133,10 @@ export class EnterAction {
     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);
     }
@@ -151,7 +147,7 @@ export class EnterAction {
     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, insertStartPoint);
 
     const isTopLevelBlock =
       !newBlock.outputConnection &&

src/actions/mover.ts

@@ -4,11 +4,18 @@
  * 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,
+  Events,
   registry,
   utils,
   WorkspaceSvg,
@@ -101,9 +108,14 @@ export class Mover {
    * Should only be called if canMove has returned true.
    *
    * @param workspace The workspace we might be moving on.
+   * @param insertStartPoint The starting point for the move in the insert case,
+   *     when the block should be deleted if aborted rather than reverted.
    * @returns True iff a move has successfully begun.
    */
-  startMove(workspace: WorkspaceSvg) {
+  startMove(
+    workspace: WorkspaceSvg,
+    insertStartPoint: RenderedConnection | null = null,
+  ) {
     const cursor = workspace?.getCursor();
     const block = this.getCurrentBlock(workspace);
     if (!cursor || !block) throw new Error('precondition failure');
@@ -114,7 +126,7 @@ export class Mover {
     cursor.setCurNode(ASTNode.createBlockNode(block));
 
     this.patchWorkspace(workspace);
-    this.patchDragStrategy(block);
+    this.patchDragStrategy(block, insertStartPoint);
     // Begin dragging block.
     const DraggerClass = registry.getClassFromOptions(
       registry.Type.BLOCK_DRAGGER,
@@ -154,6 +166,7 @@ export class Mover {
     this.unpatchWorkspace(workspace);
     this.unpatchDragStrategy(info.block);
     this.moves.delete(workspace);
+
     return true;
   }
 
@@ -172,17 +185,31 @@ 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.
+    // If it's an insert-style move then we delete the block.
+    const keyboardDragStrategy =
+      info.block.getDragStrategy() as KeyboardDragStrategy;
+    const {insertStartPoint} = keyboardDragStrategy;
+
+    // Monkey patch dragger to trigger delete.
     // eslint-disable-next-line @typescript-eslint/no-explicit-any
-    (info.dragger as any).shouldReturnToStart = () => true;
+    (info.dragger as any).wouldDeleteDraggable = () => !!insertStartPoint;
+    if (!insertStartPoint) {
+      // 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;
 
     // Explicitly call `hidePreview` because it is not called in revertDrag.
     // @ts-expect-error Access to private property dragStrategy.
     blockSvg.dragStrategy.connectionPreviewer.hidePreview();
+    // Prevent the stragegy connecting the block so we just delete one block.
+    // @ts-expect-error Access to private property dragStrategy.
+    blockSvg.dragStrategy.connectionCandidate = null;
+
     info.dragger.onDragEnd(
       info.fakePointerEvent('pointerup'),
-      new utils.Coordinate(0, 0),
+      info.startLocation,
     );
 
     this.unpatchWorkspace(workspace);
@@ -302,11 +329,16 @@ export class Mover {
    * Monkeypatch: replace the block's drag strategy and cache the old value.
    *
    * @param block The block to patch.
+   * @param insertStartPoint The starting point for the move in the insert case,
+   *     when the block should be deleted if aborted rather than reverted.
    */
-  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));
   }
 
   /**

src/keyboard_drag_strategy.ts

@@ -36,6 +36,13 @@ export class KeyboardDragStrategy extends dragging.BlockDragStrategy {
   /** Where a constrained movement should start when traversing the tree. */
   private searchNode: ASTNode | null = null;
 
+  constructor(
+    block: BlockSvg,
+    public insertStartPoint: RenderedConnection | null,
+  ) {
+    super(block);
+  }
+
   override startDrag(e?: PointerEvent) {
     super.startDrag(e);
     // Set position of the dragging block, so that it doesn't pop
@@ -269,7 +276,7 @@ export class KeyboardDragStrategy extends dragging.BlockDragStrategy {
    */
   private createInitialCandidate(): ConnectionCandidate | null {
     // @ts-expect-error startParentConn is private.
-    const neighbour = this.startParentConn;
+    const neighbour = this.insertStartPoint ?? this.startParentConn;
     if (neighbour) {
       this.searchNode = ASTNode.createConnectionNode(neighbour);
       switch (neighbour.type) {

src/navigation.ts

@@ -404,8 +404,8 @@ export class Navigation {
       const passiveFocusNode = this.passiveFocusIndicator.getCurNode();
       this.passiveFocusIndicator.hide();
       const disposed = passiveFocusNode?.getSourceBlock()?.disposed;
-      // If there's a gesture then it will either set the node if it has not 
-      // been disposed (which can happen when blocks are reloaded) or be a click 
+      // If there's a gesture then it will either set the node if it has not
+      // been disposed (which can happen when blocks are reloaded) or be a click
       // that should not set one.
       if (!Blockly.Gesture.inProgress() && passiveFocusNode && !disposed) {
         cursor.setCurNode(passiveFocusNode);
@@ -662,25 +662,25 @@ export class Navigation {
   }
 
   /**
-   * Tries to intelligently connect the blocks or connections
-   * represented by the given nodes, based on node types and locations.
+   * Finds the starting point for an insert.
    *
-   * @param stationaryNode The first node to connect.
+   * @param stationaryNode The first node to find a connection for.
    * @param movingBlock The block we're moving.
-   * @returns True if the key was handled; false if something went
-   *     wrong.
+   * @returns The initial connection to use or begin move mode at.
    */
-  tryToConnectBlock(
+  findInsertStartPoint(
     stationaryNode: Blockly.ASTNode,
     movingBlock: Blockly.BlockSvg,
-  ): boolean {
+  ): Blockly.RenderedConnection | null {
     const stationaryType = stationaryNode.getType();
     const stationaryLoc = stationaryNode.getLocation();
+    const movingHasOutput = !!movingBlock.outputConnection;
 
     if (stationaryNode.getType() === Blockly.ASTNode.types.FIELD) {
+      // Can't connect a block to a field, so try going up to the source block.
       const sourceBlock = stationaryNode.getSourceBlock();
-      if (!sourceBlock) return false;
-      return this.tryToConnectBlock(
+      if (!sourceBlock) return null;
+      return this.findInsertStartPoint(
         Blockly.ASTNode.createBlockNode(sourceBlock),
         movingBlock,
       );
@@ -691,27 +691,27 @@ export class Navigation {
       // Move to the block if we're trying to insert a statement block into
       // a value connection.
       if (
-        !movingBlock.outputConnection &&
+        !movingHasOutput &&
         stationaryAsConnection.type === Blockly.ConnectionType.INPUT_VALUE
       ) {
         const sourceBlock = stationaryNode.getSourceBlock();
-        if (!sourceBlock) return false;
-        return this.tryToConnectBlock(
+        if (!sourceBlock) return null;
+        return this.findInsertStartPoint(
           Blockly.ASTNode.createBlockNode(sourceBlock),
           movingBlock,
         );
       }
 
       // Connect the moving block to the stationary connection using
       // the most plausible connection on the moving block.
-      return this.insertBlock(movingBlock, stationaryAsConnection);
+      return stationaryAsConnection;
     } else if (stationaryType === Blockly.ASTNode.types.WORKSPACE) {
-      return this.moveBlockToWorkspace(movingBlock, stationaryNode);
+      return null;
     } else if (stationaryType === Blockly.ASTNode.types.BLOCK) {
       const stationaryBlock = stationaryLoc as Blockly.BlockSvg;
 
       // 1. Connect blocks to first compatible input
-      const inputType = movingBlock.outputConnection
+      const inputType = movingHasOutput
         ? Blockly.inputs.inputTypes.VALUE
         : Blockly.inputs.inputTypes.STATEMENT;
       const compatibleInputs = stationaryBlock.inputList.filter(
@@ -726,36 +726,61 @@ export class Navigation {
             connection = connection.targetBlock()!.nextConnection!;
           }
         }
-        return this.insertBlock(
-          movingBlock,
-          connection as Blockly.RenderedConnection,
-        );
+        return connection as Blockly.RenderedConnection;
       }
 
       // 2. Connect statement blocks to next connection.
-      if (stationaryBlock.nextConnection && !movingBlock.outputConnection) {
-        return this.insertBlock(movingBlock, stationaryBlock.nextConnection);
+      if (stationaryBlock.nextConnection && !movingHasOutput) {
+        return stationaryBlock.nextConnection;
       }
 
       // 3. Output connection. This will wrap around or displace.
       if (stationaryBlock.outputConnection) {
-        // Move to parent if we're trying to insert a statement block.
-        if (
-          !movingBlock.outputConnection &&
+        // Try to wrap.
+        const target = stationaryBlock.outputConnection.targetConnection;
+        if (movingHasOutput && target) {
+          const sourceNode = Blockly.ASTNode.createConnectionNode(target);
+          if (sourceNode) {
+            return this.findInsertStartPoint(sourceNode, movingBlock);
+          }
+        } else if (
+          !movingHasOutput &&
           stationaryNode.getType() === Blockly.ASTNode.types.BLOCK
         ) {
+          // Move to parent if we're trying to insert a statement block.
           const parent = stationaryNode.getSourceBlock()?.getParent();
-          if (!parent) return false;
-          return this.tryToConnectBlock(
+          if (!parent) return null;
+          return this.findInsertStartPoint(
             Blockly.ASTNode.createBlockNode(parent),
             movingBlock,
           );
         }
-        return this.insertBlock(movingBlock, stationaryBlock.outputConnection);
+        return stationaryBlock.outputConnection;
       }
     }
-    this.warn(`Unexpected case in tryToConnectBlock ${stationaryType}.`);
-    return false;
+    this.warn(`Unexpected case in findInsertStartPoint ${stationaryType}.`);
+    return null;
+  }
+
+  /**
+   * Tries to intelligently connect the blocks or connections
+   * represented by the given nodes, based on node types and locations.
+   *
+   * @param stationaryNode The first node to connect.
+   * @param movingBlock The block we're moving.
+   * @returns True if the key was handled; false if something went
+   *     wrong.
+   */
+  tryToConnectBlock(
+    stationaryNode: Blockly.ASTNode,
+    movingBlock: Blockly.BlockSvg,
+  ): boolean {
+    const destConnection = this.findInsertStartPoint(
+      stationaryNode,
+      movingBlock,
+    );
+    if (!destConnection) return false;
+    return this.insertBlock(movingBlock, destConnection);
   }
 
   /**