Skip to content

Conversation

@microbit-matt-hillsdon
Copy link
Contributor

@microbit-matt-hillsdon microbit-matt-hillsdon commented Jun 30, 2025

Fixes #623

The implementation conflated having no insertStartPosition with the block being new. But when the workspace is selected then there is no insert start position even if the block is new. Instead track whether we're inserting or just moving separately.

Demo: https://fix-ws-insert-cancel.blockly-keyboard-experimentation.pages.dev/

The workspace selection case is broken
@microbit-matt-hillsdon microbit-matt-hillsdon requested a review from a team as a code owner June 30, 2025 15:03
@microbit-matt-hillsdon microbit-matt-hillsdon requested review from RoboErikG and removed request for a team June 30, 2025 15:03
@microbit-matt-hillsdon
Copy link
Contributor Author

Fixed conflicts (just test imports).

@microbit-matt-hillsdon
Copy link
Contributor Author

This will conflict with #626.

My preference is for you to merge the approved #626 first and then I'll fix the conflicts here again which I've already done once on an integration branch.

@maribethb
Copy link
Collaborator

This will conflict with #626.

My preference is for you to merge the approved #626 first and then I'll fix the conflicts here again which I've already done once on an integration branch.

This is #626, which PR did you mean to comment this on and which one did you want us to actually merge first?

@microbit-matt-hillsdon
Copy link
Contributor Author

This is #626, which PR did you mean to comment this on and which one did you want us to actually merge first?

Whoops! #625 I think, which also changes the mover code. Happy either way, just trying to save effort.

Copy link
Contributor

@RoboErikG RoboErikG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it and I see #625 is already in. Sorry for the delay, I was out last week. I can merge once the conflicts are resolved.

This looks good for kb-exp, though I'm going to file a bug for us to rethink the mover APIs before this gets merged into Core as this feels a bit messy. Let us know if you have any suggestions!

@microbit-matt-hillsdon
Copy link
Contributor Author

Something else to ponder at the same time is scenarios like #635.

Conflicts fixed, please consider this an unsolicited ad for git rerere.

➜  blockly-keyboard-experimentation git:(fix-ws-insert-cancel) ✗ git merge main
...
Resolved 'src/actions/move.ts' using previous resolution.
Resolved 'src/actions/mover.ts' using previous resolution.
...

@RoboErikG RoboErikG mentioned this pull request Jul 7, 2025
1 task
@RoboErikG RoboErikG merged commit f08551a into RaspberryPiFoundation:main Jul 7, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Insert block then cancel via escape (during move) doesn't remove the block if the workspace was selected

3 participants