Undo/Redo cleanup

[corranwebster]

This PR does a number of things:

• adds comprehensive tests for most of the classes in traits.undo
• makes AbstractUndoItem inherit from AbstractCommand (the interfaces were almost identical)
  • updates the code so that it uses the AbstractCommand interface which permits the use of arbitrary Pyface commands in the TraitsUI history interface.
  • add backward compatibility shims so in the event that third parties are using the AbstractUndoItem API then everything should still work.
  • recommends removal of the AbstractUndoItem.merge_undo method in TraitUI 8 (in fact we can probably remove AbstractUndoItem as well.
• fixes Bug merging strings in UndoItem #1506
• fixes Adding a mergeable UndoItem to middle of UndoHistory does not fire redoable event #1509
• adds some stub documentation about how the Undo/Redo system works.

This PR should be backward compatible, so can be included in the next minor release.

This is a safe first step for #1060.

[rahulporuri]

LGTM with a few comments/questions.

[rahulporuri]

Suggested change

	#: List of accumulated undo changes. Each item is a list of
	#: AbstractUndoItems that should be done or undone as a group.
	#: This trait should be considered private.
	history = List()
	#: List of accumulated undo changes. Each item is a list of
	#: AbstractUndoItems that should be done or undone as a group.
	#: This trait should be considered private.
	history = List(List(Instance(AbstractUndoItem)))

Is ^ the right trait definition - based on the docstring you added.

[corranwebster]

It should probably be List(List(Instance(ICommand))).

[corranwebster]

But I am hesitant to over-specify it as I would like to replace this state with an UndoManager, so UndoHistory is essentially just a wrapper around UndoManager. Still investigating how much that actually breaks.

[corranwebster]

I have decided to not make ths change for now.

[rahulporuri]

i don't fully understand why this behavior exists - why merging identical values (does that mean the same id) is accepted but equal values doesn't work as expected. Is this a bug which was reported recently?

[corranwebster]

This is testing declared behaviour from this comment:

traitsui/traitsui/undo.py

Lines 242 to 244 in 79b66f5

	# Discard undo items that are identical to us. This is to eliminate
	# the same undo item being created by multiple listeners monitoring the
	# same list for changes:

Since it is declared it should be tested that the code does what it says; if we feel that relaxing to equality is desirable, we should modify the code and the comment, but I don't have enough insight into whether that would be a problem.

[corranwebster]

Ah - just realized there is a missing piece in this. If we accept arbitrary ICommand instances in the history, then we need to call do() somewhere, and we should add a test for this.

[rahulporuri]

Ah - just realized there is a missing piece in this. If we accept arbitrary ICommand instances in the history, then we need to call do() somewhere, and we should add a test for this.

Just so i'm clear, this PR is on hold until ^ changes are made?

[corranwebster]

That plus the suggested doc fixes - this one is a spare-time effort.

[corranwebster]

@rahulporuri I think this is good to go.

This now:

• fixes the issue I mentioned with the do method
• adds a deprecation warning for the old API
• adds tests for these
• adds hyperlinks and a but of clean-up to the new documentation.

[rahulporuri]

Still LGTM

[rahulporuri]

It'll be useful to track this in an issue.

[corranwebster]

Done!