Issues with copy/paste behaviour

[LappiLappland]

Check for duplicates

• I have searched for similar issues before opening a new one.

Description

Several issues with copy/paste

1. Error when copying blocks from different workspaces
2. Error when copying blocks from mutator (if we consider first fixed)
3. Blocks from mutator get pasted onto main workspace, while Blockly's default behaviour is to paste them inside respected mutator workspace (if we consider first and second fixed)
4. Blocks from mutator workspace get saved in the storage. This allows to paste procedures_mutatorarg onto main workspace breaking project's import and code generation behaviour
5. Missing "paste into view" logic for paste by context menu compared to paste by hotkey

Reproduction steps

Videos show reproduction steps

Stack trace

issue 1
Cannot read properties of null (reading 'getParent')
TypeError: Cannot read properties of null (reading 'getParent')
    at eval (webpack-internal:///./src/multiselect_shortcut.js:180:35)
    at Array.forEach (<anonymous>)
    at Object.callback (webpack-internal:///./src/multiselect_shortcut.js:178:17)
    at ShortcutRegistry$$module$build$src$core$shortcut_registry.onKeyDown (webpack-internal:///./node_modules/blockly/blockly_compressed.js:1290:193)
    at onKeyDown$$module$build$src$core$inject (webpack-internal:///./node_modules/blockly/blockly_compressed.js:424:269)
    at HTMLDivElement.f (webpack-internal:///./node_modules/blockly/blockly_compressed.js:86:159)

issue 2
Cannot read properties of undefined (reading 'has')
TypeError: Cannot read properties of undefined (reading 'has')
    at hasSelectedParent (webpack-internal:///./src/global.js:108:59)
    at Object.check (webpack-internal:///./src/multiselect_shortcut.js:151:72)
    at Object.preconditionFn (webpack-internal:///./src/multiselect_shortcut.js:138:29)
    at ShortcutRegistry$$module$build$src$core$shortcut_registry.onKeyDown (webpack-internal:///./node_modules/blockly/blockly_compressed.js:1299:71)
    at onKeyDown$$module$build$src$core$inject (webpack-internal:///./node_modules/blockly/blockly_compressed.js:432:269)
    at HTMLDivElement.f (webpack-internal:///./node_modules/blockly/blockly_compressed.js:97:159)

Screenshots

Issue 1

issue.1.mp4

issue 2

issue.2.mp4

issue 3 and 4

issue.3.and.4.mp4

issue 5

issue.5.mp4

Browsers

No response

[LappiLappland]

I could maybe work on these issues, but I need to know a few details:

1. Should it even be possible to multiselect from 2 different workspaces like that? My selection in some cases was getting dropped, so I assume it's a bug
2. What's the purpose of if (data.source) { workspace = data.source; }? I don't think the key source exists in simple Blockly. Is this some plugin-specific?
3. Copy/paste from both shortcut and context menu callbacks should technically be almost identical (if not completely). Is it ok to move them into shared functions?
4. I would suggest not saving anything from mutator workspaces into storage

[mjgallag]

@LappiLappland that would be great if you want to give it a try. Perhaps you can start with just issue 1 above? If you take a look at all the tests I wrote you'll see that the typical pattern is to write a test to establish what single select behavior is followed by a failing test for what you think the multiselect behavior should be. If you want to start a draft pr with just the tests we can go back and forth and make sure we are on the same page as to what the solution should be first. Don't be shy with questions. Thanks!

[mjgallag]

Thanks for finding these bugs @LappiLappland! Some of these I may just fix shortly in #133 (Blockly 12) if the fixes are relatively easy to avoid a merge from main (Blockly 11) since 12 will hopefully be merging late spring/early summer when it lands in App Inventor. I'll reply here when done and it'd be great if you could then retest in the 12 branch and we can see what is left to fix.

[mjgallag]

@LappiLappland see cd0718e that should fix at least some of the issues you reported above. For now I decided just to not have multiselect inside mutator as that was much simpler to implement and it is not too useful in there anyway. But now copy/paste works inside mutator the same as in vanilla blockly. Thanks for finding these little bugs it seems they have been around a while, maybe since beginning of plugin.

[LappiLappland]

@mjgallag I see, thanks for taking care of the issues! I've taken a look at your cd0718e and noticed one issue:

copy block inside mutator, close mutator, paste block

Cannot create a rendered block in a headless workspace
TypeError: Cannot create a rendered block in a headless workspace
    at new BlockSvg$$module$build$src$core$block_svg (webpack-internal:///./node_modules/blockly/blockly_compressed.js:1049:651)
    at WorkspaceSvg$$module$build$src$core$workspace_svg.newBlock (webpack-internal:///./node_modules/blockly/blockly_compressed.js:1787:444)

Maybe you could use workspace.rendered check when pasting for mutators? Also maybe storing id of mutator workspace instead of entire workspace reference could be better - mutator workspace will be able to get garbage collected and we can use common.getWorkspaceById to find the mutator workspace

Storage issue persists, I can copy mutator block, reload page and paste it onto main workspace. Paste behaviour diffference remains aswell for hotkey and context menu. But I assume you haven't worked on them yet

[mjgallag]

@LappiLappland I'm going to figure 12 out on Monday so you can contribute some PRs if interested. Might do a feat branch. Don't want to deal with merging from 11.

[mjgallag]

@LappiLappland changed my mind, I want to try to keep #133 true to Update to Blockly 12 even if it means a bit more rebasing/merging from main. You'll notice I backed cd0718e out of it. Take a look at https://github.com/mjgallag/workspace-multiselect/tree/add-tests-for-keyboard-actions-in-mutators. I was wondering if you wanted to start your own branch from that and submit a PR with the fixes as I wasn't totally set on my fix anyway. I mostly just wanted to get out test helpers updated to support mutators in a relatively sane way. You can just open the PR against main, we can work against 11, makes the most sense for now. Thanks for any help you want to provide!