Comments not recorded in delete event

[NeilFraser]

In the playground, turn on event logging and open the console.
Create any block (I'll use 'not' in this example) and place it on the workspace.
Add a 'hello world' comment to this block. Note that a Change event is correctly created.
Delete the block. Note that the Delete event's oldXML.outerHTML lacks the comment:

<block type="logic_negate" id="TDR|)+6F]?Id!;Pl9GcJ" x="137" y="212"></block>

Then if undo is executed, the block reappears without the comment.

This is not a regression.

[rachel-fenichel]

@kchadha FYI

[RoboErikG]

Investigated some. The problem is that BlockSVG.dispose disposes of the icons before super.dispose which is where the delete event gets recorded. Since the icon owns the comment the comment gets destroyed before we can save it.

Short term I'll investigate if we can just destroy the icons after calling super, but long term the icon probably shouldn't own the comment like that.

[RoboErikG]

The fix was rolled back as it broke other things.

[BeksOmega]

I ran into this when I was testing the trashcan contents, and I was wondering, is there any reason why we couldn't just dispose of the block before disposing of the icons? i.e.:

  Blockly.BlockSvg.superClass_.dispose.call(this, healStack);
  Blockly.Events.disable();
  try {
    var icons = this.getIcons();
    for (var i = 0; i < icons.length; i++) {
      icons[i].dispose();
    }
  } finally {
    Blockly.Events.enable();
  }

instead of:

Blockly.Events.disable();
try {
  var icons = this.getIcons();
  for (var i = 0; i < icons.length; i++) {
    icons[i].dispose();
  }
} finally {
  Blockly.Events.enable();
}
Blockly.BlockSvg.superClass_.dispose.call(this, healStack);

I did a quick test and I think it would work. I couldn't replicate the error found in #1970, and I don't believe any of the functions following the superClass_.dispose() call would access the block, but it does seem like it would be a bit of a silly thing to do.

Here's the link to the relevant .dispose() function.

Edit: I went back and looked at #1970 & #1972 and I believe the only problem is that it needs to call this.unplug(healStack) before creating the delete event. I did a quick check and there were no more errors.

[rachel-fenichel]

@RoboErikG you were looking at this earlier. Does this fix make sense to you?

[RoboErikG]

As far as I can tell it would work, but I'm really hesitant to call the super's dispose before the specific dispose. It's typically an implied part of the API that more general cleanup code gets called after the specific cleanup code.

I'd be okay with making this as a temporary fix with a TODO to move the super's dispose down again once we separate out the comment's model and UI.

[BeksOmega]

We could also use the old fix, and just add this.unplug(healStack) before creating the delete event. If we were to add that, the blocks wouldn't be connected anymore so we wouldn't get the error from #1972. (Tested and confirmed) You can see how block.js does this inside it's dispose function.

If we were to do that it would preserve the cleanup order, as well as fix the issue.

[RoboErikG]

I like that solution more, and there shouldn't be any issues with unplug being called twice. =)