Open QuirkyCort opened 2 weeks ago
fwiw this occurs in both v11.0.0 and v11.1.1 so it's not related to the disappearing input blocks.
See also #7950.
Initially I knew the following (true) facts:
BlockChange
change is replayed in the secondary workspace, BlockChange.prototype.run
handles replaying mutation
-element events by calling loadExtraState
on the block.CONTROLS_IF_MUTATOR_MIXIN.loadExtraState
calls this.updateShape_()
.CONTROLS_IF_MUTATOR_MIXIN.updateShape_
does not saving and restoring connections, unlike its sibling .rebuildShape_
.It therefore seemed logical that the solution would be to have loadExtraState
instead call this.rebuildShape_()
instead.
I had actually begun making this change when I decided to do a little additional refactoring: specifically, pulling the connection-saving code out of rebuildShape_
into a separate method I had named saveChildBlocks_
(to correspond with the existing reconnectChildBlocks_
) when I noticed that there is already a saveConnections
method that does something similar, but slightly different: instead of returning the connections, it saves each on the corresponding clause blocks in the mutator bubble. It's not called from anywhere in blocks/logic.ts
, but is called from the saveConnectionsListener
in core/icons/mutator_icon.ts
, with the intention of keeping the saved connections up-to-date in the face of changes to blocks connected to the CONTROLS_IF
block on the main workspace while the mutator bubble is open.
The purpose of this (saving the connections on the clause blocks, and keeping them updated) is so that rearranging clause blocks in the mutator bubble will correspondingly rearrange the blocks connected to the CONTROLS_IF
block.
Relevant to this issue is the fact that such rearrangements cannot be conveyed only via BlockChange
events, because the mutation
element conveys only:
and no sequence of such mutations can convey information about how to reorder connected blocks, so if such changes are to be mirrored they must be mirrored by another mechanism—and in fact they are.
Playing around with the mirror demo further (after refining its logging code) demonstrated that in fact reconfiguring the CONTROLS_IF
block via its mutator normally causes the primary workspace to emit a series of BlockMove
move events that—in most circumstances—successfully disconnect and then reconnect the child blocks in the secondary workspace. The reproduction steps reported in this issue are a (fairly rare) exception, where the code in test/index.js
sees only a bare BlockChange
event emitted, without being preceded and followed by BlockMove
events as it would normally be.
In fact, after further playing around with various combinations of mutations—making a longer and longer CONTROLS_IF
block by inserting additional elseif clause blocks in the mutator—it appeared that the BlockMove
events are reliably emitted in every case except when the reshaping of the CONTROLS_IF happens to result in the one of the (formerly) attached blocks not moving.
This was very surprising, so I traced the execution of the reproduction case from the point that rebuildShape_
calls reconnectChildBlocks_
at step 5 of the reproduction, and observed Connection.prototype.connect_
create a block_move
and then .fire()
it (and then traced further to ensure that the event was actually being added to `FIRE_QUEUE
, just for good measure).
Why is this BlockMove
not being observed by the change listener set up to implement the mirroring? The most plausible hypothesis I could think of was that for some reason BlockMove
events that do not actually move the block (i.e. have zero offset) are being suppressed.
Further tracing revealed an interesting situation: at the time fireNow
is called following step 5 of the reproduction, the contents of FIRE_QUEUE
are (edited for clarity):
FIRE_QUEUE = [
/* 0: BlockMove */ {
blockId: "GHBo}8?anH~0O3/k#)9E",
newCoordinate: {x: 85.49684143066406, y: 120.89669799804688},
newInputName: undefined,
newParentId: undefined,
oldCoordinate: undefined,
oldInputName: "ELSE",
oldParentId: "R((cl0jw+oRvjZJ9JT}l",
reason: ['disconnect'],
},
/* 1: BlockMove */ {
blockId: "GHBo}8?anH~0O3/k#)9E",
newCoordinate: undefined,
newInputName: "ELSE",
newParentId: "R((cl0jw+oRvjZJ9JT}l",
oldCoordinate: {x: 85.49684143066406, y: 120.89669799804688},
oldInputName: undefined,
oldParentId: undefined,
reason: ['connect'],
},
/* 2: BlockChange */ {
blockId: "R((cl0jw+oRvjZJ9JT}l",
element: "mutation",
name: undefined
newValue: '{"elseIfCount":1,"hasElse":true}',
oldValue: '{"hasElse":true}',
}
}
Well: that's not ideal: I'd expect BlockChange
to be between the BlockMove
events, not after both. Even if the BlockMove
events hadn't disappeared, this order would still result in an incorrect replay in the secondary workspace.
So it looks like the two BlockMove
events might be being coalesced into a single event which, seeming to have no effect, is subsequently dropped for being moot. More tracing determined that the answer is yes, with a call to filter(FIRE_QUEUE, true)
returning an array containing only a single event—the BlockChange
.
So, It appears that the actual cause of the failure reported in this issue is that the BlockMove
event(s) reconnecting the blocks are being emitted before the BlockChange
event instead of after it—and curiously enough the filter
method contains the following code that appears to attempt to kludge a fix for this by reordering the the events in the queue:
// Move mutation events to the top of the queue.
// Intentionally skip first event.
for (let i = 1, event; (event = queue[i]); i++) {
// AnyDuringMigration because: Property 'element' does not exist on type
// 'Abstract'.
if (
event.type === CHANGE &&
(event as AnyDuringMigration).element === 'mutation'
) {
queue.unshift(queue.splice(i, 1)[0]);
}
}
This code first appears in 5578458, merged in PR #298. I have not so far been able to establish whether this was actually created to fix the emit-order issue or if it just happend to do so coincidentally.
(The null-event filtering was added later, in PR #1398, attempting to fix issue #1373 that had resulted from changes made in PR #1205 to address the fact that the previous filter routine had been O(n^2) due to use of Array.prototype.splice()
.)
The ultimate root cause of this issue, and the reason the BlockChange
event is being emitted after the BlockMove
events is that this is how the MutatorIcon.prototype.recomposeSourceBlock
method is implemented (simplified):
private recomposeSourceBlock() {
// ...
this.sourceBlock.compose(this.rootBlock);
// ...
eventUtils.fire(/* BlockChange */, this.sourceBlock, 'mutation', /* ... */);
// ...
}
The block's compose
method is run, modifying the shape of the block and causing all the BlockMove
events to be emitted, and then the BlockChange
event is emitted.
There are three principle categories of fixes that could be applied to this issue.
This would seem to be the most elegant and notionally correct approach. The tricky thing is how to arrange for blocks' compose
methods to emit the BlockChange
event at the correct time. There are a few possible approaches:
compose
directly create and fire a single BlockChange
event.
fire
call would be removed from recomposeSourceBlock
.BlockChange
events at all, rendering them broken.BlockChange
events, and verify these will be correctly deduped by the filter
function. ('post-dedupe')
BlockChange
-emitting fire
call would be removed from recomposeSourceBlock
.BlockChange
events (with a large number of calls to saveExtraState
.Block
-shape-changing methods set a flag or place a marker in the event queue to indicate that a BlockChange
event is pending. ('pre-dedupe')
BlockChange
-emitting fire
call would be removed from recomposeSourceBlock
.fire
function would be modified to check for such a flag/marker and create and, if present, enqueue a single BlockChange
before enqueueing the fired event.In any case it should be possible to remove the existing post-filtering reorder hack from the filter
function, though it would be wise to try to fully understand the original reason why it was created in the first place before doing so.
This would entail updating the filter
function to put the reordering hack above the main filter routine (probably breaking it out into a separate method.
BlockChange
amongst multiple BlockMove
events.Currently compose
has to do the reconnections, and only after compose
has returned can recomposeSourceBlock
emit the BlockChange
event. We could achieve correct event ordering without substantial changes to event emitting code by separating the existing connect
method into two separate well-known block methods:
compose
method would continue to do the disconnections and shape update (and, for legacy block definitions, also the reconnections).reconnectChildBlocks_
method, would be called by recomposeSourceBlock
after emitting the BlockChange
event to do the reconnections.
restoreConnections
: compare with the existing saveConnections
block method.Advantages:
compose
to this new method.Disadvantages:
MutatorIcon
and block definitions—but perhaps in a reasonably appropriate way given the existing saveConnections
/ saveConnectionsListener
mechanism.filter
function.(This option suggested by @BeksOmega.)
This would take a belt-and-braces approach by doing both of the two previous options.
BlockChange
event in second position in queue) without necessarily needing to write a fully-general implementation (one that could e.g. correctly handle overlapping mutations to multiple blocks).There are ways to resolve this issue without resolving the underlying issue in Blockly. The simplest is probably to modify the CONTROLS_IF_MUTATOR_MIXIN
's loadExtraState
method to call this.rebuildShape_
instead of this.updateShape_
.
Summary of discussion in the team meeting:
BlockChange
event has been emitted, to do the reconnections. Notes that, worst-case, the bug continues to exist for developers who don't implement this method.filter
, which we can't delete without making the bug much worse (i.e. would be a breaking change); I proposed adding reconnect block method and making a modest effort to improve the reordering code to be slightly less broken; this would probably eliminate the bug in most cases, even for developers who don't implement the reconnect method.I propose adding the block method first, since that is the less risky approach and definitely fixes the problem (for the library blocks, at least), and then looking at fixing the reordering code, and then attempting to improve the reordering code (after ensuring it has sufficient test coverage to reduce the likelihood of regression).
Check for duplicates
Description
When all mirroring events from one workspace to another, I expect both workspaces to be identical. But when an "else if" is added to an "if" block, it is possible for the two workspace to differ.
The problem can be reproduced on https://google.github.io/blockly-samples/examples/mirror-demo/, and I have also verified it on my own application where I can confirmed that all events are being mirrored.
Reproduction steps
Stack trace
No response
Screenshots
No response
Browsers
No response