zdm/undo-redo
Archived
3
Fork 0
Code Issues 2 Pull Requests Releases 1 Activity

JavaFX Integration #4

Merged
kske merged 4 commits from f/javafx into develop 2021-12-16 12:05:50 +01:00
Conversation 0 Commits 4 Files Changed 11 +31 -5
3 changed files with 31 additions and 5 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit d80dd94e90 - Show all commits

8
core/src/main/java/dev/kske/undoredo/core/ChangeManager.java
View File

@ -1,6 +1,6 @@
package dev.kske.undoredo.core; package dev.kske.undoredo.core;
import java.util.List; import java.util.*;
/** /**
* A change manager keeps track of subsequent changes and allows un- and redoing them. A specific * A change manager keeps track of subsequent changes and allows un- and redoing them. A specific
@ -22,6 +22,12 @@ public interface ChangeManager<C extends Change> {
*/ */
void addChange(C change); void addChange(C change);
/**
* @return the change that was applied last
* @since 0.0.1
*/
Optional<Change> getLastChange();
/** /**
* Undoes the current change. * Undoes the current change.
* *

5
core/src/main/java/dev/kske/undoredo/core/UnlimitedChangeManager.java
View File

@ -22,6 +22,11 @@ public final class UnlimitedChangeManager<C extends Change> implements ChangeMan
++index; ++index;
} }
@Override
public Optional<Change> getLastChange() {
return index == -1 ? Optional.empty() : Optional.of(changes.get(index));
kske marked this conversation as resolved Outdated
delvh commented 2021-12-15 12:00:38 +01:00
Outdated
Review
Copy Link

index < 0

`index < 0`
}
@Override @Override
public boolean undo() { public boolean undo() {
if (isUndoAvailable()) { if (isUndoAvailable()) {

23
core/src/test/java/dev/kske/undoredo/core/ChangeManagerTest.java
View File

@ -35,12 +35,25 @@ class ChangeManagerTest {
} }
/** /**
* Tests the consistency of the change list. * Tests retrieving the last change.
* *
* @since 0.0.1 * @since 0.0.1
*/ */
@Test @Test
@Order(2) @Order(2)
void testLastChange() {
assertTrue(manager.getLastChange().isEmpty());
manager.addChange(change);
assertEquals(change, manager.getLastChange().get());
}
/**
* Tests the consistency of the change list.
*
* @since 0.0.1
*/
@Test
@Order(3)
kske marked this conversation as resolved Outdated
delvh commented 2021-12-15 12:12:57 +01:00
Outdated
Review
Copy Link

I see you noticed the problem when adding a new test that has to be executed earlier:
You have to increment all subsequent priorities.
For this reason I propose another mechanism for using @Order: Instead of @Order(n), we should use @Order(10^n) for the tests that are created when creating the class.
Adding newer tests is then as simple as using @Order(10^n / i) where i is the amount of other elements with that order.
That way, when we have the basic tests, we don't have to subsequently change all indices below them when adding newer tests.

Take the following example:
@Order(1), @Order(2), @Order(3), @Order(4), @Order(5) are the tests we created initially.
Now, we add @Order(2b), @Order(3a), @Order(3c) and @Order(6). With our mechanism, that is pretty inefficient.
With my method, we would have initially
@Order(10), @Order(100), @Order(1 000), @Order(10 000), @Order(100 000).
Now, we simply add @Order(150), @Order(500), @Order(1 500) and @Order(1 000 000).
And then we're done.

I see you noticed the problem when adding a new test that has to be executed earlier: You have to increment all subsequent priorities. For this reason I propose another mechanism for using `@Order`: Instead of `@Order(n)`, we should use `@Order(10^n)` for the tests that are created when creating the class. Adding newer tests is then as simple as using `@Order(10^n / i)` where `i` is the amount of other elements with that order. That way, when we have the basic tests, we don't have to subsequently change all indices below them when adding newer tests. Take the following example: `@Order(1)`, `@Order(2)`, `@Order(3)`, `@Order(4)`, `@Order(5)` are the tests we created initially. Now, we add `@Order(2b)`, `@Order(3a)`, `@Order(3c)` and `@Order(6)`. With our mechanism, that is pretty inefficient. With my method, we would have initially `@Order(10)`, `@Order(100)`, `@Order(1 000)`, `@Order(10 000)`, `@Order(100 000)`. Now, we simply add `@Order(150)`, `@Order(500)`, `@Order(1 500)` and `@Order(1 000 000)`. And then we're done.
kske commented 2021-12-15 15:07:55 +01:00
Outdated
Review
Copy Link

How would you deal with a class that has more than nine tests?

How would you deal with a class that has more than nine tests?
delvh commented 2021-12-15 15:20:54 +01:00
Outdated
Review
Copy Link

Bring out my "division by 2" skills again.
Also, you first need to have 9 tests that depend on order.
In most cases, tests should not depend on a specific order.

Bring out my "division by 2" skills again. Also, you first need to have 9 tests that depend on order. In most cases, tests should not depend on a specific order.
kske commented 2021-12-15 15:27:17 +01:00
Outdated
Review
Copy Link

I would prefer having gaps of 10, or maybe 100, but with exponential gaps, the rule is unnecessarily complex and we can easily surpass Integer.MAX_VALUE / 2, which is the default order for JUnit tests, leading to unexpected results for tests without an explicitly defined order.

I would prefer having gaps of 10, or maybe 100, but with exponential gaps, the rule is unnecessarily complex and we can easily surpass `Integer.MAX_VALUE / 2`, which is the default order for JUnit tests, leading to unexpected results for tests without an explicitly defined order.
delvh commented 2021-12-15 17:24:51 +01:00
Outdated
Review
Copy Link

That's another possibility.
I don't care.
Should we use the notation @Order(10n) then?

That's another possibility. I don't care. Should we use the notation `@Order(10n)` then?
👍 1
void testGetChanges() { void testGetChanges() {
assertTrue(manager.getChanges().isEmpty()); assertTrue(manager.getChanges().isEmpty());
manager.addChange(change); manager.addChange(change);
@ -54,7 +67,7 @@ class ChangeManagerTest {
* @since 0.0.1 * @since 0.0.1
*/ */
@Test @Test
@Order(2) @Order(4)
void testUndo() { void testUndo() {
assertFalse(manager.isUndoAvailable()); assertFalse(manager.isUndoAvailable());
assertFalse(manager.undo()); assertFalse(manager.undo());
@ -63,6 +76,7 @@ class ChangeManagerTest {
assertTrue(manager.undo()); assertTrue(manager.undo());
assertFalse(manager.isUndoAvailable()); assertFalse(manager.isUndoAvailable());
assertFalse(manager.undo()); assertFalse(manager.undo());
assertTrue(manager.getLastChange().isEmpty());
} }
/** /**
@ -71,7 +85,7 @@ class ChangeManagerTest {
* @since 0.0.1 * @since 0.0.1
*/ */
@Test @Test
@Order(4) @Order(5)
void testRedo() { void testRedo() {
assertFalse(manager.isRedoAvailable()); assertFalse(manager.isRedoAvailable());
assertFalse(manager.redo()); assertFalse(manager.redo());
@ -83,6 +97,7 @@ class ChangeManagerTest {
assertTrue(manager.redo()); assertTrue(manager.redo());
assertFalse(manager.isRedoAvailable()); assertFalse(manager.isRedoAvailable());
assertFalse(manager.redo()); assertFalse(manager.redo());
assertEquals(change, manager.getLastChange().get());
} }
/** /**
@ -91,7 +106,7 @@ class ChangeManagerTest {
* @since 0.0.1 * @since 0.0.1
*/ */
@Test @Test
@Order(5) @Order(6)
void testMark() { void testMark() {
assertTrue(manager.isAtMarkedIndex()); assertTrue(manager.isAtMarkedIndex());
manager.addChange(change); manager.addChange(change);