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

@@ -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();

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();
+}