JavaFX Integration #4
@ -1,11 +1,15 @@
|
||||
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
|
||||
* 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äfer
|
||||
@ -22,6 +26,12 @@ public interface ChangeManager<C extends Change> {
|
||||
*/
|
||||
void addChange(C change);
|
||||
|
||||
/**
|
||||
* @return the change that was applied last
|
||||
* @since 0.0.1
|
||||
*/
|
||||
Optional<C> getLastChange();
|
||||
|
||||
/**
|
||||
* Undoes the current change.
|
||||
*
|
||||
|
@ -22,6 +22,11 @@ public final class UnlimitedChangeManager<C extends Change> implements ChangeMan
|
||||
++index;
|
||||
}
|
||||
|
||||
@Override
|
||||
public Optional<C> getLastChange() {
|
||||
return index < 0 ? Optional.empty() : Optional.of(changes.get(index));
|
||||
kske marked this conversation as resolved
Outdated
|
||||
}
|
||||
|
||||
@Override
|
||||
public boolean undo() {
|
||||
if (isUndoAvailable()) {
|
||||
|
@ -27,20 +27,33 @@ class ChangeManagerTest {
|
||||
* @since 0.0.1
|
||||
*/
|
||||
@Test
|
||||
@Order(1)
|
||||
@Order(10)
|
||||
void testAddChange() {
|
||||
assertSame(0, wrapper.value);
|
||||
manager.addChange(change);
|
||||
assertSame(1, wrapper.value);
|
||||
}
|
||||
|
||||
/**
|
||||
* Tests retrieving the last change.
|
||||
*
|
||||
* @since 0.0.1
|
||||
*/
|
||||
@Test
|
||||
@Order(20)
|
||||
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(2)
|
||||
@Order(30)
|
||||
kske marked this conversation as resolved
Outdated
delvh
commented
I see you noticed the problem when adding a new test that has to be executed earlier: Take the following example: 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
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
Bring out my "division by 2" skills again. 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
I would prefer having gaps of 10, or maybe 100, but with exponential gaps, the rule is unnecessarily complex and we can easily surpass 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
That's another possibility. That's another possibility.
I don't care.
Should we use the notation `@Order(10n)` then?
|
||||
void testGetChanges() {
|
||||
assertTrue(manager.getChanges().isEmpty());
|
||||
manager.addChange(change);
|
||||
@ -54,7 +67,7 @@ class ChangeManagerTest {
|
||||
* @since 0.0.1
|
||||
*/
|
||||
@Test
|
||||
@Order(2)
|
||||
@Order(40)
|
||||
void testUndo() {
|
||||
assertFalse(manager.isUndoAvailable());
|
||||
assertFalse(manager.undo());
|
||||
@ -63,6 +76,7 @@ class ChangeManagerTest {
|
||||
assertTrue(manager.undo());
|
||||
assertFalse(manager.isUndoAvailable());
|
||||
assertFalse(manager.undo());
|
||||
assertTrue(manager.getLastChange().isEmpty());
|
||||
}
|
||||
|
||||
/**
|
||||
@ -71,7 +85,7 @@ class ChangeManagerTest {
|
||||
* @since 0.0.1
|
||||
*/
|
||||
@Test
|
||||
@Order(4)
|
||||
@Order(50)
|
||||
void testRedo() {
|
||||
assertFalse(manager.isRedoAvailable());
|
||||
assertFalse(manager.redo());
|
||||
@ -83,6 +97,7 @@ class ChangeManagerTest {
|
||||
assertTrue(manager.redo());
|
||||
assertFalse(manager.isRedoAvailable());
|
||||
assertFalse(manager.redo());
|
||||
assertEquals(change, manager.getLastChange().get());
|
||||
}
|
||||
|
||||
/**
|
||||
@ -91,7 +106,7 @@ class ChangeManagerTest {
|
||||
* @since 0.0.1
|
||||
*/
|
||||
@Test
|
||||
@Order(5)
|
||||
@Order(60)
|
||||
void testMark() {
|
||||
assertTrue(manager.isAtMarkedIndex());
|
||||
manager.addChange(change);
|
||||
|
27
javafx/.classpath
Normal file
@ -0,0 +1,27 @@
|
||||
<?xml version="1.0" encoding="UTF-8"?>
|
||||
<classpath>
|
||||
<classpathentry kind="src" output="target/classes" path="src/main/java">
|
||||
<attributes>
|
||||
<attribute name="optional" value="true"/>
|
||||
<attribute name="maven.pomderived" value="true"/>
|
||||
</attributes>
|
||||
</classpathentry>
|
||||
<classpathentry kind="src" output="target/test-classes" path="src/test/java">
|
||||
<attributes>
|
||||
<attribute name="optional" value="true"/>
|
||||
<attribute name="maven.pomderived" value="true"/>
|
||||
<attribute name="test" value="true"/>
|
||||
</attributes>
|
||||
</classpathentry>
|
||||
<classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER/org.eclipse.jdt.internal.debug.ui.launcher.StandardVMType/JavaSE-11">
|
||||
<attributes>
|
||||
<attribute name="maven.pomderived" value="true"/>
|
||||
</attributes>
|
||||
</classpathentry>
|
||||
<classpathentry kind="con" path="org.eclipse.m2e.MAVEN2_CLASSPATH_CONTAINER">
|
||||
<attributes>
|
||||
<attribute name="maven.pomderived" value="true"/>
|
||||
</attributes>
|
||||
</classpathentry>
|
||||
<classpathentry kind="output" path="target/classes"/>
|
||||
</classpath>
|
23
javafx/.project
Normal file
@ -0,0 +1,23 @@
|
||||
<?xml version="1.0" encoding="UTF-8"?>
|
||||
<projectDescription>
|
||||
<name>undo-redo-javafx</name>
|
||||
<comment></comment>
|
||||
<projects>
|
||||
</projects>
|
||||
<buildSpec>
|
||||
<buildCommand>
|
||||
<name>org.eclipse.jdt.core.javabuilder</name>
|
||||
<arguments>
|
||||
</arguments>
|
||||
</buildCommand>
|
||||
<buildCommand>
|
||||
<name>org.eclipse.m2e.core.maven2Builder</name>
|
||||
<arguments>
|
||||
</arguments>
|
||||
</buildCommand>
|
||||
</buildSpec>
|
||||
<natures>
|
||||
<nature>org.eclipse.jdt.core.javanature</nature>
|
||||
<nature>org.eclipse.m2e.core.maven2Nature</nature>
|
||||
</natures>
|
||||
</projectDescription>
|
28
javafx/pom.xml
Normal file
@ -0,0 +1,28 @@
|
||||
<project xmlns="http://maven.apache.org/POM/4.0.0"
|
||||
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
|
||||
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>
|
||||
<name>Undo-Redo JavaFX Integration</name>
|
||||
|
||||
<parent>
|
||||
<groupId>dev.kske</groupId>
|
||||
<artifactId>undo-redo</artifactId>
|
||||
<version>0.0.1-SNAPSHOT</version>
|
||||
</parent>
|
||||
|
||||
<dependencies>
|
||||
<dependency>
|
||||
<groupId>dev.kske</groupId>
|
||||
<artifactId>undo-redo-core</artifactId>
|
||||
<version>0.0.1-SNAPSHOT</version>
|
||||
</dependency>
|
||||
<dependency>
|
||||
<groupId>org.openjfx</groupId>
|
||||
<artifactId>javafx-base</artifactId>
|
||||
<version>11</version>
|
||||
delvh
commented
Shouldn't it be Otherwise we run into version problems... Shouldn't it be `provided`?
Otherwise we run into version problems...
kske
commented
A project using this library should be able to use any JavaFX version >= 11. A fixed version would be defined like this: A project using this library should be able to use any JavaFX version >= 11. A fixed version would be defined like this: `[11]`. `<scope>provided</scope>` assumes that the dependency is present in some kind of runtime container, like the JDK or a Java EE container, which isn't the case here.
delvh
commented
I thought that was exactly the goal behind I thought that was exactly the goal behind `provided`: To declare that any version of JavaFX is possible, as long as it is present...
kske
commented
I don't think the scope has anything to do with the version: https://maven.apache.org/guides/introduction/introduction-to-dependency-mechanism.html#Dependency_Scope There is a possibility to specify version ranger, but the default should be sufficient for us: https://docs.oracle.com/middleware/1212/core/MAVEN/maven_version.htm#MAVEN402 I don't think the scope has anything to do with the version: https://maven.apache.org/guides/introduction/introduction-to-dependency-mechanism.html#Dependency_Scope
There is a possibility to specify version ranger, but the default should be sufficient for us: https://docs.oracle.com/middleware/1212/core/MAVEN/maven_version.htm#MAVEN402
|
||||
</dependency>
|
||||
</dependencies>
|
||||
|
||||
</project>
|
@ -0,0 +1,134 @@
|
||||
package dev.kske.undoredo.javafx;
|
||||
|
||||
import java.util.List;
|
||||
|
||||
import javafx.beans.property.*;
|
||||
|
||||
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
```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
|
||||
* @author Kai S. K. Engelbart
|
||||
* @since 0.0.1
|
||||
delvh
commented
I have to say, I like that you were even able to implement it without having to use listeners. I also like that our implementation seems to be
I have to say, I like that you were even able to implement it without having to use listeners.
Maybe we should add a paragraph in the Javadoc of `ChangeManager` that that is the preferred way to listen to changes.
I also like that our implementation seems to be
1. less filesystem-space consuming (amount of classes and modules)
2. less memory consuming during runtime (we only use the necessary objects unlike `UndoFX` which creates an object storing the current position whenever you undo or redo something...)
3. far more readable (I have no idea what UndoFX is doing at any point)
4. without weird dependencies that make no sense at all
5. conformant with JavaFX coding standard
6. more performant as there are no listeners and callbacks and equals that are always tested
kske
commented
UndoFX might be more flexible in some cases (e.g. non-linear history and such), but for our and most other purposes this solution should be more than enough. UndoFX might be more flexible in some cases (e.g. non-linear history and such), but for our and most other purposes this solution should be more than enough.
|
||||
*/
|
||||
kske marked this conversation as resolved
delvh
commented
Perhaps we should extract these names into Perhaps we should extract these names into `public static final` constants to avoid confusion.
kske
commented
You mean the string literal? So for every property that is declared we would declare another constant? You mean the string literal? So for every property that is declared we would declare another constant?
delvh
commented
Yes. Yes.
kske
commented
But what purpose should this have? When would it be used? But what purpose should this have? When would it be used?
delvh
commented
`Bindings.bind(someProperty, ChangeManagerWrapper.UndoAvailable)` or however it is called.
|
||||
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, LAST_CHANGE);
|
||||
kske marked this conversation as resolved
Outdated
delvh
commented
Wait, protected? 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
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.
|
||||
protected ReadOnlyBooleanWrapper atMarkedIndex =
|
||||
new ReadOnlyBooleanWrapper(this, AT_MARKED_INDEX);
|
||||
protected ReadOnlyBooleanWrapper undoAvailable =
|
||||
new ReadOnlyBooleanWrapper(this, UNDO_AVAILABLE);
|
||||
protected ReadOnlyBooleanWrapper redoAvailable =
|
||||
new ReadOnlyBooleanWrapper(this, REDO_AVAILABLE);
|
||||
|
||||
protected final 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;
|
||||
}
|
||||
|
||||
@Override
|
||||
public void addChange(C change) {
|
||||
manager.addChange(change);
|
||||
updateProperties();
|
||||
}
|
||||
|
||||
delvh marked this conversation as resolved
delvh
commented
Wait, what was the result of the last discussion we had about that? Wait, what was the result of the last discussion we had about that?
I can't remember, and I don't remember either which PR in which project it was where we talked about that.
kske
commented
We discussed this in #2. I think @DieGurke resolved the suggestion as we didn't reach a conclusion after stating our opinions. We discussed this in #2. I think @DieGurke resolved the suggestion as we didn't reach a conclusion after stating our opinions.
|
||||
@Override
|
||||
public boolean undo() {
|
||||
if (manager.undo()) {
|
||||
updateProperties();
|
||||
return true;
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
@Override
|
||||
public boolean redo() {
|
||||
if (manager.redo()) {
|
||||
updateProperties();
|
||||
return true;
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
@Override
|
||||
public void mark() {
|
||||
manager.mark();
|
||||
setAtMarkedIndex(manager.isAtMarkedIndex());
|
||||
}
|
||||
|
||||
/**
|
||||
* Sets the values of all properties to those present in the wrapped change manager.
|
||||
*
|
||||
* @since 0.0.1
|
||||
*/
|
||||
private void updateProperties() {
|
||||
setLastChange(manager.getLastChange().orElse(null));
|
||||
setAtMarkedIndex(manager.isAtMarkedIndex());
|
||||
setUndoAvailable(manager.isUndoAvailable());
|
||||
setRedoAvailable(manager.isRedoAvailable());
|
||||
}
|
||||
|
||||
@Override
|
||||
public final ReadOnlyObjectProperty<C> lastChangeProperty() {
|
||||
return lastChange.getReadOnlyProperty();
|
||||
}
|
||||
|
||||
protected final void setLastChange(C lastChange) {
|
||||
this.lastChange.set(lastChange);
|
||||
}
|
||||
|
||||
@Override
|
||||
public final ReadOnlyBooleanProperty atMarkedIndexProperty() {
|
||||
return atMarkedIndex.getReadOnlyProperty();
|
||||
}
|
||||
|
||||
protected final void setAtMarkedIndex(boolean atMarkedIndex) {
|
||||
this.atMarkedIndex.set(atMarkedIndex);
|
||||
}
|
||||
|
||||
@Override
|
||||
public final ReadOnlyBooleanProperty undoAvailableProperty() {
|
||||
return undoAvailable.getReadOnlyProperty();
|
||||
}
|
||||
|
||||
protected final void setUndoAvailable(boolean undoAvailable) {
|
||||
this.undoAvailable.set(undoAvailable);
|
||||
}
|
||||
|
||||
@Override
|
||||
public final ReadOnlyBooleanProperty redoAvailableProperty() {
|
||||
return redoAvailable.getReadOnlyProperty();
|
||||
}
|
||||
|
||||
protected final void setRedoAvailable(boolean redoAvailable) {
|
||||
this.redoAvailable.set(redoAvailable);
|
||||
}
|
||||
|
||||
@Override
|
||||
public List<C> getChanges() {
|
||||
return manager.getChanges();
|
||||
}
|
||||
}
|
@ -0,0 +1,47 @@
|
||||
package dev.kske.undoredo.javafx;
|
||||
|
||||
import java.util.Optional;
|
||||
|
||||
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
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> {
|
||||
|
||||
ReadOnlyObjectProperty<C> lastChangeProperty();
|
||||
|
||||
@Override
|
||||
default Optional<C> getLastChange() {
|
||||
return Optional.of(lastChangeProperty().get());
|
||||
}
|
||||
|
||||
ReadOnlyBooleanProperty atMarkedIndexProperty();
|
||||
|
||||
@Override
|
||||
default boolean isAtMarkedIndex() {
|
||||
return atMarkedIndexProperty().get();
|
||||
}
|
||||
|
||||
ReadOnlyBooleanProperty undoAvailableProperty();
|
||||
|
||||
@Override
|
||||
default boolean isUndoAvailable() {
|
||||
return undoAvailableProperty().get();
|
||||
}
|
||||
|
||||
ReadOnlyBooleanProperty redoAvailableProperty();
|
||||
|
||||
@Override
|
||||
default boolean isRedoAvailable() {
|
||||
return redoAvailableProperty().get();
|
||||
}
|
||||
}
|
@ -0,0 +1,7 @@
|
||||
/**
|
||||
* Contains JavaFX-based wrapper API for integrating Undo-Redo with JavaFX.
|
||||
*
|
||||
* @author Kai S. K. Engelbart
|
||||
* @since 0.0.1
|
||||
*/
|
||||
package dev.kske.undoredo.javafx;
|
15
javafx/src/main/java/module-info.java
Normal file
@ -0,0 +1,15 @@
|
||||
/**
|
||||
* Contains JavaFX-based wrapper API for integrating Undo-Redo with JavaFX.
|
||||
*
|
||||
* @author Kai S. K. Engelbart
|
||||
* @since 0.0.1
|
||||
*/
|
||||
module dev.kske.undoredo.javafx {
|
||||
|
||||
exports dev.kske.undoredo.javafx;
|
||||
|
||||
opens dev.kske.undoredo.javafx to javafx.base;
|
||||
kske marked this conversation as resolved
Outdated
delvh
commented
`requires transitive`?
kske
commented
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
I think 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;
|
||||
}
|
11
pom.xml
@ -14,6 +14,7 @@
|
||||
|
||||
<modules>
|
||||
<module>core</module>
|
||||
<module>javafx</module>
|
||||
</modules>
|
||||
|
||||
<licenses>
|
||||
@ -47,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>
|
||||
|
index < 0