feat: Introduce `Mover` actions + experimental `DragMover` implementation #317

cpcallen · 2025-03-13T13:47:59Z

Introduces a Mover class which:

Creates actions ~~(for now only keyboard shortcuts)~~ to start a move, move in cursor-key directions, and end or abort a move.
Implements a mode for when a block move is taking place. This is done using only Mover-internal state. When in moving mode, the arrow keys are hooked to trigger block moving actions rather than their normal cursor moving actions.
When a move is initiated, saves information about the original location of the block, so that the block can be restored to that location if the move is aborted.
Includes a framework for—but does not yet implement—constrained and unconstrained movement.

Additionally, introduces a DragMover subclass that provides an experimental implementation of unconstrained block movement using Dragger.

To try this out, move the cursor to a movable block, press m, then use alt+arrows to move the block. Press enter to finish the move, or esc to abort it.

Moving using the dragger sort of works, with a few known issues:

Blocks being moved lurch around somewhat wildly, especially when they are near compatible connectors.
Sometimes blocks not being moved also move around.
~~Moving top-level blocks works reasonably well, but attempting to move a non-top-level block will result in it being deleted unless the move is aborted.~~ FIXED!

The purpose of separating this experimental implementation into a separate subclass is that it is anticipated that it may well be thrown away, both because of the above noted issues and because this approach may be fundamentally incompatible with constrained movement. Specifically: constrained movement should first concern itself with where the moving block(s) might be connected, and only then decide where to position them on screen (e.g. so that they are very near but not obscuring an insertion marker)—whereas dragging begins with the position of the block and from that decides where the block might connect.

Part of #106.

Introduce the Mover class, with shortcuts that will put a workspace into (and exit from) moving mode. While in moving mode, cursor navigation via the cursor keys is disabled.

Create an experimental implementation of unconstrained dragging using the existing Dragger class. Implement this on a new Mover subclass DragMover, to keep this experimental code (that will probably be thrown away) separate from the main Mover implementation. Known bugs: * Blocks lurch around somewhat wildly while being moved. * Moving top-level blocks works reasonably well, but attempting to move a non-top-level block will result in it being deleted unless the move is aborted.

microbit-matt-hillsdon · 2025-03-13T14:03:28Z

Deployed here so I could play with it easily (won't automatically update for subsequent pushes)

kmcnaught · 2025-03-13T14:41:48Z

Just to note that Alt+left arrow takes me back to the previous page in Chrome on Windows 11. (we get away with Alt+right arrow for testing as there isn't usually a subsequent page, though of course there could be)

cpcallen · 2025-03-13T16:19:42Z

Just to note that Alt+left arrow takes me back to the previous page in Chrome on Windows 11. (we get away with Alt+right arrow for testing as there isn't usually a subsequent page, though of course there could be)

I had originally proposed using shift for the unconstrained movement modifier, but you (IMHO quite correctly) objected to this on the grounds that it should be reserved for doing mutiselect. If alt also doesn't work then we are quickly running out of modifiers. For now I will make both ctrl and alt usable (ctrl won't work for macOS users who use Spaces with default shortcut keys), and add a note to #106 that we need to figure out what to do instead.

Alt+arrows causes browser navigation on Windows. :-(

cpcallen · 2025-03-13T16:30:46Z

Now provides a context menu item (on blocks) for the move action, and supports ctrl+arrow keys as an alternative to alt+arrow keys.

There was a bug that would often cause blocks to be deleted. This was because the dragger thought the block was on the toolbox, which is a delete area. Fix this by giving the dragger much better fake PointerEvents, so that it knows where the block actually is - good enough that you can move the block to the trash can and it will be deleted!

cpcallen · 2025-03-13T17:05:27Z

Fixed issue with blocks being deleted when the move finished. The fix was to provide better fake PointerEvents to the dragger. A nice side-effect of the fix is that moving a block to the trash will now delete it! (You have to move it so that the upper-left corner of the block overlaps the trash can icon).

The blocks still lurch around a bit, and the implementation is kind of janky (don't look at what happens when you abort a move!), but it is absolutley good enough to be useful now, so I'm marking this PR as ready for review.

gonfunko · 2025-03-13T20:01:42Z

I notice that if you move a block such that an insertion marker appears, and abort the move at that point, you get the following stack trace:

Failed to execute 'insertBefore' on 'Node': The node before which the new node is to be inserted is not a child of this node.
NotFoundError: Failed to execute 'insertBefore' on 'Node': The node before which the new node is to be inserted is not a child of this node.
    at BlockSvg$$module$build$src$core$block_svg.setParent (webpack-internal:///./node_modules/blockly/blockly_compressed.js:1246:28)
    at RenderedConnection$$module$build$src$core$rendered_connection.disconnectInternal (webpack-internal:///./node_modules/blockly/blockly_compressed.js:983:430)
    at RenderedConnection$$module$build$src$core$rendered_connection.disconnectInternal (webpack-internal:///./node_modules/blockly/blockly_compressed.js:1238:479)
    at RenderedConnection$$module$build$src$core$rendered_connection.disconnect (webpack-internal:///./node_modules/blockly/blockly_compressed.js:982:323)
    at BlockSvg$$module$build$src$core$block_svg.unplugFromStack (webpack-internal:///./node_modules/blockly/blockly_compressed.js:1000:210)
    at BlockSvg$$module$build$src$core$block_svg.unplug (webpack-internal:///./node_modules/blockly/blockly_compressed.js:998:417)
    at InsertionMarkerPreviewer$$module$build$src$core$insertion_marker_previewer.hideInsertionMarker (webpack-internal:///./node_modules/blockly/blockly_compressed.js:1410:264)
    at InsertionMarkerPreviewer$$module$build$src$core$insertion_marker_previewer.hidePreview (webpack-internal:///./node_modules/blockly/blockly_compressed.js:1409:486)
    at BlockDragStrategy$$module$build$src$core$dragging$block_drag_strategy.revertDrag (webpack-internal:///./node_modules/blockly/blockly_compressed.js:1223:140)
    at BlockSvg$$module$build$src$core$block_svg.revertDrag (webpack-internal:///./node_modules/blockly/blockly_compressed.js:1272:444)

And wind up with an orphaned insertion marker in some cases.

gonfunko · 2025-03-13T20:04:09Z

src/navigation_controller.ts

+  hasNavigationFocus: boolean = false;
+


I think this was inadvertently introduced resolving a merge conflict?

gonfunko · 2025-03-13T20:04:27Z

src/navigation_controller.ts

-      ShortcutRegistry.registry.unregister(shortcut.name);
-    }
-
+    this.mover.install();


uninstall()?

gonfunko · 2025-03-13T20:09:56Z

src/actions/mover.ts

+
+  constructor(
+    protected navigation: Navigation,
+    canEdit: (ws: WorkspaceSvg) => boolean,


canEdit could also just be declared protected here for consistency

gonfunko · 2025-03-13T20:12:36Z

src/actions/mover.ts

+   * given workspace.
+   *
+   * @param workspace The workspace to move on.
+   * @returns True iff we can beign a move.


gonfunko · 2025-03-13T20:14:16Z

src/actions/mover.ts

+    if (!block) {
+      const cursor = workspace?.getCursor();
+      const curNode = cursor?.getCurNode();
+      block = curNode?.getSourceBlock() ?? undefined;
+    }


Might be worth factoring this out into a getCurrentBlock() since startMove also appears to need it

gonfunko · 2025-03-13T20:14:49Z

src/actions/mover.ts

+    cursor.setCurNode(ASTNode.createBlockNode(block)!);
+
+    // Additional implementation goes here.
+    console.log('startMove');


Remove? Applies throughout, assuming this was just debugging logging.

gonfunko · 2025-03-13T20:15:25Z

src/actions/mover.ts

+   * @returns True iff move successfully finished.
+   */
+  finishMove(workspace: WorkspaceSvg) {
+    if (!workspace) return false;


Make the workspace arg to this function nullable or remove this null check, same for abortMove. Or perhaps unify them since they're currently identical?

gonfunko · 2025-03-13T20:20:05Z

src/actions/drag_mover.ts

+   * @returns True iff move successfully finished.
+   */
+  override finishMove(workspace: WorkspaceSvg) {
+    if (!workspace) return false;


As in the base class, either remove the null check or make the arg nullable here and in abortMove

…tion (#352) * feat: Introduce Mover class, moving mode Introduce the Mover class, with shortcuts that will put a workspace into (and exit from) moving mode. While in moving mode, cursor navigation via the cursor keys is disabled. * feat: Save block starting location and connections * feat: Experiment: unconstrained movement using Dragger Create an experimental implementation of unconstrained dragging using the existing Dragger class. Implement this on a new Mover subclass DragMover, to keep this experimental code (that will probably be thrown away) separate from the main Mover implementation. Known bugs: * Blocks lurch around somewhat wildly while being moved. * Moving top-level blocks works reasonably well, but attempting to move a non-top-level block will result in it being deleted unless the move is aborted. * feat: Add Move Block (M) context menu item * fix: Allow ctrl+arrows to also perform unconstrained moves Alt+arrows causes browser navigation on Windows. :-( * fix: Use better fake PointerEvents to prevent random deletions There was a bug that would often cause blocks to be deleted. This was because the dragger thought the block was on the toolbox, which is a delete area. Fix this by giving the dragger much better fake PointerEvents, so that it knows where the block actually is - good enough that you can move the block to the trash can and it will be deleted! * chore: address feedback from #317 * chore: lint and format * fix: remove allowCollision for m * fix: #356 by hiding the connection preview before reverting a drag * fix: spelling --------- Co-authored-by: Christopher Allen <cpcallen+git@google.com>

rachel-fenichel · 2025-03-31T16:36:50Z

Closed in favor of #352

…tion (RaspberryPiFoundation#352) * feat: Introduce Mover class, moving mode Introduce the Mover class, with shortcuts that will put a workspace into (and exit from) moving mode. While in moving mode, cursor navigation via the cursor keys is disabled. * feat: Save block starting location and connections * feat: Experiment: unconstrained movement using Dragger Create an experimental implementation of unconstrained dragging using the existing Dragger class. Implement this on a new Mover subclass DragMover, to keep this experimental code (that will probably be thrown away) separate from the main Mover implementation. Known bugs: * Blocks lurch around somewhat wildly while being moved. * Moving top-level blocks works reasonably well, but attempting to move a non-top-level block will result in it being deleted unless the move is aborted. * feat: Add Move Block (M) context menu item * fix: Allow ctrl+arrows to also perform unconstrained moves Alt+arrows causes browser navigation on Windows. :-( * fix: Use better fake PointerEvents to prevent random deletions There was a bug that would often cause blocks to be deleted. This was because the dragger thought the block was on the toolbox, which is a delete area. Fix this by giving the dragger much better fake PointerEvents, so that it knows where the block actually is - good enough that you can move the block to the trash can and it will be deleted! * chore: address feedback from RaspberryPiFoundation#317 * chore: lint and format * fix: remove allowCollision for m * fix: RaspberryPiFoundation#356 by hiding the connection preview before reverting a drag * fix: spelling --------- Co-authored-by: Christopher Allen <cpcallen+git@google.com> feat: heuristic insert (RaspberryPiFoundation#355) * chore: simplify tryToConnectNodes We only insert blocks so remove other cases. * feat: heurstic insertion logic - use first statement or value input - in statement inputs move to the last next connection in the chain - use next connections over previous connections chore: convert webdriverio tests to TypeScript (RaspberryPiFoundation#353) * chore: convert webdriverio tests to TypeScript * chore: comment typo fix feat: allow for toolbox with multiple tab stops (RaspberryPiFoundation#358) - Switch to focusin/out for toolbox (maybe in future elsewhere) - Drop the tab index fiddling as it's not practical to get it right in the face of more than one tab stop in the toolbox, especially if that toolbox has a DOM structure not known to the plugin. Given that we're reinstating the ability to shift-tab flyout->toolbox it makes sense to allow tab to the flyout. Shift-tab to the flyout remains not possible in the auto-hide case but that's not unreasonable. This version can be made to work with MakeCode without further modifications. feat: use ts and factor out helper functions

cpcallen added this to Blockly 2025 Mar 13, 2025

cpcallen moved this to In Progress in Blockly 2025 Mar 13, 2025

cpcallen added 3 commits March 13, 2025 13:50

feat: Introduce Mover class, moving mode

cda9d83

Introduce the Mover class, with shortcuts that will put a workspace into (and exit from) moving mode. While in moving mode, cursor navigation via the cursor keys is disabled.

feat: Save block starting location and connections

9b2b06e

cpcallen force-pushed the feat/106 branch from 9e65e16 to 746685c Compare March 13, 2025 13:51

feat: Add Move Block (M) context menu item

7fd74a4

cpcallen mentioned this pull request Mar 13, 2025

Make it possible to move blocks on the workspace #106

Closed

8 tasks

fix: Allow ctrl+arrows to also perform unconstrained moves

b83bcf3

Alt+arrows causes browser navigation on Windows. :-(

cpcallen marked this pull request as ready for review March 13, 2025 17:05

cpcallen requested a review from a team as a code owner March 13, 2025 17:05

cpcallen requested review from gonfunko and removed request for a team March 13, 2025 17:05

gonfunko reviewed Mar 13, 2025

View reviewed changes

gonfunko self-assigned this Mar 13, 2025

rachel-fenichel mentioned this pull request Mar 29, 2025

feat: introduce Mover actions + experimental DragMover implementation #352

Merged

rachel-fenichel closed this Mar 31, 2025

github-project-automation bot moved this from In Progress to Done in Blockly 2025 Mar 31, 2025

cpcallen mentioned this pull request Apr 24, 2025

Consider splitting Enter into multiple actions #312

Open

cpcallen deleted the feat/106 branch May 22, 2025 12:56

cpcallen mentioned this pull request Jun 17, 2025

fix(mover): Pass drag delta to dragger.onDrag in pixels #599

Merged

feat: Introduce Mover actions + experimental DragMover implementation #317

feat: Introduce Mover actions + experimental DragMover implementation #317

Uh oh!

Conversation

cpcallen commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

microbit-matt-hillsdon commented Mar 13, 2025

Uh oh!

kmcnaught commented Mar 13, 2025

Uh oh!

cpcallen commented Mar 13, 2025

Uh oh!

cpcallen commented Mar 13, 2025

Uh oh!

cpcallen commented Mar 13, 2025

Uh oh!

gonfunko commented Mar 13, 2025

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 Mar 31, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

feat: Introduce `Mover` actions + experimental `DragMover` implementation #317

feat: Introduce `Mover` actions + experimental `DragMover` implementation #317

cpcallen commented Mar 13, 2025 •

edited

Loading