fix: Make cut/copy/paste work consistently and as expected #9107
Conversation
maribethb
left a comment
maribethb
left a comment
There was a problem hiding this comment.
Generally I think this makes sense but it would be nice if there were more test cases specifically covering the kinds of blocks that weren't copyable before this and now are.
lgtm pending question about editable though.
|
Sorry, just merged my change so you got the merge conflict, please be sure to add the ephemeralFocusTaken check as well. Thanks! |
cpcallen
left a comment
cpcallen
left a comment
There was a problem hiding this comment.
There are a number of significant issues with this PR, including making a change that we had decided not to make as well as breaking copying of workspace comments.
I think it should be (mostly) reverted, and a different stab taken at making it possible to copy blocks from the workspace, by ascertaining what change had made them uncopybable and addressing that as narrowly as possible. Further increasing the range of things that can be copied should be left until we decide we want to implement the proposed changes described in #9098 (or a comparable alternative approach).
| * | ||
| * @param focused The focused object. | ||
| */ | ||
| function isCuttable(focused: IFocusableNode): boolean { |
There was a problem hiding this comment.
The reason I created isCopyable, above, was to ensure that cut and copy used common criteria for deciding if something can be cut/copied, with cut being allowed for anything that can be copied and is additionally deletable—AFAICT there is no circumstance where it should be possible to cut something but not copy it.
So isCuttable should call isCopyable (not just isICopyable), and return false if it does. Or… we could just have the cut preconditionFn just check isCopyable(focused) && focused.isDeletable(), as it previously did.
| return ( | ||
| isICopyable(focused) && | ||
| isIDeletable(focused) && | ||
| focused.isOwnDeletable() && | ||
| isDraggable(focused) && | ||
| focused.isOwnMovable() | ||
| ); |
There was a problem hiding this comment.
This change makes things that we decided we did not want to make copyable copyable.
This includes shadow blocks, but also blocks that have custom isDeletable and isMovable methods.
While I was working on #9084 we decided we did not want to do that in v12.
I'm not necessarily averse to making some things copyable (which were not previously copyable), but It is in my opinion a mistake to have done so without introducing an isCopyable method on ICopyable so that block definition authors can have more fine-grained control.
| name: names.COPY, | ||
| preconditionFn(workspace, scope) { | ||
| const focused = scope.focusedNode; | ||
| if (!(focused instanceof BlockSvg)) return false; |
There was a problem hiding this comment.
This prevents anything except blocks—e.g., workspace comments—from being copied.
| isCopyable(focused) && | ||
| !getFocusManager().ephemeralFocusTaken() | ||
| !!targetWorkspace && | ||
| !targetWorkspace.isReadOnly() && |
There was a problem hiding this comment.
It's not clear to me that the main workspace being readonly should prevent blocks from being copied from the flyout.
(It also isn't clear to me that it should prevent blocks from being copied from the main workspace either—but that was the previous behaviour so leaving it as-is for the time being is fine.)
3 participants
The basics
The details
Resolves
Fixes RaspberryPiFoundation/blockly-keyboard-experimentation#546
This is a dependency for RaspberryPiFoundation/blockly-keyboard-experimentation#556
Proposed Changes
Updates the preconditions for cut/copy/paste to be more aware of flyouts and to use the following criteria:
Reason for Changes
You couldn't copy a block in the toolbox and then paste it into the workspace and when paste was enabled was inconsistent.
Test Coverage
Documentation
Additional Information