From 98eb77dce56719bb831bbbdd0754b8fa8eab2393 Mon Sep 17 00:00:00 2001 From: Matt Hillsdon Date: Mon, 14 Apr 2025 16:55:45 +0100 Subject: [PATCH 1/7] fix: avoid insert then move Insert has the side effects of popping out blocks that isn't undone by moving to the next position. --- src/actions/enter.ts | 12 +++----- src/actions/mover.ts | 28 ++++++++++++++---- src/keyboard_drag_strategy.ts | 9 +++++- src/navigation.ts | 56 ++++++++++++++++++++++++----------- 4 files changed, 73 insertions(+), 32 deletions(-) diff --git a/src/actions/enter.ts b/src/actions/enter.ts index 2c7d7a4b..0650146d 100644 --- a/src/actions/enter.ts +++ b/src/actions/enter.ts @@ -135,14 +135,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 startConnection = stationaryNode + ? this.navigation.findBestInsertionConnection(stationaryNode, newBlock) + : null; if (workspace.getTopBlocks().includes(newBlock)) { this.positionNewTopLevelBlock(workspace, newBlock); } @@ -153,7 +149,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, startConnection); } /** diff --git a/src/actions/mover.ts b/src/actions/mover.ts index 4d2c40e9..73b80ae5 100644 --- a/src/actions/mover.ts +++ b/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, @@ -95,9 +102,14 @@ export class Mover { * Should only be called if canMove has returned true. * * @param workspace The workspace we might be moving on. + * @param startConnection The starting point for the move, if other than the current + * position. * @returns True iff a move has successfully begun. */ - startMove(workspace: WorkspaceSvg) { + startMove( + workspace: WorkspaceSvg, + startConnection: RenderedConnection | null = null, + ) { const cursor = workspace?.getCursor(); const block = this.getCurrentBlock(workspace); if (!cursor || !block) throw new Error('precondition failure'); @@ -107,7 +119,7 @@ export class Mover { cursor.setCurNode(ASTNode.createBlockNode(block)); this.patchWorkspace(workspace); - this.patchDragStrategy(block); + this.patchDragStrategy(block, startConnection); // Begin dragging block. const DraggerClass = registry.getClassFromOptions( registry.Type.BLOCK_DRAGGER, @@ -144,6 +156,7 @@ export class Mover { this.unpatchWorkspace(workspace); this.unpatchDragStrategy(info.block); this.moves.delete(workspace); + return true; } @@ -175,6 +188,7 @@ export class Mover { this.unpatchWorkspace(workspace); this.unpatchDragStrategy(info.block); this.moves.delete(workspace); + return true; } @@ -289,11 +303,15 @@ export class Mover { * Monkeypatch: replace the block's drag strategy and cache the old value. * * @param block The block to patch. + * @param initialConnection The starting connection. */ - private patchDragStrategy(block: BlockSvg) { + private patchDragStrategy( + block: BlockSvg, + initialConnection: RenderedConnection | null, + ) { // @ts-expect-error block.dragStrategy is private. this.oldDragStrategy = block.dragStrategy; - block.setDragStrategy(new KeyboardDragStrategy(block)); + block.setDragStrategy(new KeyboardDragStrategy(block, initialConnection)); } /** diff --git a/src/keyboard_drag_strategy.ts b/src/keyboard_drag_strategy.ts index c8fee5db..d4f5f8be 100644 --- a/src/keyboard_drag_strategy.ts +++ b/src/keyboard_drag_strategy.ts @@ -35,6 +35,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, + private startConnection: RenderedConnection | null, + ) { + super(block); + } + override startDrag(e?: PointerEvent) { super.startDrag(e); // Set position of the dragging block, so that it doesn't pop @@ -263,7 +270,7 @@ export class KeyboardDragStrategy extends dragging.BlockDragStrategy { */ private createInitialCandidate(): ConnectionCandidate | null { // @ts-expect-error startParentConn is private. - const neighbour = this.startParentConn; + const neighbour = this.startConnection ?? this.startParentConn; if (neighbour) { this.searchNode = ASTNode.createConnectionNode(neighbour); switch (neighbour.type) { diff --git a/src/navigation.ts b/src/navigation.ts index 72c112a0..0d514286 100644 --- a/src/navigation.ts +++ b/src/navigation.ts @@ -668,17 +668,17 @@ export class Navigation { * @returns True if the key was handled; false if something went * wrong. */ - tryToConnectBlock( + findBestInsertionConnection( stationaryNode: Blockly.ASTNode, movingBlock: Blockly.BlockSvg, - ): boolean { + ): Blockly.RenderedConnection | null { const stationaryType = stationaryNode.getType(); const stationaryLoc = stationaryNode.getLocation(); if (stationaryNode.getType() === Blockly.ASTNode.types.FIELD) { const sourceBlock = stationaryNode.getSourceBlock(); - if (!sourceBlock) return false; - return this.tryToConnectBlock( + if (!sourceBlock) return null; + return this.findBestInsertionConnection( Blockly.ASTNode.createBlockNode(sourceBlock), movingBlock, ); @@ -693,8 +693,8 @@ export class Navigation { stationaryAsConnection.type === Blockly.ConnectionType.INPUT_VALUE ) { const sourceBlock = stationaryNode.getSourceBlock(); - if (!sourceBlock) return false; - return this.tryToConnectBlock( + if (!sourceBlock) return null; + return this.findBestInsertionConnection( Blockly.ASTNode.createBlockNode(sourceBlock), movingBlock, ); @@ -702,9 +702,9 @@ export class Navigation { // 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; @@ -724,15 +724,12 @@ 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); + return stationaryBlock.nextConnection; } // 3. Output connection. This will wrap around or displace. @@ -743,17 +740,40 @@ export class Navigation { stationaryNode.getType() === Blockly.ASTNode.types.BLOCK ) { const parent = stationaryNode.getSourceBlock()?.getParent(); - if (!parent) return false; - return this.tryToConnectBlock( + if (!parent) return null; + return this.findBestInsertionConnection( 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 findBestInsertionConnection ${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.findBestInsertionConnection( + stationaryNode, + movingBlock, + ); + if (!destConnection) return false; + return this.insertBlock(movingBlock, destConnection); } /** From 94633bddd6cc9b299ab616dad60a743936f31aff Mon Sep 17 00:00:00 2001 From: Matt Hillsdon Date: Tue, 15 Apr 2025 14:06:15 +0100 Subject: [PATCH 2/7] feat: delete on abort undo is still multiple steps but will need Blockly input for that --- src/actions/enter.ts | 6 +++--- src/actions/mover.ts | 33 ++++++++++++++++++++++----------- src/keyboard_drag_strategy.ts | 4 ++-- src/navigation.ts | 22 ++++++++++------------ 4 files changed, 37 insertions(+), 28 deletions(-) diff --git a/src/actions/enter.ts b/src/actions/enter.ts index b2a6d434..17610bbb 100644 --- a/src/actions/enter.ts +++ b/src/actions/enter.ts @@ -132,8 +132,8 @@ export class EnterAction { const newBlock = this.createNewBlock(workspace); if (!newBlock) return; - const startConnection = stationaryNode - ? this.navigation.findBestInsertionConnection(stationaryNode, newBlock) + const insertStartPoint = stationaryNode + ? this.navigation.findInsertStartPoint(stationaryNode, newBlock) : null; if (workspace.getTopBlocks().includes(newBlock)) { this.positionNewTopLevelBlock(workspace, newBlock); @@ -145,7 +145,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, startConnection); + this.mover.startMove(workspace, insertStartPoint); } /** diff --git a/src/actions/mover.ts b/src/actions/mover.ts index 73b80ae5..a9db21bb 100644 --- a/src/actions/mover.ts +++ b/src/actions/mover.ts @@ -102,13 +102,13 @@ export class Mover { * Should only be called if canMove has returned true. * * @param workspace The workspace we might be moving on. - * @param startConnection The starting point for the move, if other than the current - * position. + * @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, - startConnection: RenderedConnection | null = null, + insertStartPoint: RenderedConnection | null = null, ) { const cursor = workspace?.getCursor(); const block = this.getCurrentBlock(workspace); @@ -119,7 +119,7 @@ export class Mover { cursor.setCurNode(ASTNode.createBlockNode(block)); this.patchWorkspace(workspace); - this.patchDragStrategy(block, startConnection); + this.patchDragStrategy(block, insertStartPoint); // Begin dragging block. const DraggerClass = registry.getClassFromOptions( registry.Type.BLOCK_DRAGGER, @@ -172,9 +172,20 @@ 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; + // If it's an insert-style move then we delete the block. + const keyboardDragStrategy = + info.block.getDragStrategy() as KeyboardDragStrategy; + const {insertStartPoint: startConnection} = keyboardDragStrategy; + + if (startConnection) { + // Monkey patch dragger to trigger delete. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (info.dragger as any).wouldDeleteDraggable = () => true; + } else { + // 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. @@ -182,7 +193,7 @@ export class Mover { blockSvg.dragStrategy.connectionPreviewer.hidePreview(); info.dragger.onDragEnd( info.fakePointerEvent('pointerup'), - new utils.Coordinate(0, 0), + info.startLocation, ); this.unpatchWorkspace(workspace); @@ -303,15 +314,15 @@ export class Mover { * Monkeypatch: replace the block's drag strategy and cache the old value. * * @param block The block to patch. - * @param initialConnection The starting connection. + * @param insertStartPoint The starting connection. */ private patchDragStrategy( block: BlockSvg, - initialConnection: RenderedConnection | null, + insertStartPoint: RenderedConnection | null, ) { // @ts-expect-error block.dragStrategy is private. this.oldDragStrategy = block.dragStrategy; - block.setDragStrategy(new KeyboardDragStrategy(block, initialConnection)); + block.setDragStrategy(new KeyboardDragStrategy(block, insertStartPoint)); } /** diff --git a/src/keyboard_drag_strategy.ts b/src/keyboard_drag_strategy.ts index 22fbcfdd..73973e19 100644 --- a/src/keyboard_drag_strategy.ts +++ b/src/keyboard_drag_strategy.ts @@ -37,7 +37,7 @@ export class KeyboardDragStrategy extends dragging.BlockDragStrategy { constructor( block: BlockSvg, - private startConnection: RenderedConnection | null, + public insertStartPoint: RenderedConnection | null, ) { super(block); } @@ -269,7 +269,7 @@ export class KeyboardDragStrategy extends dragging.BlockDragStrategy { */ private createInitialCandidate(): ConnectionCandidate | null { // @ts-expect-error startParentConn is private. - const neighbour = this.startConnection ?? this.startParentConn; + const neighbour = this.insertStartPoint ?? this.startParentConn; if (neighbour) { this.searchNode = ASTNode.createConnectionNode(neighbour); switch (neighbour.type) { diff --git a/src/navigation.ts b/src/navigation.ts index 554acf21..f9382ac3 100644 --- a/src/navigation.ts +++ b/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,15 +662,13 @@ 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. */ - findBestInsertionConnection( + findInsertStartPoint( stationaryNode: Blockly.ASTNode, movingBlock: Blockly.BlockSvg, ): Blockly.RenderedConnection | null { @@ -680,7 +678,7 @@ export class Navigation { if (stationaryNode.getType() === Blockly.ASTNode.types.FIELD) { const sourceBlock = stationaryNode.getSourceBlock(); if (!sourceBlock) return null; - return this.findBestInsertionConnection( + return this.findInsertStartPoint( Blockly.ASTNode.createBlockNode(sourceBlock), movingBlock, ); @@ -696,7 +694,7 @@ export class Navigation { ) { const sourceBlock = stationaryNode.getSourceBlock(); if (!sourceBlock) return null; - return this.findBestInsertionConnection( + return this.findInsertStartPoint( Blockly.ASTNode.createBlockNode(sourceBlock), movingBlock, ); @@ -743,7 +741,7 @@ export class Navigation { ) { const parent = stationaryNode.getSourceBlock()?.getParent(); if (!parent) return null; - return this.findBestInsertionConnection( + return this.findInsertStartPoint( Blockly.ASTNode.createBlockNode(parent), movingBlock, ); @@ -770,7 +768,7 @@ export class Navigation { stationaryNode: Blockly.ASTNode, movingBlock: Blockly.BlockSvg, ): boolean { - const destConnection = this.findBestInsertionConnection( + const destConnection = this.findInsertStartPoint( stationaryNode, movingBlock, ); From 674e87b7d33e1175b34bdea7c135db1a4c3eb968 Mon Sep 17 00:00:00 2001 From: Matt Hillsdon Date: Tue, 15 Apr 2025 14:09:25 +0100 Subject: [PATCH 3/7] chore: consistent local variable names --- src/actions/mover.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/actions/mover.ts b/src/actions/mover.ts index a9db21bb..56def01b 100644 --- a/src/actions/mover.ts +++ b/src/actions/mover.ts @@ -175,9 +175,9 @@ export class Mover { // If it's an insert-style move then we delete the block. const keyboardDragStrategy = info.block.getDragStrategy() as KeyboardDragStrategy; - const {insertStartPoint: startConnection} = keyboardDragStrategy; + const {insertStartPoint} = keyboardDragStrategy; - if (startConnection) { + if (insertStartPoint) { // Monkey patch dragger to trigger delete. // eslint-disable-next-line @typescript-eslint/no-explicit-any (info.dragger as any).wouldDeleteDraggable = () => true; From 31b15e67ffda91cdab6a0e27a6d5b378a9082e8b Mon Sep 17 00:00:00 2001 From: Matt Hillsdon Date: Tue, 15 Apr 2025 14:16:29 +0100 Subject: [PATCH 4/7] chore: reduce whitespace change --- src/actions/mover.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/actions/mover.ts b/src/actions/mover.ts index 56def01b..06c5a87a 100644 --- a/src/actions/mover.ts +++ b/src/actions/mover.ts @@ -199,7 +199,6 @@ export class Mover { this.unpatchWorkspace(workspace); this.unpatchDragStrategy(info.block); this.moves.delete(workspace); - return true; } From 19e02bf8b79f5d7241f6904006ddeb84b7194e39 Mon Sep 17 00:00:00 2001 From: Matt Hillsdon Date: Tue, 15 Apr 2025 14:19:34 +0100 Subject: [PATCH 5/7] chore: parameter docs --- src/actions/mover.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/actions/mover.ts b/src/actions/mover.ts index 06c5a87a..cb497d93 100644 --- a/src/actions/mover.ts +++ b/src/actions/mover.ts @@ -313,7 +313,8 @@ 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 connection. + * @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, From 2ecf843e87ad20bf9cf0200b7a0bdd930b678560 Mon Sep 17 00:00:00 2001 From: Matt Hillsdon Date: Tue, 15 Apr 2025 14:20:19 +0100 Subject: [PATCH 6/7] chore: fix warn message to have correct method name --- src/navigation.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/navigation.ts b/src/navigation.ts index f9382ac3..1687eb21 100644 --- a/src/navigation.ts +++ b/src/navigation.ts @@ -749,9 +749,7 @@ export class Navigation { return stationaryBlock.outputConnection; } } - this.warn( - `Unexpected case in findBestInsertionConnection ${stationaryType}.`, - ); + this.warn(`Unexpected case in findInsertStartPoint ${stationaryType}.`); return null; } From bdf5e0597009d2ce32e9a443efdd9a1e27410ec0 Mon Sep 17 00:00:00 2001 From: Matt Hillsdon Date: Tue, 15 Apr 2025 14:54:53 +0100 Subject: [PATCH 7/7] fix: just delete one block on insert abort --- src/actions/mover.ts | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/actions/mover.ts b/src/actions/mover.ts index cb497d93..84645237 100644 --- a/src/actions/mover.ts +++ b/src/actions/mover.ts @@ -177,11 +177,10 @@ export class Mover { info.block.getDragStrategy() as KeyboardDragStrategy; const {insertStartPoint} = keyboardDragStrategy; - if (insertStartPoint) { - // Monkey patch dragger to trigger delete. - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (info.dragger as any).wouldDeleteDraggable = () => true; - } else { + // Monkey patch dragger to trigger delete. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (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; @@ -191,6 +190,10 @@ export class Mover { // 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'), info.startLocation,