diff --git a/src/line_cursor.ts b/src/line_cursor.ts index 1325c22..4c39357 100644 --- a/src/line_cursor.ts +++ b/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); } }