Duplicating blocks with shadows keep the same shadow id

[paulkaplan]

When you create two of the same block by dragging them out of the toolbox twice, you get different IDs for all the blocks (including the shadow)

However, when you right-click and duplicate, you get a block with a different ID but the shadow has the same ID.

This is causing a bug in scratch-vm scratchfoundation/scratch-vm#589 because the vm-side block cache can be fooled by blocks with the same IDs (specifically, if you delete one of them and then try to read out the block XML, you get an error).

@rachel-fenichel is this a known / desired behavior?

[RoboErikG]

That only happens when you duplicate a block where the shadow is hidden by another block. This is because the shadow is serialized to xml and not present in the model when it's replaced by another block, and duplicating it doesn't touch the serialized shadow xml.

When you remove the x position blocks the shadows should be loaded from xml back into the model and the second one will get a new id.

[paulkaplan]

Ok interesting. I can't seem to find where in the code that is happening. What would be the easiest way to fix this problem?

For more context, you can see scratchfoundation/scratch-vm#589, where the TL;DR is "it isn't safe to delete blocks if there are duplicate IDs."

[paulkaplan]

It looks like one fix would be to use the opt_noId option in blockToDom while copying the XML.

[RoboErikG]

I think your VM is missing a step when a shadow block is replaced by a normal block. In the connection code the shadow gets serialized and then the model object is disposed. I'm guessing your VM doesn't dispose of the shadow object and this leads to having two copies of it when you try to delete.

The options are to add a check in the VM to remove the shadow block when another block is attached or we can start sending events for the shadow block being deleted/created. We hadn't added events for it because we originally thought only other Blockly instances would listen to events and would always perform the delete/create of the shadow block in response to the move.

[paulkaplan]

My knowledge of this area of the VM is still very limited, so /cc @thisandagain and @rschamp who might have more context?

I believe if we remove the shadow block when another block is attached then we would lose a complete picture of the workspace (i.e. we would not be able to serialize/deserialize the workspace properly). We would have to maintain a "present but not really present" list of obscured shadows in order to support the duplicate IDs.

It might be easier to remove the IDs when copy/pasting? (or is this indicative of a bigger problem that will keep coming up in other ways?)

[rachel-fenichel]

As discussed offline:

• scratch-blocks is emitting the correct events (plus a few extra right now, which makes debugging annoying: Create and delete events are being fired for insertion markers #935)
• scratch-vm is explicitly ignoring events related to shadow blocks
• you can debug events by turning on "log events" in the vertical playground, or in the blockly playground
• scratch-vm needs to add the concept of an occluded shadow block that it manages

[tmickel]

scratch-vm definitely has the concept of occluded shadow blocks... it might be broken, though :)