BlockChange mutation events not fired in the correct order and/or at all

[AnmAtAnm]

[Edit by @cpcallen, 2024-08-30:]

When a block is mutated, BlockChange events are either not emitted, or emitted out-of-order with respect to BlockMove events that record child blocks being disconnected and reconnected. This has been the root cause of multiple other issues. See comment below for full write-up of situation as it currently stands.

[Original description:]

Mutation events are pulled ahead of other events in a group. It is not clear why this is necessary, but it leads to a situation, where other events can reference inputs of the mutated block that no longer exist (assuming sequential processing). [Edit: this specific problem was fixed with a better band-aid in #8539.]

For cross-platform simplicity, events should be emitted in an order that can be processed sequentially. In this case, the move event that disconnects the attached block should occur before the mutation event that deletes the input.

Before:

After:

Playground Events, with mutation change before move and oldInputName:

Similar construction in the mirror demo does not reference the missing input (no oldInputName field).

[rachel-fenichel]

@NeilFraser

[maribethb]

@NeilFraser is this the bug/situation you are currently looking at with mutator events?

[cpcallen]

This bug reported the underlying root cause of at least two later bugs (#2037—which has been more than once— and #8225) as well as a contributing cause to at least one other (#7950).

BlockChange events not fired when blocks are mutated

The fundamental problem is that Blockly does not contain any code to fire BlockChange (.element === 'mutation') events in the general case of block mutation (i.e., changes to the shape of a block), only in certain specific situations.

Exception: MutatorIcon

Where a block is being mutated via a MutatorIcon, the MutatorIcon.prototype.recomposeSourceBlock method will fire a BlockChange event. Unfortunately, it does so only after the associated BlockMove events disconnecting and reconnecting child events have been fired.

• It appears that commit 5578458 attempted to patch this issue by adding code to reorder BlockChange events to the filter function called from fireNow, but this was buggy and has caused several other issues (including this one) to be filed.
• A better fix was made in fix(events): Simplify filter function, add new enqueueEvent function #8539, but this is still just a band-aid to reorder the events after the fact.
• Various alternative fixes have been proposed for this specific cause but they mostly do no address the wider issue.

Exception: mutateCallers

When a procedure definition block is mutated (by its own domToMutation, loadExtraState or compose methods, as well as in certain other situations) it will call mutateCallers (in core/procedures.ts), which will mutate the corresponding procedure call blocks and fire BlockChange events for them.

• Fortuitously the BlockChange event is fired after the BlockMove events disconnecting child blocks.
• Unfortunately all child blocks will be disconnected from all callers, even for unaffected inputs, and none will be reconnected, even if the removed parameter is immediately replaced in a different position in the procedure definition's mutator.

Quasi-exception: BlockChange .element === 'field' events

There are a number of blocks that mutate themselves via a FieldInput validator (e.g. lists_getIndex).

• These blocks generally do not depend on a BlockChange .element === 'mutation' event being fired in order to correctly mutate themselves in undo/redo/mirroring/etc.
  • They also do not generally implement .saveExtraState and .loadExtraState, so it may not be possible to fire mutation events for them.
• There is code in core/field_input.ts that will fire a 'field' event when the field is modified
  • This can happen only after validation, so the BlockChange event will be fired after any BlockMove events disconnecting blocks from deleted inputs, as it should be.
  • Fortuitously it appears that there may be no block that attempts to reconnect a child block during mutation, so we may not need to worry about event ordering for reconnection events.

Observations

Trying to find a fully-general solution is complicated by a number of factors:

• None of the library blocks explicitly fires a BlockChange event. We could potentially move responsibility for firing such events from Blockly to the individual block definitions, but this would be a substantial, breaking change.
• Creating BlockChange mutation events in the general case is complicated by the fact that these events currently represent the shape of a block not directly (i.e., with a record of its fields and inputs) but by the serialised value from .saveAdditionalState(), but:
  • That method does not necessarily exist—e.g. it is missing from many blocks that mutate via a field validator.
  • The returned value does not necessarily change, and if it does change, it does not change at the same time as the block's shape, but might update before, between, or after a series of field or input additions/deletions.
  • Representing the shape of a block in some other way would be a breaking change.
    • It is not clear that the shape of a block could be fully represented in a serialisable way (e.g.: field validators).
• There is no single-point-of-entry for modifications to the shape of a block. Inputs and fields can be added, deleted and reordered at any time via a number of different methods both on the block (for inputs) and inputs (for fields).
  • One of the proposed fixes for #8225 envisioned hooking such methods to capture the start of a mutation, but there is no general way to know when a mutation is finished.
  • We could fire BlockChange event capturing the actual shape of the block for every single input/field addition/deletion/reordering, and depend on the filter function to merge these.
  • We could record the additional state of the block at the time the first modification is observed and then fire a BlockChange event with the "final" additional state as soon as any other event is fired. To work properly, this would require block mutators to update whatever internal properties before and after each individual modification, rather than only once before or after the complete mutation.

See also

• Block.removeInput Doesn't Fire Change Event #1717 discusses specifically the case firing BlockChange events when inputs are deleted.