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

* docs(events): JSDoc improvments

* fix(filter): Introduce enqueueEvent; don't reorder in filter

  Remove the broken BlockChange event reordering code from filter.
  Instead, do necessary event reordering (correctly, this time)
  in a new enqueueEvent function used by fireInternal.

* chore(events): Deprecate forward parameter of filter

  Since we don't filter in undo any more, and filtering in reverse
  order causes problems, prepare to remove this opportunity for
  error.

* fix(events): Only merge adjacent events

  Simplify filter by having it consider only adjacent events in the
  queue for merging.

  Previously any events in the queue could potentially be merged if
  they were of suitable (typically identical) .type, even if other
  events had occurred between them (with the sole exception of
  BlockMove events, which would only be merged with adjacent
  events).  This could potentially result in replay failures during
  undo/redo/mirroring, though it is unknown whether any such
  problems occurred in practice.

* refactor(events): Tweaks

  - Use for…of loop where appropriate.
  - Make filter reason-merging code more concise.
  - Use arrow functions when calling Array.prototype.filter.

* chore(events): Fix typos for PR #8539.

core/events/utils.ts

@@ -9,6 +9,7 @@
 import type {Block} from '../block.js';
 import * as common from '../common.js';
 import * as registry from '../registry.js';
+import * as deprecation from '../utils/deprecation.js';
 import * as idGenerator from '../utils/idgenerator.js';
 import type {Workspace} from '../workspace.js';
 import type {WorkspaceSvg} from '../workspace_svg.js';
@@ -26,7 +27,6 @@ import {
   isClick,
   isViewportChange,
 } from './predicates.js';
-import {EventType} from './type.js';
 
 /** Group ID for new events.  Grouped events are indivisible. */
 let group = '';
@@ -79,9 +79,20 @@ export type BumpEvent =
 const FIRE_QUEUE: Abstract[] = [];
 
 /**
- * Create a custom event and fire it.
+ * Enqueue an event to be dispatched to change listeners.
  *
- * @param event Custom data for event.
+ * Notes:
+ *
+ * - Events are enqueued until a timeout, generally after rendering is
+ *   complete or at the end of the current microtask, if not running
+ *   in a browser.
+ * - Queued events are subject to destructive modification by being
+ *   combined with later-enqueued events, but only until they are
+ *   fired.
+ * - Events are dispatched via the fireChangeListener method on the
+ *   affected workspace.
+ *
+ * @param event Any Blockly event.
  */
 export function fire(event: Abstract) {
   TEST_ONLY.fireInternal(event);
@@ -108,14 +119,14 @@ function fireInternal(event: Abstract) {
       setTimeout(fireNow, 0);
     }
   }
-  FIRE_QUEUE.push(event);
+  enqueueEvent(event);
 }
 
 /** Dispatch all queued events. */
 function fireNow() {
   const queue = filter(FIRE_QUEUE, true);
   FIRE_QUEUE.length = 0;
-  for (let i = 0, event; (event = queue[i]); i++) {
+  for (const event of queue) {
     if (!event.workspaceId) {
       continue;
     }
@@ -126,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.
@@ -151,107 +202,103 @@ function fireNow() {
  * https://github.com/google/blockly/issues/2037#issuecomment-2209696351
  *
  * Later, in PR #1205 the original O(n^2) implementation was replaced
- * by a linear-time implementation, though addiitonal fixes were made
+ * by a linear-time implementation, though additonal fixes were made
  * subsequently.
  *
- * This function was previously called from Workspace.prototype.undo,
- * but this was the cause of issue #7026, the originally-chosen fix
- * for which was the addition (in PR #7069) of code to fireNow to
- * post-filter the .undoStack_ and .redoStack_ of any workspace that
- * had just been involved in dispatching events.  This apparently
- * resolved the issue but added considerable additional complexity and
- * made it difficlut to reason about how events are processed for
- * undo/redo, so both the call from undo and the post-processing code
- * was later removed.
+ * In August 2024 a number of significant simplifications were made:
  *
- * @param queueIn Array of events.
+ * This function was previously called from Workspace.prototype.undo,
+ * but the mutation of events by this function was the cause of issue
+ * #7026 (note that events would combine differently in reverse order
+ * vs. forward order).  The originally-chosen fix for this was the
+ * addition (in PR #7069) of code to fireNow to post-filter the
+ * .undoStack_ and .redoStack_ of any workspace that had just been
+ * involved in dispatching events; this apparently resolved the issue
+ * but added considerable additional complexity and made it difficult
+ * to reason about how events are processed for undo/redo, so both the
+ * call from undo and the post-processing code was removed, and
+ * forward=true was made the default while calling the function with
+ * forward=false was deprecated.
+ *
+ * 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.
+ *
+ * Additionally, the event merging code was modified so that only
+ * immediately adjacent events would be merged.  This simplified the
+ * implementation while ensuring that the merging of events cannot
+ * cause them to be reordered.
+ *
+ * @param queue Array of events.
  * @param forward True if forward (redo), false if backward (undo).
+ *     This parameter is deprecated: true is now the default and
+ *     calling filter with it set to false will in future not be
+ *     supported.
  * @returns Array of filtered events.
  */
-export function filter(queueIn: Abstract[], forward: boolean): Abstract[] {
-  let queue = queueIn.slice();
-  // Shallow copy of queue.
+export function filter(queue: Abstract[], forward = true): Abstract[] {
   if (!forward) {
-    // Undo is merged in reverse order.
-    queue.reverse();
+    deprecation.warn('filter(queue, /*forward=*/false)', 'v11.2', 'v12');
+    // Undo was merged in reverse order.
+    queue = queue.slice().reverse(); // Copy before reversing in place.
   }
-  const mergedQueue = [];
-  const hash = Object.create(null);
+  const mergedQueue: Abstract[] = [];
   // Merge duplicates.
-  for (let i = 0, event; (event = queue[i]); i++) {
-    if (!event.isNull()) {
-      // Treat all UI events as the same type in hash table.
-      const eventType = event.isUiEvent ? EventType.UI : event.type;
-      // TODO(#5927): Check whether `blockId` exists before accessing it.
-      const blockId = (event as AnyDuringMigration).blockId;
-      const key = [eventType, blockId, event.workspaceId].join(' ');
-
-      const lastEntry = hash[key];
-      const lastEvent = lastEntry ? lastEntry.event : null;
-      if (!lastEntry) {
-        // Each item in the hash table has the event and the index of that event
-        // in the input array.  This lets us make sure we only merge adjacent
-        // move events.
-        hash[key] = {event, index: i};
-        mergedQueue.push(event);
-      } else if (isBlockMove(event) && lastEntry.index === i - 1) {
-        // Merge move events.
-        lastEvent.newParentId = event.newParentId;
-        lastEvent.newInputName = event.newInputName;
-        lastEvent.newCoordinate = event.newCoordinate;
-        if (event.reason) {
-          if (lastEvent.reason) {
-            // Concatenate reasons without duplicates.
-            const reasonSet = new Set(event.reason.concat(lastEvent.reason));
-            lastEvent.reason = Array.from(reasonSet);
-          } else {
-            lastEvent.reason = event.reason;
-          }
-        }
-        lastEntry.index = i;
-      } else if (
-        isBlockChange(event) &&
-        event.element === lastEvent.element &&
-        event.name === lastEvent.name
-      ) {
-        // Merge change events.
-        lastEvent.newValue = event.newValue;
-      } else if (isViewportChange(event)) {
-        // Merge viewport change events.
-        lastEvent.viewTop = event.viewTop;
-        lastEvent.viewLeft = event.viewLeft;
-        lastEvent.scale = event.scale;
-        lastEvent.oldScale = event.oldScale;
-      } else if (isClick(event) && isBubbleOpen(lastEvent)) {
-        // Drop click events caused by opening/closing bubbles.
-      } else {
-        // Collision: newer events should merge into this event to maintain
-        // order.
-        hash[key] = {event, index: i};
-        mergedQueue.push(event);
+  for (const event of queue) {
+    const lastEvent = mergedQueue[mergedQueue.length - 1];
+    if (event.isNull()) continue;
+    if (
+      !lastEvent ||
+      lastEvent.workspaceId !== event.workspaceId ||
+      lastEvent.group !== event.group
+    ) {
+      mergedQueue.push(event);
+      continue;
+    }
+    if (
+      isBlockMove(event) &&
+      isBlockMove(lastEvent) &&
+      event.blockId === lastEvent.blockId
+    ) {
+      // Merge move events.
+      lastEvent.newParentId = event.newParentId;
+      lastEvent.newInputName = event.newInputName;
+      lastEvent.newCoordinate = event.newCoordinate;
+      // Concatenate reasons without duplicates.
+      if (lastEvent.reason || event.reason) {
+        lastEvent.reason = Array.from(
+          new Set((lastEvent.reason ?? []).concat(event.reason ?? [])),
+        );
       }
+    } else if (
+      isBlockChange(event) &&
+      isBlockChange(lastEvent) &&
+      event.blockId === lastEvent.blockId &&
+      event.element === lastEvent.element &&
+      event.name === lastEvent.name
+    ) {
+      // Merge change events.
+      lastEvent.newValue = event.newValue;
+    } else if (isViewportChange(event) && isViewportChange(lastEvent)) {
+      // Merge viewport change events.
+      lastEvent.viewTop = event.viewTop;
+      lastEvent.viewLeft = event.viewLeft;
+      lastEvent.scale = event.scale;
+      lastEvent.oldScale = event.oldScale;
+    } else if (isClick(event) && isBubbleOpen(lastEvent)) {
+      // Drop click events caused by opening/closing bubbles.
+    } else {
+      mergedQueue.push(event);
     }
   }
   // Filter out any events that have become null due to merging.
-  queue = mergedQueue.filter(function (e) {
-    return !e.isNull();
-  });
+  queue = mergedQueue.filter((e) => !e.isNull());
   if (!forward) {
     // 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;
 }
 
@@ -420,6 +467,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.oldCoordinate = 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));