Correctly Deal with Identity Changes and Divergent History #5

Merged
kske merged 3 commits from b/divergent-history into develop 2021-12-24 22:16:46 +01:00
8 changed files with 120 additions and 13 deletions

View File

@ -2,7 +2,7 @@ package dev.kske.undoredo.core;
/**
* Base interface for changes to be registered in an undo manager.
*
*
* @author Maximilian Käfer
* @since 0.0.1
*/
@ -17,7 +17,7 @@ public interface Change {
/**
* Inverts this change.
*
*
* @implSpec This method is not supposed to alter the state of this change, but rather to create
* a new complementary change.
* @return the inverted change
@ -26,6 +26,8 @@ public interface Change {
Change invert();
/**
* @apiNote If this method returns {@code true} when adding this change to a change manager, it
* will be discarded immediately and therefore can be garbage collected.
* @return whether the application of this change would result in an identical state
* @since 0.0.1
*/

View File

@ -19,12 +19,14 @@ import java.util.*;
public interface ChangeManager<C extends Change> {
/**
* Applies the given change and appends it to the change list.
* Applies the given change and appends it to the change list. If the change is an identity, no
* action is taken.
*
* @param change the change to add
* @return whether the change has been added
* @since 0.0.1
*/
void addChange(C change);
boolean addChange(C change);
/**
* @return the change that was applied last

View File

@ -3,6 +3,8 @@ package dev.kske.undoredo.core;
import java.util.*;
/**
* A simple change manager with a linear history model backed by an array list.
*
* @param <C> the change type to store in this change manager
* @author Maximilian K&auml;fer
* @author Kai S. K. Engelbart
@ -15,11 +17,23 @@ public final class UnlimitedChangeManager<C extends Change> implements ChangeMan
private int index = -1;
private int markedIndex = -1;
/**
* @implNode As this change manager uses a linear history model, all changes behind the last
delvh marked this conversation as resolved
Review

I can definitely see needing this Javadoc more often.
What about implementing a superclass called LinearHistoryChangeManager that automatically supplies the Javadoc and general implementations of functions (undo, redo, and addChange given a new protected abstract function removeDivergentChanges)?

I can definitely see needing this Javadoc more often. What about implementing a superclass called `LinearHistoryChangeManager` that automatically supplies the Javadoc and general implementations of functions (`undo`, `redo`, and `addChange` given a new protected abstract function `removeDivergentChanges`)?
Review

I think we need to create more change manager implementations before deciding what belongs in the superclass and what doesn't. In any case, this branch is probably the wrong place add this.

I think we need to create more change manager implementations before deciding what belongs in the superclass and what doesn't. In any case, this branch is probably the wrong place add this.
* applied change will be discarded and therefore can be garbage collected.
*/
@Override
public void addChange(C change) {
change.apply();
changes.add(change);
++index;
public boolean addChange(C change) {
if (!change.isIdentity()) {
change.apply();
// Remove divergent changes
changes.subList(index + 1, changes.size()).clear();
changes.add(change);
++index;
return true;
}
return false;
}
@Override

View File

@ -14,11 +14,22 @@ class ChangeManagerTest {
IntWrapper wrapper;
IntChange change;
/**
* Generates an int change with the given value.
*
* @param value the value of the change
* @return the created change
* @since 0.0.1
*/
IntChange change(int value) {
return new IntChange(wrapper, value);
}
@BeforeEach
void prepareChangeManager() {
manager = new UnlimitedChangeManager<>();
wrapper = new IntWrapper();
change = new IntChange(wrapper, 1);
change = change(1);
}
/**
@ -30,7 +41,20 @@ class ChangeManagerTest {
@Order(10)
void testAddChange() {
assertSame(0, wrapper.value);
assertTrue(manager.addChange(change));
assertSame(1, wrapper.value);
}
/**
* Tests whether identity changes are ignored when adding them to the change manager.
*
* @since 0.0.1
*/
@Test
@Order(15)
void testDiscardIdentity() {
manager.addChange(change);
assertFalse(manager.addChange(change(0)));
assertSame(1, wrapper.value);
}
@ -100,6 +124,24 @@ class ChangeManagerTest {
assertEquals(change, manager.getLastChange().get());
}
/**
kske marked this conversation as resolved
Review

Perhaps you should
a) test by adding two changes that are undone
b) Add an assertSame for the size of changes stored

Perhaps you should a) test by adding two changes that are undone b) Add an assertSame for the size of changes stored
Review

Good point. I actually discovered a bug through this. The fix is coming in the next commit.

Good point. I actually discovered a bug through this. The fix is coming in the next commit.
* Tests whether the changes after the current index are discarded when adding a change.
*
* @since 0.0.1
*/
@Test
@Order(55)
void testDiscardDivergentHistory() {
manager.addChange(change);
manager.addChange(change(2));
manager.undo();
manager.undo();
assertTrue(manager.addChange(change(3)));
assertFalse(manager.isRedoAvailable());
assertSame(3, wrapper.value);
assertSame(1, manager.getChanges().size());
}
/**
* Tests marking a change.
*

View File

@ -1,5 +1,7 @@
package dev.kske.undoredo.core;
import java.util.Objects;
/**
* @author Kai S. K. Engelbart
* @since 0.0.1
@ -28,4 +30,24 @@ class IntChange implements Change {
public boolean isIdentity() {
return value == 0;
}
@Override
public String toString() {
return String.valueOf(value);
}
@Override
public int hashCode() {
return Objects.hash(value, wrapper);
}
@Override
public boolean equals(Object obj) {
if (this == obj)
return true;
if (!(obj instanceof IntChange))
return false;
IntChange other = (IntChange) obj;
return value == other.value && Objects.equals(wrapper, other.wrapper);
}
}

View File

@ -1,5 +1,7 @@
package dev.kske.undoredo.core;
import java.util.Objects;
/**
* @author Kai S. K. Engelbart
* @since 0.0.1
@ -7,4 +9,24 @@ package dev.kske.undoredo.core;
class IntWrapper {
int value;
@Override
public String toString() {
return String.valueOf(value);
}
@Override
public int hashCode() {
return Objects.hash(value);
}
@Override
public boolean equals(Object obj) {
if (this == obj)
return true;
if (!(obj instanceof IntWrapper))
return false;
IntWrapper other = (IntWrapper) obj;
return value == other.value;
}
}

View File

@ -3,7 +3,7 @@
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>
<artifactId>javafx</artifactId>
<artifactId>undo-redo-javafx</artifactId>
<name>Undo-Redo JavaFX Integration</name>
<parent>

View File

@ -50,9 +50,12 @@ public class ChangeManagerWrapper<C extends Change, M extends ChangeManager<C>>
}
@Override
public void addChange(C change) {
manager.addChange(change);
updateProperties();
public boolean addChange(C change) {
if (manager.addChange(change)) {
updateProperties();
return true;
}
return false;
}
@Override