-
Notifications
You must be signed in to change notification settings - Fork 593
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
support copy/pasting between tabs and projects #10366
Conversation
Quick question, will this allow copy and pasting blocks across different targets? Like if someone tried to copy and paste blocks from micro:bit and put them in arcade, what would happen? I'm guessing that we would hit that first error case, but just wanted to ask. |
@srietkerk nope. the targets are on different domains, so they can't access each other's localstorage. if we wanted to support that, then we would have to write to the clipboard instead but that's its own can of worms. clipboard support is better today than it used to be but can still be a little flaky |
} | ||
|
||
export function getCopyPasteHandlers() { | ||
if (_handleCopy) { |
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.
Is this check needed?
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.
yes, they might be unset. the whole reason that this is in the external file is so that if our blockly code is required by a different part of the app that doesn't have the webapp code (like pxt renderer), everything will fall back to the default behavior
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.
Makes sense. Is it okay to just have the one check or should we check the other variables as well?
return !!!Blockly.clipboard.paste(copyData, copyWorkspace, centerCoords); | ||
}; | ||
|
||
if (data.version !== 1) { |
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.
I might be misunderstanding something, but how does the version given from the CopyDataEntry
also provide an indicator for the version of the editor?
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 allow pasting from other versions of the editor, i just show a warning with a "paste anyway" option. this version number is meant to indicate a change in the copy data type which would prevent us from pasting at all even if the user chose to continue past the version mismatch
Ah, that makes sense!! I see that now in the code. Thanks for the clarification! |
this adds support for copy/pasting code between tabs and projects. i borrowed the strategy of the cross tab copy paste plugin which stores copied data in localstorage instead of the clipboard (thanks for the pointer @srietkerk!). the reason we can't just use that plugin directly is because we have some extra error cases to deal with that the plugin doesn't handle:
first off, it's possible to copy code from one project that has an extension and paste a block from that extension into another project that does not have it installed. in this case, i display an error message explaining that the block can't be pasted. it might be good in the future to give information about what the exact missing extension is; right now i'm just checking to make sure all of the pasted blocks exist. we don't really have a way of associating a block with a given extension at the moment, but we could probably use the filename where the declaration is to figure it out.
secondly, you could copy code from a project in a different version of the editor (e.g. beta or maybe you just copied before the editor reload happened for a new version). 99% of the time that scenario is probably fine, but we do occasionally make changes to some of the blocks in the default extensions so i added a warning message that explains that pasting this code could harm your project. the "Paste Anyway" option on this dialog is in red to prevent people from mindlessly clicking it.
even with those warnings, there is still at least one other error case that i don't currently handle. it's possible that you could paste code from a project that has a different version of an extension where some of the blocks have changed. in this case, i think the most likely thing you'd see is some blocks that have some empty holes in them where the inputs have changed and you'd be able to just hit undo to fix it.
we could prevent that error altogether by enforcing that all installed extensions have to be the exact same version in both projects but i think that this scenario is unlikely enough that it would just end up being needlessly annoying for extensions that update frequently. maybe if we get to the point where we do actually associate the blocks with extensions then it might make sense to also add a check for this so that we can cut down on false positives