feat: improved insertion flow #455

rachel-fenichel · 2025-04-19T07:26:53Z

Fixes #421

Based on #438

This is intended to be reviewed one commit at a time; each commit is small.

Behaviour after change

Connecting a block to the target location happens after the move starts.
- This avoids the problem identified in Unexpected 'pop out' of conditional / logic block #421, where blocks were unexpectedly getting popped out onto the workspace at the start of drags.
- Aborting a drag with Escape deletes the newly inserted block.
- Aborting a drag with Escape puts the cursor at the last target connection, (if any).
- Undo after insertion deletes the inserted block
- Inserting around a value block happens correctly

Additional information

This is largely the change that @microbit-matt-hillsdon made, rebased onto main.

I had a grand plan to do all the work to find the insertStartPoint in KeyboardDragStrategy instead of having to pass it into the drag strategy. That's the reason that I started by passing the block and isNewBlock into the drag strategy. This idea failed because getStationaryNode must be called before creating the new block, to avoid updating the cursor. I could pass the stationary node into the drag strategy instead of the start point, but in both cases I'm doing layer bypass to move information around. So that was a dud.

A few changes:

Pass the moving block into startMove, instead of expecting the mover to find it by looking at the cursor.
Make the keyboard drag strategy responsible for selecting the moving block, rather than startMove.
Make paired helpers to patch/unpatch the dragger. This is mostly so that we can see what we need to clean up later.
- (Unfortunately, as I typed this I realized that the dragger is created new for every drag, so there is no point in unpatching it at the end).
Set the cursor correctly after aborting an insertion 4da9e6d
Fix event grouping so that undo deletes the newly inserted block 565031a
Handle the issue when a value block is selected f648b46

microbit-matt-hillsdon · 2025-04-21T18:12:52Z

'M' then 'Escape' on an existing statement block now moves the cursor between that block and the previous block which is a regression. Even weirder if you move it before cancelling as the connection location is based on where the preview was shown not where it moves back to on cancel.

For a value block (e.g. test in if) selection seems to be lost entirely in the same scenario (or at least is not visible).

BenHenning · 2025-04-22T01:23:04Z

Sorry, will need to take a look at this tomorrow. There's a lot of context that I don't know here that I'll need to familiarize myself with in order to do an effective review.

Feel free to redirect to someone who has more context if that's preferable, but otherwise I will take a pass tomorrow (likely in the early afternoon PT).

microbit-matt-hillsdon

Keen to get this one merged. It looks good from my perspective except for the one note I added previously re cancelling a move of a preexisting block. I've added a suggested fix.

@rachel-fenichel there's also your note in the PR description about the lack of need to unpatch the dragger as it's recreated. So that's potentially some easy simplification.

microbit-matt-hillsdon · 2025-04-24T13:06:22Z

src/actions/mover.ts

+      info.startLocation,
    );

+    if (target) {


This seems a straightforward fix for my feedback:

Suggested change

if (target) {

if (dragStrategy.isNewBlock && target) {

If we go ahead with removing connection positions then the code in the block will likely need a tweak too, but let's get this in ahead of thinking about that.

BenHenning

Apologies for the delayed review. I had a few comments, but they're largely nits so going ahead and approving.

The solutions make sense to me, though admittedly I don't grok the complete dragging context. :)

BenHenning · 2025-04-25T00:53:09Z

src/actions/enter.ts

    workspace.setResizesEnabled(false);
-    Events.setGroup(true);
+    const existingGroup = Events.getGroup();
+    if (!existingGroup) {


Maybe add a line comment here explaining why this is important (otherwise the context feels maybe a bit hidden).

Added a short comment. This is also a pattern in all of Blockly.

BenHenning · 2025-04-25T00:59:18Z

src/navigation.ts

+   *
+   * @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


Is this correct? Seems like it should be tied to whether the connection was successful (per insertBlock's docs), not anything about keys.

BenHenning · 2025-04-25T01:05:38Z

src/actions/move.ts

-      callback: (workspace) => this.mover.startMove(workspace),
+      preconditionFn: (workspace) => {
+        const startBlock = this.getCurrentBlock(workspace);
+        return !!startBlock && this.mover.canMove(workspace, startBlock);


Why not just startBlock && ...? '!!' seems unnecessary here. Ditto below in a few places.

Needs to return a boolean, not null, and the compiler yelled.

BenHenning · 2025-04-25T01:09:29Z

src/actions/move.ts

+    const curNode = workspace?.getCursor()?.getCurNode();
+    let block = curNode?.getSourceBlock();
+    if (!block) return undefined;
+    while (block && block.isShadow()) {


I'm assuming the truthy check for block here is because TypeScript doesn't realize that it's impossible for it to ever be null or undefined?

If it's null inference is actually good enough to detect that, you should be able to drop that part of the condition.

BenHenning · 2025-04-25T01:10:39Z

src/keyboard_drag_strategy.ts

  override startDrag(e?: PointerEvent) {
+    if (!this.cursor) throw new Error('precondition failure');
+    // Select and focus block.
+    common.setSelected(this.block);


Won't setCurNode guarantee selection? It'd be nice to consolidate since otherwise every setSelected() needs to be reconsidered for focus.

Removed setSelected

BenHenning · 2025-04-25T01:11:27Z

src/keyboard_drag_strategy.ts

  ): ConnectionCandidate | null {
-    const cursor = draggingBlock.workspace.getCursor();
-    if (!cursor) return null;
+    // TODO: Handle the case where the cursor is null, or never return null


This seems worthwhile to maybe file an issue for and point to it.

Filed #471 and removed the comment here because it's a general problem as described by the issue, this is just an instance of it.

rachel-fenichel · 2025-04-25T20:29:34Z

@gonfunko please take a look--I fixed the issues Ben identified + accepted Matt's suggested patch. I think the tests failing are the ones that Erik is currently working on/investigating.

rachel-fenichel · 2025-04-25T22:07:23Z

Retested manually and with automated tests after RaspberryPiFoundation/blockly#8933 and it still works, so I'm merging now.

rachel-fenichel requested a review from a team as a code owner April 19, 2025 07:26

rachel-fenichel requested review from BenHenning and removed request for a team April 19, 2025 07:26

rachel-fenichel force-pushed the clean-insert-then-move branch from 565031a to 2cd4149 Compare April 19, 2025 07:29

microbit-matt-hillsdon mentioned this pull request Apr 20, 2025

feat: avoid insert then move; delete on abort #438

Closed

2 tasks

rachel-fenichel assigned BenHenning Apr 21, 2025

microbit-matt-hillsdon reviewed Apr 24, 2025

View reviewed changes

BenHenning approved these changes Apr 25, 2025

View reviewed changes

BenHenning assigned rachel-fenichel and unassigned BenHenning Apr 25, 2025

rachel-fenichel force-pushed the clean-insert-then-move branch from 2cd4149 to cb61c52 Compare April 25, 2025 20:27

rachel-fenichel requested a review from gonfunko April 25, 2025 20:28

rachel-fenichel assigned gonfunko and unassigned rachel-fenichel Apr 25, 2025

rachel-fenichel added 12 commits April 25, 2025 14:08

refactor: create findInsertStartPoint

e02058a

refactor: pass Navigation into the keyboard drag strategy

80ba31a

refactor: pass block into startMove

ee73e71

refactor: set selected and curNode in startDrag

ea28013

feat: wire isNewBlock into keyboard drag strategy

1d73f80

feat: monkeypatch dragger as needed to delete on aborted insertion

1923d3c

feat: pass insertStartPoint into the drag strategy after all

246a14d

feat: make insertion wrap a block immediately

8654f66

fix: undo deletes the newly insertedblock

a25cebe

fix: reference to getCurrentBlock

44bc613

chore: fix review feedback

67fd4c2

chore: remove unnecessary unpatch method

ec99e35

rachel-fenichel force-pushed the clean-insert-then-move branch from cb61c52 to ec99e35 Compare April 25, 2025 21:17

gonfunko approved these changes Apr 25, 2025

View reviewed changes

rachel-fenichel merged commit cedde35 into RaspberryPiFoundation:main Apr 25, 2025
6 of 8 checks passed

feat: improved insertion flow #455

feat: improved insertion flow #455

Uh oh!

Conversation

rachel-fenichel commented Apr 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Behaviour after change

Additional information

Uh oh!

microbit-matt-hillsdon commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BenHenning commented Apr 22, 2025

Uh oh!

microbit-matt-hillsdon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenHenning left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rachel-fenichel commented Apr 25, 2025

Uh oh!

rachel-fenichel commented Apr 25, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rachel-fenichel commented Apr 19, 2025 •

edited

Loading

microbit-matt-hillsdon commented Apr 21, 2025 •

edited

Loading