Fix undo/redo merge bugs (#885)

* Add tests for Gitub #883.

* Add tests for #884, and fixes to test for merging.

* Add some tests for update events.

* Fix indentation

Co-authored-by: Poruri Sai Rahul <rporuri@enthought.com>

* Improve comments.

Co-authored-by: Poruri Sai Rahul <rporuri@enthought.com>

pyface/undo/command_stack.py

@@ -185,7 +185,7 @@ def push(self, command):
 
         # See if the command can be merged with the previous one.
         if len(self._macro_stack) == 0:
-            if self._index >= 0:
+            if self._index >= 0 and not self._stack[self._index].clean:
                 merged = self._stack[self._index].command.merge(command)
             else:
                 merged = False
@@ -199,8 +199,13 @@ def push(self, command):
         # Execute the command.
         result = command.do()
 
-        # Do nothing more if the command was merged.
+        # Update the stack state for a merged command.
         if merged:
+            if len(self._macro_stack) == 0:
+                # If not in macro mode, remove everything after the current
+                # command from the stack.
+                del self._stack[self._index+1:]
+            self.undo_manager.stack_updated = self
             return result
 
         # Only update the command stack if there is no current macro.

pyface/undo/tests/test_command_stack.py

@@ -12,10 +12,13 @@
 import unittest
 
 from pyface.undo.api import CommandStack, UndoManager
-from pyface.undo.tests.testing_commands import SimpleCommand, UnnamedCommand
+from pyface.undo.tests.testing_commands import (
+    MergeableCommand, SimpleCommand, UnnamedCommand,
+)
+from traits.testing.api import UnittestTools
 
 
-class TestCommandStack(unittest.TestCase):
+class TestCommandStack(UnittestTools, unittest.TestCase):
     def setUp(self):
         self.stack = CommandStack()
         undo_manager = UndoManager()
@@ -31,21 +34,92 @@ def test_empty_command_stack(self):
 
     def test_1_command_pushed(self):
         with self.assert_n_commands_pushed(self.stack, 1):
-            self.stack.push(self.command)
+            with self.assertTraitChanges(self.stack.undo_manager,
+                                         'stack_updated', count=1):
+                self.stack.push(self.command)
 
     def test_n_command_pushed(self):
         n = 4
         with self.assert_n_commands_pushed(self.stack, n):
             for i in range(n):
+                with self.assertTraitChanges(self.stack.undo_manager,
+                                            'stack_updated', count=1):
+                    self.stack.push(self.command)
+
+    def test_push_after_undo(self):
+        with self.assert_n_commands_pushed(self.stack, 1):
+            self.stack.push(self.command)
+            self.stack.undo()
+            with self.assertTraitChanges(self.stack.undo_manager,
+                                         'stack_updated', count=1):
+                self.stack.push(self.command)
+
+    def test_push_after_n_undo(self):
+        with self.assert_n_commands_pushed(self.stack, 1):
+            n = 4
+            for i in range(n):
+                self.stack.push(self.command)
+            n = 4
+            for i in range(n):
+                self.stack.undo()
+
+            with self.assertTraitChanges(self.stack.undo_manager,
+                                         'stack_updated', count=1):
                 self.stack.push(self.command)
 
+    # Command merging tests ---------------------------------------------------
+
+    def test_1_merge_command_pushed(self):
+        self.command = MergeableCommand()
+        with self.assert_n_commands_pushed(self.stack, 1):
+            with self.assertTraitChanges(self.stack.undo_manager,
+                                         'stack_updated', count=1):
+                self.stack.push(self.command)
+
+    def test_n_merge_command_pushed(self):
+        n = 4
+        with self.assert_n_commands_pushed(self.stack, 1):
+            self.command = MergeableCommand()
+            self.stack.push(self.command)
+            for i in range(n):
+                command = MergeableCommand()
+                with self.assertTraitChanges(self.stack.undo_manager,
+                                             'stack_updated', count=1):
+                    self.stack.push(command)
+        self.assertEqual(self.command.amount, n+1)
+
+    def test_merge_after_undo(self):
+        with self.assert_n_commands_pushed(self.stack, 2):
+            self.stack.push(self.command)
+            command = MergeableCommand()
+            self.stack.push(command)
+            command = SimpleCommand()
+            self.stack.push(command)
+            self.stack.undo()
+            command = MergeableCommand()
+            with self.assertTraitChanges(self.stack.undo_manager,
+                                         'stack_updated', count=1):
+                self.stack.push(command)
+
+    def test_merge_after_clean(self):
+        with self.assert_n_commands_pushed(self.stack, 2):
+            command = MergeableCommand()
+            self.stack.push(command)
+            self.stack.clean = True
+            command = MergeableCommand()
+            with self.assertTraitChanges(self.stack.undo_manager,
+                                         'stack_updated', count=1):
+                self.stack.push(command)
+
     # Undo/Redo tests ---------------------------------------------------------
 
     def test_undo_1_command(self):
         with self.assert_n_commands_pushed_and_undone(self.stack, 1):
             self.stack.push(self.command)
             self.assertEqual(self.stack.undo_name, self.command.name)
-            self.stack.undo()
+            with self.assertTraitChanges(self.stack.undo_manager,
+                                         'stack_updated', count=1):
+                self.stack.undo()
 
     def test_undo_n_command(self):
         n = 4

pyface/undo/tests/testing_commands.py

@@ -31,3 +31,26 @@ def undo(self):
 
 class UnnamedCommand(SimpleCommand):
     name = ""
+
+
+class MergeableCommand(SimpleCommand):
+
+    name = "Increment"
+
+    amount = Int(1)
+
+    def do(self):
+        self.redo()
+
+    def redo(self):
+        self.data += self.amount
+
+    def undo(self):
+        self.data -= self.amount
+
+    def merge(self, other):
+        if not isinstance(other, MergeableCommand):
+            return False
+        self.data += other.amount
+        self.amount += other.amount
+        return True