fix(events): Simplify filter function, add new enqueueEvent function

core/events/utils.ts

@@ -119,7 +119,7 @@ function fireInternal(event: Abstract) {
       setTimeout(fireNow, 0);
     }
   }
-  FIRE_QUEUE.push(event);
+  enqueueEvent(event);
 }
 
 /** Dispatch all queued events. */
@@ -137,6 +137,46 @@ function fireNow() {
   }
 }
 
+/**
+ * Enqueue an event on FIRE_QUEUE.
+ *
+ * Normally this is equivalent to FIRE_QUEUE.push(event), but if the
+ * enqueued event is a BlockChange event and the most recent event(s)
+ * on the queue are BlockMove events that (re)connect other blocks to
+ * the changed block (and belong to the same event group) then the
+ * enqueued event will be enqueued before those events rather than
+ * after.
+ *
+ * This is a workaround for a problem caused by the fact that
+ * MutatorIcon.prototype.recomposeSourceBlock can only fire a
+ * BlockChange event after the mutating block's compose method
+ * returns, meaning that if the compose method reconnects child blocks
+ * the corresponding BlockMove events are emitted _before_ the
+ * BlockChange event, causing issues with undo, mirroring, etc.; see
+ * https://github.com/google/blockly/issues/8225#issuecomment-2195751783
+ * (and following) for details.
+ */
+function enqueueEvent(event: Abstract) {
+  if (isBlockChange(event) && event.element === 'mutation') {
+    let i;
+    for (i = FIRE_QUEUE.length; i > 0; i--) {
+      const otherEvent = FIRE_QUEUE[i - 1];
+      if (
+        otherEvent.group !== event.group ||
+        otherEvent.workspaceId !== event.workspaceId ||
+        !isBlockMove(otherEvent) ||
+        otherEvent.newParentId !== event.blockId
+      ) {
+        break;
+      }
+    }
+    FIRE_QUEUE.splice(i, 0, event);
+    return;
+  }
+
+  FIRE_QUEUE.push(event);
+}
+
 /**
  * Filter the queued events by merging duplicates, removing null
  * events and reording BlockChange events.
@@ -178,6 +218,12 @@ function fireNow() {
  * to reason about how events are processed for undo/redo, so both the
  * call from undo and the post-processing code was removed.
  *
+ * At the same time, the buggy code to reorder BlockChange events was
+ * replaced by a less-buggy version of the same functionality in a new
+ * function, enqueueEvent, called from fireInternal, thus assuring
+ * that events will be in the correct order at the time filter is
+ * called.
+ *
  * @param queueIn Array of events.
  * @param forward True if forward (redo), false if backward (undo).
  * @returns Array of filtered events.
@@ -254,18 +300,6 @@ export function filter(queueIn: Abstract[], forward: boolean): Abstract[] {
     // Restore undo order.
     queue.reverse();
   }
-  // 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 === EventType.BLOCK_CHANGE &&
-      (event as AnyDuringMigration).element === 'mutation'
-    ) {
-      queue.unshift(queue.splice(i, 1)[0]);
-    }
-  }
   return queue;
 }
 
@@ -434,6 +468,7 @@ export function disableOrphans(event: Abstract) {
 
 export const TEST_ONLY = {
   FIRE_QUEUE,
+  enqueueEvent,
   fireNow,
   fireInternal,
   setGroupInternal,

tests/mocha/event_test.js

@@ -42,6 +42,26 @@ suite('Events', function () {
         'type': 'simple_test_block',
         'message0': 'simple test block',
       },
+      {
+        'type': 'inputs_test_block',
+        'message0': 'first %1 second %2',
+        'args0': [
+          {
+            'type': 'input_statement',
+            'name': 'STATEMENT1',
+          },
+          {
+            'type': 'input_statement',
+            'name': 'STATEMENT2',
+          },
+        ],
+      },
+      {
+        'type': 'statement_test_block',
+        'message0': '',
+        'previousStatement': null,
+        'nextStatement': null,
+      },
     ]);
   });
 
@@ -1102,6 +1122,165 @@ suite('Events', function () {
     });
   });
 
+  suite('enqueueEvent', function () {
+    const {FIRE_QUEUE, enqueueEvent} = eventUtils.TEST_ONLY;
+
+    function newDisconnectEvent(parent, child, inputName, workspaceId) {
+      const event = new Blockly.Events.BlockMove(child);
+      event.workspaceId = workspaceId;
+      event.oldParentId = parent.id;
+      event.oldInputName = inputName;
+      event.oldCoordinate = undefined;
+      event.newParentId = undefined;
+      event.newInputName = undefined;
+      event.newCoordinate = new Blockly.utils.Coordinate(0, 0);
+      return event;
+    }
+
+    function newConnectEvent(parent, child, inputName, workspaceId) {
+      const event = new Blockly.Events.BlockMove(child);
+      event.workspaceId = workspaceId;
+      event.oldParentId = undefined;
+      event.oldInputName = undefined;
+      event.oleCoordinate = new Blockly.utils.Coordinate(0, 0);
+      event.newParentId = parent.id;
+      event.newInputName = inputName;
+      event.newCoordinate = undefined;
+      return event;
+    }
+
+    function newMutationEvent(block, workspaceId) {
+      const event = new Blockly.Events.BlockChange(block);
+      event.workspaceId = workspaceId;
+      event.element = 'mutation';
+      return event;
+    }
+
+    test('Events are enqueued', function () {
+      // Disable events during block creation to avoid firing BlockCreate
+      // events.
+      eventUtils.disable();
+      const block = this.workspace.newBlock('simple_test_block', '1');
+      eventUtils.enable();
+
+      try {
+        assert.equal(FIRE_QUEUE.length, 0);
+        const events = [
+          new Blockly.Events.BlockCreate(block),
+          new Blockly.Events.BlockMove(block),
+          new Blockly.Events.Click(block),
+        ];
+        events.map((e) => enqueueEvent(e));
+        assert.equal(FIRE_QUEUE.length, events.length, 'FIRE_QUEUE.length');
+        for (let i = 0; i < events.length; i++) {
+          assert.equal(FIRE_QUEUE[i], events[i], `FIRE_QUEUE[${i}]`);
+        }
+      } finally {
+        FIRE_QUEUE.length = 0;
+      }
+    });
+
+    test('BlockChange event reordered', function () {
+      eventUtils.disable();
+      const parent = this.workspace.newBlock('inputs_test_block', 'parent');
+      const child1 = this.workspace.newBlock('statement_test_block', 'child1');
+      const child2 = this.workspace.newBlock('statement_test_block', 'child2');
+      eventUtils.enable();
+
+      try {
+        assert.equal(FIRE_QUEUE.length, 0);
+        const events = [
+          newDisconnectEvent(parent, child1, 'STATEMENT1'),
+          newDisconnectEvent(parent, child2, 'STATEMENT2'),
+          newConnectEvent(parent, child1, 'STATEMENT1'),
+          newConnectEvent(parent, child2, 'STATEMENT2'),
+          newMutationEvent(parent),
+        ];
+        events.map((e) => enqueueEvent(e));
+        assert.equal(FIRE_QUEUE.length, events.length, 'FIRE_QUEUE.length');
+        assert.equal(FIRE_QUEUE[0], events[0], 'FIRE_QUEUE[0]');
+        assert.equal(FIRE_QUEUE[1], events[1], 'FIRE_QUEUE[1]');
+        assert.equal(FIRE_QUEUE[2], events[4], 'FIRE_QUEUE[2]');
+        assert.equal(FIRE_QUEUE[3], events[2], 'FIRE_QUEUE[3]');
+        assert.equal(FIRE_QUEUE[4], events[3], 'FIRE_QUEUE[4]');
+      } finally {
+        FIRE_QUEUE.length = 0;
+      }
+    });
+
+    test('BlockChange event for other workspace not reordered', function () {
+      eventUtils.disable();
+      const parent = this.workspace.newBlock('inputs_test_block', 'parent');
+      const child = this.workspace.newBlock('statement_test_block', 'child');
+      eventUtils.enable();
+
+      try {
+        assert.equal(FIRE_QUEUE.length, 0);
+        const events = [
+          newDisconnectEvent(parent, child, 'STATEMENT1', 'ws1'),
+          newConnectEvent(parent, child, 'STATEMENT1', 'ws1'),
+          newMutationEvent(parent, 'ws2'),
+        ];
+        events.map((e) => enqueueEvent(e));
+        assert.equal(FIRE_QUEUE.length, events.length, 'FIRE_QUEUE.length');
+        for (let i = 0; i < events.length; i++) {
+          assert.equal(FIRE_QUEUE[i], events[i], `FIRE_QUEUE[${i}]`);
+        }
+      } finally {
+        FIRE_QUEUE.length = 0;
+      }
+    });
+
+    test('BlockChange event for other group not reordered', function () {
+      eventUtils.disable();
+      const parent = this.workspace.newBlock('inputs_test_block', 'parent');
+      const child = this.workspace.newBlock('statement_test_block', 'child');
+      eventUtils.enable();
+
+      try {
+        assert.equal(FIRE_QUEUE.length, 0);
+        const events = [];
+        eventUtils.setGroup('group1');
+        events.push(newDisconnectEvent(parent, child, 'STATEMENT1'));
+        events.push(newConnectEvent(parent, child, 'STATEMENT1'));
+        eventUtils.setGroup('group2');
+        events.push(newMutationEvent(parent, 'ws2'));
+        events.map((e) => enqueueEvent(e));
+        assert.equal(FIRE_QUEUE.length, events.length, 'FIRE_QUEUE.length');
+        for (let i = 0; i < events.length; i++) {
+          assert.equal(FIRE_QUEUE[i], events[i], `FIRE_QUEUE[${i}]`);
+        }
+      } finally {
+        FIRE_QUEUE.length = 0;
+        eventUtils.setGroup(false);
+      }
+    });
+
+    test('BlockChange event for other parent not reordered', function () {
+      eventUtils.disable();
+      const parent1 = this.workspace.newBlock('inputs_test_block', 'parent1');
+      const parent2 = this.workspace.newBlock('inputs_test_block', 'parent2');
+      const child = this.workspace.newBlock('statement_test_block', 'child');
+      eventUtils.enable();
+
+      try {
+        assert.equal(FIRE_QUEUE.length, 0);
+        const events = [
+          newDisconnectEvent(parent1, child, 'STATEMENT1', 'ws1'),
+          newConnectEvent(parent1, child, 'STATEMENT1', 'ws1'),
+          newMutationEvent(parent2, 'ws2'),
+        ];
+        events.map((e) => enqueueEvent(e));
+        assert.equal(FIRE_QUEUE.length, events.length, 'FIRE_QUEUE.length');
+        for (let i = 0; i < events.length; i++) {
+          assert.equal(FIRE_QUEUE[i], events[i], `FIRE_QUEUE[${i}]`);
+        }
+      } finally {
+        FIRE_QUEUE.length = 0;
+      }
+    });
+  });
+
   suite('Filters', function () {
     function addMoveEvent(events, block, newX, newY) {
       events.push(new Blockly.Events.BlockMove(block));