Refactor comments

[apple502j]

Resolves

Resolves scratchfoundation/scratch-gui#3687
Resolves #2501

note: #1893 will no longer be resolved

Proposed Changes

First commits move comment attribute to Sprite, similar to blocks attribute. This was necessary to fix gui#3687. It also changes newBlockId so that it returns the mapping; the mapping is used inside Sprite.duplicate to fix gui#3687. RenderedTarget and Target still has comments attribute, so it should not cause problems.

Second commit is to fix #2501 - comments attached to blocks will now be deleted if the block is deleted, by deleting the comment item in deleteBlock.

Third commit is for #1893 . It has one breaking change; deserializeBlocks now return {blocks, oldToNew} instead of blocks. The mapping is used to sync Comment.blockId and block.comment later, in parseScratchObject. It also checks if the block actually exists - so any project that was saved with "invisible comment" (by #2501) will lose them.

Fourth commit is mostly tests. One new integration test is added to test #1893 and two unit tests are added to engine_blocks and sprites_rendered-targets to test #2501 and gui#3687, and they all pass for me.

Reason for Changes

• moving comments to Sprite: first, it does not make sense that a Target - which includes clones - is controlling the sprite-wide comments. This also fixes consistency between blocks and comments. The major reason is, though, we needed to handle duplication inside Sprite.duplicate, not RenderedTarget.duplicte - because we needed the mapping. I could use this.clones[0] but it will make messy code.
• newBlockIds return value - it was necessary to implement duplicating comments attached to blocks
• Deleting comments when blocks are deleted - it was visually gone, but not on VM. They still exist on the memory and in the project file, and the only way to remove it is removing the sprite (or modifying it)
• deserializeBlocks change - deserializeInputDesc generates random uid for primitives (including variables/lists), which broke comments because the id is different. It now returns oldToNew mapping which can be used to correct comment's block id.
• parseScratchObject was changed to use the returned value from deserializeBlocks to attach correct blocks to comments.

Test Coverage

Added 1 integration test (variable-comment) and 2 unit tests (see above).

Manual testing:

1. Make a variable and a list
2. Grab a variable reporter and a list reporter on a sprite
3. Add comments to those blocks
4. Add workspace comment
5. Duplicate sprite
6. The comments exist on the duplicated sprite
7. Export sprite
8. Upload again
9. All comments still exist
10. Delete blocks by dragging them to toolbox (do not touch comments)
11. Export sprite again
12. See the sprite.json of the exported sprite
13. There should be only one comment (workspace comment)

[cwillisf]

Thanks for the fixes! I made a couple of suggestions (and I think @adroitwhiz's suggestions are pretty good too!). Also I think we should reconsider the call signatures of deserializeInputDesc and deserializeBlocks to make it easier to understand what they're doing. For example, I think we can avoid a lot of the complication in comment attachment fixup for deserialized primitives by reusing the existing ID instead of mapping old ID to new ID. I want to talk to some others on the Scratch team and maybe we'll have more suggestions after that.

[cwillisf]

If you split the fix for #1893 into a separate PR then we can probably take the rest of the fixes while we're still debating the details of the deserialized primitive block IDs and such.

[apple502j]

I'm doing that @cwillisf