From 67e192e208665cda0fd7c4b48bac57cda8d347f3 Mon Sep 17 00:00:00 2001
From: "Evan W. Patton" <ewpatton@gmail.com>
Date: Fri, 27 Oct 2017 19:04:17 -0400
Subject: [PATCH] Fix logic bugs related to `e1e9427` (#1398)

This commit adds a fallback when attempting to merge events that
collide on a hash but do not actually merge. Previously, the latter
event would be dropped.

This also fixes a minor UX problem where moving the isNull check into
the merging loop resulted in extra events in the undo queue that
served no purpose and required multiple undo operations without visual
feedback.

Fixes #1373
---
 core/events.js             |  7 +++++-
 tests/jsunit/event_test.js | 48 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/core/events.js b/core/events.js
index 9edbf6e7d..72dc05fda 100644
--- a/core/events.js
+++ b/core/events.js
@@ -201,10 +201,15 @@ Blockly.Events.filter = function(queueIn, forward) {
            lastEvent.element == 'warningOpen')) {
         // Merge click events.
         lastEvent.newValue = event.newValue;
+      } else {
+        // Collision: newer events should merge into this event to maintain order
+        hash[key] = event;
+        mergedQueue.push(event);
       }
     }
   }
-  queue = mergedQueue;
+  // Filter out any events that have become null due to merging.
+  queue = mergedQueue.filter(function(e) { return !e.isNull(); });
   if (!forward) {
     // Restore undo order.
     queue.reverse();
diff --git a/tests/jsunit/event_test.js b/tests/jsunit/event_test.js
index d5583089e..3221ed927 100644
--- a/tests/jsunit/event_test.js
+++ b/tests/jsunit/event_test.js
@@ -512,6 +512,48 @@ function test_events_mergeUi() {
   eventTest_tearDownWithMockBlocks();
 }
 
+/**
+ * Tests that events that collide on a (event, block, workspace) tuple
+ * but cannot be merged do not get dropped during filtering.
+ */
+function test_events_stackclick() {
+  eventTest_setUpWithMockBlocks();
+  var block = workspace.newBlock('field_variable_test_block', '1');
+  var events = [
+    new Blockly.Events.Ui(block, 'click', undefined, undefined),
+    new Blockly.Events.Ui(block, 'stackclick', undefined, undefined)
+  ];
+  var filteredEvents = Blockly.Events.filter(events, true);
+  // click and stackclick should both exist
+  assertEquals(2, filteredEvents.length);
+  assertEquals('click', filteredEvents[0].element);
+  assertEquals('stackclick', filteredEvents[1].element);
+  eventTest_tearDownWithMockBlocks();
+}
+
+/**
+ * Mutator composition could result in move events for blocks
+ * connected to the mutated block that were null operations. This
+ * leads to events in the undo/redo queue that do nothing, requiring
+ * an extra undo/redo to proceed to the next event. This test ensures
+ * that two move events that do get merged (disconnecting and
+ * reconnecting a block in response to a mutator change) are filtered
+ * from the queue.
+ */
+function test_events_filteraftermerge() {
+  eventTest_setUpWithMockBlocks();
+  var block = workspace.newBlock('field_variable_test_block', '1');
+  block.setParent(null);
+  var events = [];
+  helper_addMoveEventParent(events, block, null);
+  helper_addMoveEventParent(events, block, null);
+  var filteredEvents = Blockly.Events.filter(events, true);
+  // The two events should be merged, but because nothing has changed
+  // they will be filtered out.
+  assertEquals(0, filteredEvents.length);
+  eventTest_tearDownWithMockBlocks();
+}
+
 /**
  * Helper function to simulate block move events.
  *
@@ -525,3 +567,9 @@ function helper_addMoveEvent(events, block, newX, newY) {
   block.xy_ = new goog.math.Coordinate(newX, newY);
   events[events.length-1].recordNew();
 }
+
+function helper_addMoveEventParent(events, block, parent) {
+  events.push(new Blockly.Events.BlockMove(block));
+  block.setParent(parent);
+  events[events.length-1].recordNew();
+}