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 +56 -20
7 changed files with 56 additions and 20 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 0f1c3e06d8 - Show all commits

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

@ -6,6 +6,10 @@ import java.util.*;
* A change manager keeps track of subsequent changes and allows un- and redoing them. A specific
* change can be marked using {@link #mark()} to keep track of a saved state in the application that
* uses the manager.
* <p>
* If you intend to listen to the state of a change manager, consider writing a wrapper
* implementation for an existing change manager that adds the necessary hooks. If you use JavaFX,
* take a look at the {@code dev.kske.undoredo.javafx} module.
*
* @param <C> the change type to store in this change manager
* @author Maximilian K&auml;fer

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

@ -24,7 +24,7 @@ public final class UnlimitedChangeManager<C extends Change> implements ChangeMan
@Override
public Optional<C> getLastChange() {
return index == -1 ? Optional.empty() : Optional.of(changes.get(index));
return index < 0 ? 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

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

@ -27,7 +27,7 @@ class ChangeManagerTest {
* @since 0.0.1
*/
@Test
@Order(1)
@Order(10)
void testAddChange() {
assertSame(0, wrapper.value);
manager.addChange(change);
@ -40,7 +40,7 @@ class ChangeManagerTest {
* @since 0.0.1
*/
@Test
@Order(2)
@Order(20)
void testLastChange() {
assertTrue(manager.getLastChange().isEmpty());
manager.addChange(change);
@ -53,7 +53,7 @@ class ChangeManagerTest {
* @since 0.0.1
*/
@Test
@Order(3)
@Order(30)
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() {
assertTrue(manager.getChanges().isEmpty());
manager.addChange(change);
@ -67,7 +67,7 @@ class ChangeManagerTest {
* @since 0.0.1
*/
@Test
@Order(4)
@Order(40)
void testUndo() {
assertFalse(manager.isUndoAvailable());
assertFalse(manager.undo());
@ -85,7 +85,7 @@ class ChangeManagerTest {
* @since 0.0.1
*/
@Test
@Order(5)
@Order(50)
void testRedo() {
assertFalse(manager.isRedoAvailable());
assertFalse(manager.redo());
@ -106,7 +106,7 @@ class ChangeManagerTest {
* @since 0.0.1
*/
@Test
@Order(6)
@Order(60)
void testMark() {
assertTrue(manager.isAtMarkedIndex());
manager.addChange(change);

26
javafx/src/main/java/dev/kske/undoredo/javafx/ChangeManagerWrapper.java
View File

@ -9,6 +9,11 @@ import dev.kske.undoredo.core.*;
/**
* Wraps an ordinary change manager into an observable change manager, providing the required
* properties for concrete implementations.
kske marked this conversation as resolved
delvh commented 2021-12-15 13:49:59 +01:00
Review
Copy Link
<p>
The properties have exactly the same name as their corresponding `-property()` methods and can be called i.e. as binding under exactly that name. Alternatively, the names are available as constants.
```java <p> The properties have exactly the same name as their corresponding `-property()` methods and can be called i.e. as binding under exactly that name. Alternatively, the names are available as constants. ```
* <p>
* The properties have the same name as their corresponding {@code -property()} methods and can be
* accessed reflectively from JavaFX, e.g. through
* {@link javafx.beans.binding.Bindings#select(Object, String...)}. Alternatively, the property
* names are available as constants.
*
* @param <C> the change type to store in this change manager
* @param <M> the type of change manager to wrap
@ -18,18 +23,29 @@ import dev.kske.undoredo.core.*;
public class ChangeManagerWrapper<C extends Change, M extends ChangeManager<C>>
implements ObservableChangeManager<C> {
public static final String LAST_CHANGE = "lastChange";
public static final String AT_MARKED_INDEX = "atMarkedIndex";
public static final String UNDO_AVAILABLE = "undoAvailable";
public static final String REDO_AVAILABLE = "redoAvailable";
protected ReadOnlyObjectWrapper<C> lastChange =
new ReadOnlyObjectWrapper<>(this, "lastChange");
new ReadOnlyObjectWrapper<>(this, LAST_CHANGE);
kske marked this conversation as resolved Outdated
delvh commented 2021-12-15 12:19:47 +01:00
Outdated
Review
Copy Link

Wait, protected?
How do you instantiate one then? Do you really have to create your own class? Isn't that a bit overkill?

Wait, protected? How do you instantiate one then? Do you really have to create your own class? Isn't that a bit overkill?
kske commented 2021-12-15 15:03:17 +01:00
Outdated
Review
Copy Link

I think the wrapper can be concrete, but should be open for extension in case we want a wrapper for a change manager with additional methods.

I think the wrapper can be concrete, but should be open for extension in case we want a wrapper for a change manager with additional methods.
👍 1
protected ReadOnlyBooleanWrapper atMarkedIndex =
new ReadOnlyBooleanWrapper(this, "atMarkedIndex");
new ReadOnlyBooleanWrapper(this, AT_MARKED_INDEX);
protected ReadOnlyBooleanWrapper undoAvailable =
new ReadOnlyBooleanWrapper(this, "undoAvailable");
new ReadOnlyBooleanWrapper(this, UNDO_AVAILABLE);
protected ReadOnlyBooleanWrapper redoAvailable =
new ReadOnlyBooleanWrapper(this, "redoAvailable");
new ReadOnlyBooleanWrapper(this, REDO_AVAILABLE);
protected final M manager;
protected ChangeManagerWrapper(M manager) {
/**
* Initializes a change manager wrapper.
*
* @param manager the change manager to wrap
* @since 0.0.1
*/
public ChangeManagerWrapper(M manager) {
this.manager = manager;
}

4
javafx/src/main/java/dev/kske/undoredo/javafx/ObservableChangeManager.java
View File

@ -7,9 +7,13 @@ import javafx.beans.property.*;
import dev.kske.undoredo.core.*;
/**
* A change manager that exposes its state through JavaFX properties, thereby allowing a direct
kske marked this conversation as resolved Outdated
delvh commented 2021-12-15 12:02:02 +01:00
Outdated
Review
Copy Link

Purpose of this interface?

Purpose of this interface?
* integration of Undo-Redo with JavaFX listeners and property bindings.
*
* @param <C> the change type to store in this change manager
* @author Kai S. K. Engelbart
* @since 0.0.1
* @see ChangeManagerWrapper
*/
public interface ObservableChangeManager<C extends Change> extends ChangeManager<C> {

4
javafx/src/main/java/module-info.java
View File

@ -8,6 +8,8 @@ module dev.kske.undoredo.javafx {
exports dev.kske.undoredo.javafx;
requires dev.kske.undoredo.core;
opens dev.kske.undoredo.javafx to javafx.base;
kske marked this conversation as resolved Outdated
delvh commented 2021-12-15 12:03:21 +01:00
Outdated
Review
Copy Link

requires transitive?

`requires transitive`?
kske commented 2021-12-15 15:00:23 +01:00
Outdated
Review
Copy Link

The core module doesn't have any dependencies, so this shouldn't make a difference.

The core module doesn't have any dependencies, so this shouldn't make a difference.
delvh commented 2021-12-15 15:17:25 +01:00
Outdated
Review
Copy Link

I think transitive is meant for the other way around:
Not that we require all dependencies from that module,
but that modules requiring our module also automatically require the core module.

I think `transitive` is meant for the other way around: Not that we require all dependencies from that module, but that modules requiring our module also automatically require the `core` module.
requires transitive dev.kske.undoredo.core;
requires transitive javafx.base;
}

10
pom.xml
View File

@ -48,6 +48,16 @@
</roles>
<timezone>Europe/Berlin</timezone>
</developer>
<developer>
<name>Leon Hofmeister</name>
<email>leon@kske.dev</email>
<url>https://git.kske.dev/delvh</url>
<roles>
<role>architect</role>
<role>developer</role>
</roles>
<timezone>Europe/Berlin</timezone>
</developer>
</developers>
<scm>