From 090ecd0fbc7a95e9bb8b4642cd23d8fe8d8a4e45 Mon Sep 17 00:00:00 2001 From: Nathan Glenn Date: Tue, 13 Aug 2024 17:33:20 -0500 Subject: [PATCH] fix issue with sequential atomic edits merging This was a lot easier to figure out with a test suite! Add a regressin test that checks that adjacent atomic edits remain separate. Fix it by increasing the priority of `inAtomicEdit` within `shouldAddToExistingEdit`. Break apart the large boolean equation into individual statements for greater clarity. --- .../ruleeditor/CompoundUndoManager.java | 38 ++++-- .../ruleeditor/CompoundUndoManagerTest.java | 113 ++++++++++++------ 2 files changed, 103 insertions(+), 48 deletions(-) diff --git a/src/main/java/edu/umich/soar/visualsoar/ruleeditor/CompoundUndoManager.java b/src/main/java/edu/umich/soar/visualsoar/ruleeditor/CompoundUndoManager.java index a92ce0a..33060ec 100644 --- a/src/main/java/edu/umich/soar/visualsoar/ruleeditor/CompoundUndoManager.java +++ b/src/main/java/edu/umich/soar/visualsoar/ruleeditor/CompoundUndoManager.java @@ -72,7 +72,6 @@ public class AtomicModeManager implements AutoCloseable { private AtomicModeManager() { endCompoundEdit(); inAtomicEdit = true; - startCompoundEdit(null); } @Override @@ -150,7 +149,7 @@ public void undoableEditHappened(UndoableEditEvent e) { return; } - // Not incremental edit, end previous edit and start a new one + // Not incremental edit; end previous edit and start a new one if (!inAtomicEdit) { endCompoundEdit(); @@ -159,16 +158,26 @@ public void undoableEditHappened(UndoableEditEvent e) { } private boolean shouldAddToExistingEdit(UndoableEdit ue) { - return - // we have an existing edit to add to - compoundEdit != null - && - // start new edit after document save - !lastActionWasSave.get() - && - // start new edit if user switches between insertion/deletion - !editingSwitchedBetweenInsertAndDelete() - && (isStyleChange(ue) || inAtomicEdit || isIncrementalEditOrBackspace()); + if (compoundEdit == null) { + // impossible to add to one because none exists + return false; + } + // honor atomic guarantee if requested by client + if (inAtomicEdit) { + return true; + } + + // after save, we start a new edit + if (lastActionWasSave.get()) { + return false; + } + + // start new edit if user switches between insertion/deletion + if (editingSwitchedBetweenInsertAndDelete()) { + return false; + } + + return (isStyleChange(ue) || isIncrementalEditOrBackspace()); } private void updatePreviousEditInfo() { @@ -314,6 +323,11 @@ public void removeUpdate(DocumentEvent e) { @Override public void changedUpdate(DocumentEvent e) {} + @Override + public String toString() { + return "Compound" + super.toString(); + } + class MyCompoundEdit extends CompoundEdit { public boolean isInProgress() { // in order for the canUndo() and canRedo() methods to work diff --git a/src/test/java/edu/umich/soar/visualsoar/ruleeditor/CompoundUndoManagerTest.java b/src/test/java/edu/umich/soar/visualsoar/ruleeditor/CompoundUndoManagerTest.java index e0fe1f4..8f7f2a8 100644 --- a/src/test/java/edu/umich/soar/visualsoar/ruleeditor/CompoundUndoManagerTest.java +++ b/src/test/java/edu/umich/soar/visualsoar/ruleeditor/CompoundUndoManagerTest.java @@ -54,6 +54,46 @@ public void sequentialDeletionsAreJoined() throws BadLocationException { assertEquals("", editorPane.getText(), "Redo should delete entire input string again"); } + @Test + public void adjacentAtomicEditsAreNotJoinedInAtomicModeAreJoined() throws Exception { + JEditorPane editorPane = new JEditorPane(); + Document doc = editorPane.getDocument(); + CompoundUndoManager undoManager = + new CompoundUndoManager(editorPane, new BooleanProperty(true)); + + String inputString = "hello there"; + doc.insertString(0, inputString, new SimpleAttributeSet()); + try (CompoundUndoManager.AtomicModeManager ignored = undoManager.atomicMode()) { + doc.insertString(3, "xxxxxxxxxx", new SimpleAttributeSet()); + doc.remove(0, 12); + } + assertEquals( + "xlo there", + editorPane.getText(), + "Confirming added text contents after first atomic edit"); + try (CompoundUndoManager.AtomicModeManager ignored = undoManager.atomicMode()) { + doc.insertString(3, "yyyyy", new SimpleAttributeSet()); + doc.remove(5, 2); + } + assertEquals( + "xloyyy there", + editorPane.getText(), + "Confirming added text contents after second atomic edit"); + + undoManager.undo(); + assertEquals( + "xlo there", editorPane.getText(), "undo should apply only to edits in second atomic edit"); + undoManager.undo(); + assertEquals( + inputString, editorPane.getText(), "undo should apply only to edits in first atomic edit"); + + undoManager.redo(); + assertEquals("xlo there", editorPane.getText(), "redo should apply first atomic edit again"); + undoManager.redo(); + assertEquals( + "xloyyy there", editorPane.getText(), "redo should apply second atomic edit again"); + } + @Test public void allEditsInAtomicModeAreJoined() throws Exception { JEditorPane editorPane = new JEditorPane(); @@ -118,46 +158,47 @@ public void sequentialInsertionsAndDeletionsAreSeparated() throws BadLocationExc @Test public void startNewEditAfterDocumentSave() throws BadLocationException { - JEditorPane editorPane = new JEditorPane(); - BooleanProperty lastActionWasSave = new BooleanProperty(false); - CompoundUndoManager undoManager = - new CompoundUndoManager(editorPane, lastActionWasSave); - - editorPane.setText("hello world!"); - editorPane.setCaretPosition(5); - insertOneChar(editorPane, 'a'); - insertOneChar(editorPane, 'b'); - insertOneChar(editorPane, 'c'); - lastActionWasSave.set(true); - insertOneChar(editorPane, '1'); - assertFalse(lastActionWasSave.get(), "lastActionWasSave should be set to false on any undoable actions"); - insertOneChar(editorPane, '2'); - insertOneChar(editorPane, '3'); - lastActionWasSave.set(true); - insertOneChar(editorPane, 'x'); - insertOneChar(editorPane, 'y'); - insertOneChar(editorPane, 'z'); - lastActionWasSave.set(true); - - assertEquals("helloabc123xyz world!", editorPane.getText(), "Confirming fully-entered text"); - undoManager.undo(); - assertEquals("helloabc123 world!", editorPane.getText(), "First undo"); - undoManager.undo(); - assertEquals("helloabc world!", editorPane.getText(), "Second undo"); - undoManager.undo(); - assertEquals("hello world!", editorPane.getText(), "Third undo"); - - undoManager.redo(); - assertEquals("helloabc world!", editorPane.getText(), "Redo third undo"); - undoManager.redo(); - assertEquals("helloabc123 world!", editorPane.getText(), "Redo second undo"); - undoManager.redo(); - assertEquals("helloabc123xyz world!", editorPane.getText(), "Redo first undo"); + JEditorPane editorPane = new JEditorPane(); + BooleanProperty lastActionWasSave = new BooleanProperty(false); + CompoundUndoManager undoManager = new CompoundUndoManager(editorPane, lastActionWasSave); + + editorPane.setText("hello world!"); + editorPane.setCaretPosition(5); + insertOneChar(editorPane, 'a'); + insertOneChar(editorPane, 'b'); + insertOneChar(editorPane, 'c'); + lastActionWasSave.set(true); + insertOneChar(editorPane, '1'); + assertFalse( + lastActionWasSave.get(), + "lastActionWasSave should be set to false on any undoable actions"); + insertOneChar(editorPane, '2'); + insertOneChar(editorPane, '3'); + lastActionWasSave.set(true); + insertOneChar(editorPane, 'x'); + insertOneChar(editorPane, 'y'); + insertOneChar(editorPane, 'z'); + lastActionWasSave.set(true); + + assertEquals("helloabc123xyz world!", editorPane.getText(), "Confirming fully-entered text"); + undoManager.undo(); + assertEquals("helloabc123 world!", editorPane.getText(), "First undo"); + undoManager.undo(); + assertEquals("helloabc world!", editorPane.getText(), "Second undo"); + undoManager.undo(); + assertEquals("hello world!", editorPane.getText(), "Third undo"); + + undoManager.redo(); + assertEquals("helloabc world!", editorPane.getText(), "Redo third undo"); + undoManager.redo(); + assertEquals("helloabc123 world!", editorPane.getText(), "Redo second undo"); + undoManager.redo(); + assertEquals("helloabc123xyz world!", editorPane.getText(), "Redo first undo"); } @Test public void styleChangesAreJoined() { - // TODO: don't know how to add style change events + // TODO: don't know how to add style change events } //////////////// Util methods ///////////////////