fix: Cursor position after delete (#261)

* fix: Have LineCursor decide where to move itself upon delete

Refactor to have LineCursor decide where it will move when a block
deletion occurs (if it needs to move).

The cursor is moved to one of the following locations (in order
of preference) that is valid:

- The connection to which the deleted block attached.
- The block connected to the next connection of the deleted block.
- The parent block of the deleted block.
- A location on the workspace beneath the deleted block.

BUG: because the deletion has not yet occurred, the "if valid"
check is not accurate, and e.g. the cursor will get moved to
the parent block when deleting the last/only block connected
to an input, instead of getting moved to the input itself
(which would normally be allowed for empty inputs).

Fixes #254.

* fix: Check post-delete node after deletion

Split prepareForDelete into preDelete and postDelete, with the
former making a list of possible places to move the cursor to,
and the latter checking to see if they are still valid locations
after the deletion has occurred.

Introduces a new validNode method to check for validity.

* chore: Format

Author: cpcallen

src/actions/clipboard.ts

@@ -13,6 +13,7 @@ import {
 } from 'blockly';
 import * as Constants from '../constants';
 import type {BlockSvg, WorkspaceSvg} from 'blockly';
+import {LineCursor} from '../line_cursor';
 import {Navigation} from '../navigation';
 
 const KeyCodes = blocklyUtils.KeyCodes;
@@ -163,14 +164,15 @@ export class Clipboard {
    * @returns True if this function successfully handled cutting.
    */
   private cutCallback(workspace: WorkspaceSvg) {
-    const sourceBlock = workspace
-      .getCursor()
-      ?.getCurNode()
-      .getSourceBlock() as BlockSvg;
+    const cursor = workspace.getCursor();
+    if (!cursor) throw new TypeError('no cursor');
+    const sourceBlock = cursor.getCurNode().getSourceBlock() as BlockSvg | null;
+    if (!sourceBlock) throw new TypeError('no source block');
     this.copyData = sourceBlock.toCopyData();
     this.copyWorkspace = sourceBlock.workspace;
-    this.navigation.moveCursorOnBlockDelete(workspace, sourceBlock);
+    if (cursor instanceof LineCursor) cursor.preDelete(sourceBlock);
     sourceBlock.checkAndDelete();
+    if (cursor instanceof LineCursor) cursor.postDelete();
     return true;
   }
 

src/actions/delete.ts

@@ -12,6 +12,7 @@ import {
 } from 'blockly';
 import * as Constants from '../constants';
 import type {BlockSvg, WorkspaceSvg} from 'blockly';
+import {LineCursor} from '../line_cursor';
 import {Navigation} from '../navigation';
 
 const KeyCodes = BlocklyUtils.KeyCodes;
@@ -181,8 +182,9 @@ export class DeleteAction {
     // Don't delete while dragging.  Jeez.
     if (Gesture.inProgress()) false;
 
-    this.navigation.moveCursorOnBlockDelete(workspace, sourceBlock);
+    if (cursor instanceof LineCursor) cursor.preDelete(sourceBlock);
     sourceBlock.checkAndDelete();
+    if (cursor instanceof LineCursor) cursor.postDelete();
     return true;
   }
 }

src/actions/insert.ts

@@ -88,9 +88,9 @@ export class InsertAction {
     const insertAboveItem: ContextMenuRegistry.RegistryItem = {
       displayText: (scope) => {
         if (scope.block?.previousConnection) {
-          return 'Insert Block Above (I)'
+          return 'Insert Block Above (I)';
         } else {
-          return 'Insert Block (I)'
+          return 'Insert Block (I)';
         }
       },
       preconditionFn: (scope: Scope) => {

src/line_cursor.ts

@@ -46,6 +46,9 @@ export class LineCursor extends Marker {
   /** Old Cursor instance, saved during installation. */
   private oldCursor: Blockly.Cursor | null = null;
 
+  /** Locations to try moving the cursor to after a deletion. */
+  private potentialNodes: Blockly.ASTNode[] | null = null;
+
   /**
    * @param workspace The workspace this cursor belongs to.
    */
@@ -226,7 +229,7 @@ export class LineCursor extends Marker {
   /**
    * Returns true iff the given node can be visited by the cursor when
    * using the left/right arrow keys.  Specifically, if the node is
-   * for any node for which valideLineNode would return true, plus:
+   * any node for which valideLineNode would return true, plus:
    *
    * - Any block.
    * - Any field that is not a full block field.
@@ -259,6 +262,21 @@ export class LineCursor extends Marker {
     }
   }
 
+  /**
+   * Returns true iff the given node can be visited by the cursor.
+   * Specifically, if the node is any for which validInLineNode woudl
+   * return true, or if it is a workspace node.
+   *
+   * @param node The AST node to check whether it is valid.
+   * @returns True if the node should be visited, false otherwise.
+   */
+  protected validNode(node: ASTNode | null): boolean {
+    return (
+      !!node &&
+      (this.validInLineNode(node) || node.getType() === ASTNode.types.WORKSPACE)
+    );
+  }
+
   /**
    * Moves the cursor to the next sibling that is at the same level
    * of nesting.
@@ -487,6 +505,82 @@ export class LineCursor extends Marker {
     return this.getRightMostChild(newNode);
   }
 
+  /**
+   * Prepare for the deletion of a block by making a list of nodes we
+   * could move the cursor to afterwards and save it to
+   * this.potentialNodes.
+   *
+   * After the deletion has occurred, call postDelete to move it to
+   * the first valid node on that list.
+   *
+   * The locations to try (in order of preference) are:
+   *
+   * - The current location.
+   * - The connection to which the deleted block is attached.
+   * - The block connected to the next connection of the deleted block.
+   * - The parent block of the deleted block.
+   * - A location on the workspace beneath the deleted block.
+   *
+   * N.B.: When block is deleted, all of the blocks conneccted to that
+   * block's inputs are also deleted, but not blocks connected to its
+   * next connection.
+   *
+   * @param deletedBlock The block that is being deleted.
+   */
+  preDelete(deletedBlock: Blockly.Block) {
+    const curNode = this.getCurNode();
+
+    const nodes: Blockly.ASTNode[] = [curNode];
+    // The connection to which the deleted block is attached.
+    const parentConnection =
+      deletedBlock.previousConnection?.targetConnection ??
+      deletedBlock.outputConnection?.targetConnection;
+    if (parentConnection) {
+      const parentNode = Blockly.ASTNode.createConnectionNode(parentConnection);
+      if (parentNode) nodes.push(parentNode);
+    }
+    // The block connected to the next connection of the deleted block.
+    const nextBlock = deletedBlock.getNextBlock();
+    if (nextBlock) {
+      const nextNode = Blockly.ASTNode.createBlockNode(nextBlock);
+      if (nextNode) nodes.push(nextNode);
+    }
+    //  The parent block of the deleted block.
+    const parentBlock = deletedBlock.getParent();
+    if (parentBlock) {
+      const parentNode = Blockly.ASTNode.createBlockNode(parentBlock);
+      if (parentNode) nodes.push(parentNode);
+    }
+    // A location on the workspace beneath the deleted block.
+    // Move to the workspace.
+    const curBlock = curNode.getSourceBlock();
+    if (curBlock) {
+      const workspaceNode = Blockly.ASTNode.createWorkspaceNode(
+        this.workspace,
+        curBlock.getRelativeToSurfaceXY(),
+      );
+      if (workspaceNode) nodes.push(workspaceNode);
+    }
+    this.potentialNodes = nodes;
+  }
+
+  /**
+   * Move the cursor to the first valid location in
+   * this.potentialNodes, following a block deletion.
+   */
+  postDelete() {
+    const nodes = this.potentialNodes;
+    this.potentialNodes = null;
+    if (!nodes) throw new Error('must call preDelete first');
+    for (const node of nodes) {
+      if (this.validNode(node) && !node.getSourceBlock()?.disposed) {
+        this.setCurNode(node);
+        return;
+      }
+    }
+    throw new Error('no valid nodes in this.potentialNodes');
+  }
+
   /**
    * Get the current location of the cursor.
    *

src/navigation.ts

@@ -407,61 +407,6 @@ export class Navigation {
     this.setState(mainWorkspace, Constants.STATE.FLYOUT);
   }
 
-  /**
-   * Moves the cursor to the appropriate location before a block is deleted.
-   * This is used when the user deletes a block using the delete or backspace
-   * key.
-   *
-   * @param workspace The workspace the block is being deleted on.
-   * @param deletedBlock The block that is being deleted.
-   */
-  moveCursorOnBlockDelete(
-    workspace: Blockly.WorkspaceSvg,
-    deletedBlock: Blockly.BlockSvg,
-  ) {
-    const cursor = workspace.getCursor();
-    if (!cursor) {
-      return;
-    }
-    const curNode = cursor.getCurNode();
-    const block = curNode ? curNode.getSourceBlock() : null;
-
-    if (block === deletedBlock) {
-      // If the block has a parent move the cursor to their connection point.
-      if (block.getParent()) {
-        const topConnection =
-          block.previousConnection || block.outputConnection;
-        if (topConnection?.targetConnection) {
-          cursor.setCurNode(
-            Blockly.ASTNode.createConnectionNode(
-              topConnection.targetConnection,
-            )!,
-          );
-        }
-      } else {
-        // If the block is by itself move the cursor to the workspace.
-        cursor.setCurNode(
-          Blockly.ASTNode.createWorkspaceNode(
-            block.workspace,
-            block.getRelativeToSurfaceXY(),
-          )!,
-        );
-      }
-      // If the cursor is on a block whose parent is being deleted, move the
-      // cursor to the workspace.
-    } else if (
-      block &&
-      deletedBlock.getChildren(false).includes(block as Blockly.BlockSvg)
-    ) {
-      cursor.setCurNode(
-        Blockly.ASTNode.createWorkspaceNode(
-          block.workspace,
-          block.getRelativeToSurfaceXY(),
-        )!,
-      );
-    }
-  }
-
   /**
    * Sets the navigation state to toolbox and selects the first category in the
    * toolbox. No-op if a toolbox does not exist on the given workspace.