-
Notifications
You must be signed in to change notification settings - Fork 13
feat: Commit moves before running keyboard shortcuts. #564
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
rachel-fenichel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add this test:
- start a move with keys
- press an arrow key twice
- press
c - press an arrow key twice
- check that the block is in the expected location
| utils.KeyCodes.LEFT, | ||
| utils.KeyCodes.UP, | ||
| utils.KeyCodes.DOWN, | ||
| utils.KeyCodes.ENTER, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also needs KeyCodes.SPACE from the finish move action alongside ENTER.
Potentially these could be pulled from a list of the move actions from move.ts? Would be helpful in future as we hope to add UI with the ability to rebind shortcuts (though these ones are perhaps less likely to change than some).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And we do anticipate that when all of the keyboard nav code makes it into core, the shortcut actions will directly cancel moves. This setup is necessary because some shortcuts are in core and some are in the plugin. We will keep UI rebinding in mind for design work on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't actually need space, because this shortcut finishes the move itself; Enter is there to support command-enter to bring up the contextual menu, not to commit the move. But we do need Escape to abort the move, so I added that in (and cleaned up this shortcut in the new common cleanup method introduced in #561).
|
Would be great to get #561 in first and clean up this new shortcut in the new common clean-up method, as presumably it should be cleaned up on abort too. Or call in both places here and I can fix conflicts with the other PR. |
rachel-fenichel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, and I'm okay with merging as-is since this is a blocker.
And a tag for RaspberryPiFoundation/blockly#8915 in case you don't get to writing that test today.
This PR fixes #534 by registering a keyboard shortcut under the key combos of all existing keyboard shortcuts that commits the in-progress move before returning control to the corresponding real keyboard shortcut. To fully function, this depends on RaspberryPiFoundation/blockly#9109; without it, moves will be commited, but the underlying/real shortcut action may not run.