feat: Make cursor arrow keys behave as per #129 (#143)

* feat: Make cursor arrow keys behave as per #129

  Modify LineCursor so that movement via arrow keys behaves as
  described in #129.

* fix: Remove debug logging

* fix: typos

Author: cpcallen

src/line_cursor.ts

@@ -110,60 +110,66 @@ export class LineCursor extends Marker {
   }
 
   /**
-   * Decides if the previous and next methods should traverse the given node.
-   * The previous and next method only traverse previous connections, next
-   * connections and blocks.
+   * Returns true iff the given node represents the "beginning of a
+   * new line of code" (and thus can be visited by pressing the
+   * up/down arrow keys).  Specifically, if the node is for:
+   *
+   * - Any block that is not a value block.
+   * - A top-level value block (one that is unconnected).
+   * - An unconnected next statement input.
+   * - An unconnected 'next' connection - the "blank line at the end".
+   *   This is to facilitate connecting additional blocks to a
+   *   stack/substack.
    *
    * @param node The AST node to check.
    * @returns True if the node should be visited, false otherwise.
    * @protected
    */
   validLineNode(node: ASTNode | null): boolean {
-    if (!node) {
-      return false;
-    }
-    let isValid = false;
+    if (!node) return false;
     const location = node.getLocation();
     const type = node && node.getType();
-    if (type == ASTNode.types.BLOCK) {
-      if ((location as Blockly.Block).outputConnection === null) {
-        isValid = true;
-      }
-    } else if (
-      type == ASTNode.types.INPUT &&
-      (location as Blockly.Connection).type == Blockly.NEXT_STATEMENT
-    ) {
-      isValid = true;
-    } else if (type == ASTNode.types.NEXT) {
-      isValid = true;
+    switch (type) {
+      case ASTNode.types.BLOCK:
+        return !((location as Blockly.Block).outputConnection?.isConnected());
+      case ASTNode.types.INPUT:
+        const connection = (location as Blockly.Connection);
+        return connection.type === Blockly.NEXT_STATEMENT && !connection.isConnected();
+      case ASTNode.types.NEXT:
+        return !((location as Blockly.Connection).isConnected());
+      default:
+        return false;
     }
-    return isValid;
   }
 
   /**
-   * Decides if the in and out methods should traverse the given node.
-   * The in and out method only traverse fields and input connections.
+   * 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 block.
+   * - Any field.
+   * - Any unconnected next or input connection.  This is to
+   *   facilitate connecting additional blocks.
    *
    * @param node The AST node to check whether it is valid.
    * @returns True if the node should be visited, false otherwise.
    * @protected
    */
   validInLineNode(node: ASTNode | null): boolean {
-    if (!node) {
-      return false;
-    }
-    let isValid = false;
+    if (!node) return false;
     const location = node.getLocation();
     const type = node && node.getType();
-    if (type == ASTNode.types.FIELD) {
-      isValid = true;
-    } else if (
-      type == ASTNode.types.INPUT &&
-      (location as Blockly.Connection).type == Blockly.INPUT_VALUE
-    ) {
-      isValid = true;
+    switch (type) {
+      case ASTNode.types.BLOCK:
+        return true;
+      case ASTNode.types.INPUT:
+      case ASTNode.types.NEXT:
+        return !((location as Blockly.Connection).isConnected());
+      case ASTNode.types.FIELD:
+        return true;
+      default:
+        return false;
     }
-    return isValid;
   }
 
   /**
@@ -395,60 +401,59 @@ export class LineCursor extends Marker {
   }
 
   /**
-   * Set the location of the marker and call the update method.
-   * Setting isStack to true will only work if the newLocation is the top most
-   * output or previous connection on a stack.
+   * Set the location of the marker and draw it.
    *
    * Overrides drawing logic to call `setSelected` if the location is
-   * a block, for testing on October 28 2024.
+   * a block, or `addSelect` if it's a shadow block (since shadow
+   * blocks can't be selected).
+   *
+   * TODO(#142): The selection and fake-selection code was originally
+   * a hack added for testing on October 28 2024, because the default
+   * drawer behaviour was to draw a box around the block and all
+   * attached child blocks, which was confusing when navigating
+   * stacks.
+   *
+   * Since then we have decided that we probably _do_ in most cases
+   * want navigating to a block to select the block, but more
+   * particularly that we want navigation to move _focus_.  Replace
+   * this selection hack with non-hacky changing of focus once that's
+   * possible.
    *
    * @param newNode The new location of the marker.
    */
-  setCurNode(newNode: ASTNode) {
-    const oldNode = (this as any).curNode;
-    (this as any).curNode = newNode;
-    const drawer = (this as any).drawer;
+  override setCurNode(newNode: ASTNode) {
+    const oldNode = this.getCurNode();
+    super.setCurNode(newNode);
+    const drawer = this.getDrawer();
+
     if (!drawer) {
       console.error('could not find a drawer');
       return;
     }
 
-    const newNodeIsFieldColour =
-      newNode?.getType() == ASTNode.types.FIELD &&
-      (newNode.getLocation() as Blockly.Field) instanceof FieldColour;
-    const oldNodeIsFieldColour =
-      oldNode?.getType() == ASTNode.types.FIELD &&
-      (oldNode.getLocation() as Blockly.Field) instanceof FieldColour;
-
-    if (newNode?.getType() == ASTNode.types.BLOCK) {
-      drawer.hide();
-      const block = newNode.getLocation() as Blockly.BlockSvg;
-      Blockly.common.setSelected(block);
-    } else if (newNodeIsFieldColour) {
-      drawer.hide();
-
-      if (oldNode?.getType() == ASTNode.types.BLOCK) {
+    // If old node was a block, unselect it or remove fake selection.
+    if (oldNode?.getType() === ASTNode.types.BLOCK) {
+      const block = oldNode.getLocation() as Blockly.BlockSvg;
+      if (!block.isShadow()) {
         Blockly.common.setSelected(null);
-      } else if (oldNodeIsFieldColour) {
-        const field = oldNode.getLocation() as FieldColour;
-        const block = field.getSourceBlock() as Blockly.BlockSvg;
+      } else {
         block.removeSelect();
       }
+    }
 
-      const field = newNode.getLocation() as FieldColour;
-      const block = field.getSourceBlock() as Blockly.BlockSvg;
-      block.addSelect();
-    } else {
-      if (oldNode?.getType() == ASTNode.types.BLOCK) {
-        Blockly.common.setSelected(null);
-      } else if (oldNodeIsFieldColour) {
-        const field = oldNode.getLocation() as FieldColour;
-        const block = field.getSourceBlock() as Blockly.BlockSvg;
-        block.removeSelect();
+    // If new node is a block, select it or make it look selected.
+    if (newNode?.getType() === ASTNode.types.BLOCK) {
+      drawer.hide();
+      const block = newNode.getLocation() as Blockly.BlockSvg;
+      if (!block.isShadow()) {
+        Blockly.common.setSelected(block);
+      } else {
+        block.addSelect();
       }
-
-      drawer.draw(oldNode, newNode);
+      return;
     }
+
+    drawer.draw(oldNode, newNode);
   }
 }