JavaFX Integration #4

Merged
kske merged 4 commits from f/javafx into develop 2021-12-16 12:05:50 +01:00
5 changed files with 164 additions and 3 deletions
Showing only changes of commit 1b6d7f3dde - Show all commits

View File

@ -26,7 +26,7 @@ public interface ChangeManager<C extends Change> {
* @return the change that was applied last
* @since 0.0.1
*/
Optional<Change> getLastChange();
Optional<C> getLastChange();
/**
* Undoes the current change.

View File

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

View File

@ -0,0 +1,118 @@
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
Review
<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. ```
*
* @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
*/
public class ChangeManagerWrapper<C extends Change, M extends ChangeManager<C>>
implements ObservableChangeManager<C> {
protected ReadOnlyObjectWrapper<C> lastChange =
Review

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
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
Review

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.
new ReadOnlyObjectWrapper<>(this, "lastChange");
kske marked this conversation as resolved
Review

Perhaps we should extract these names into public static final constants to avoid confusion.

Perhaps we should extract these names into `public static final` constants to avoid confusion.
Review

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?
Review

Yes.

Yes.
Review

But what purpose should this have? When would it be used?

But what purpose should this have? When would it be used?
Review

Bindings.bind(someProperty, ChangeManagerWrapper.UndoAvailable) or however it is called.

`Bindings.bind(someProperty, ChangeManagerWrapper.UndoAvailable)` or however it is called.
protected ReadOnlyBooleanWrapper atMarkedIndex =
new ReadOnlyBooleanWrapper(this, "atMarkedIndex");
protected ReadOnlyBooleanWrapper undoAvailable =
new ReadOnlyBooleanWrapper(this, "undoAvailable");
protected ReadOnlyBooleanWrapper redoAvailable =
new ReadOnlyBooleanWrapper(this, "redoAvailable");
protected final M manager;
protected ChangeManagerWrapper(M manager) {
this.manager = manager;
}
@Override
public void addChange(C change) {
manager.addChange(change);
updateProperties();
}
@Override
public boolean undo() {
if (manager.undo()) {
updateProperties();
return true;
}
return false;
}
@Override
public boolean redo() {
if (manager.redo()) {
updateProperties();
return true;
}
return false;
delvh marked this conversation as resolved
Review

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.

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.
Review

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 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();
}
}

View File

@ -0,0 +1,43 @@
package dev.kske.undoredo.javafx;
import java.util.Optional;
import javafx.beans.property.*;
import dev.kske.undoredo.core.*;
/**
* @param <C> the change type to store in this change manager
* @author Kai S. K. Engelbart
* @since 0.0.1
*/
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();
}
}

View File

@ -9,5 +9,5 @@ module dev.kske.undoredo.javafx {
exports dev.kske.undoredo.javafx;
requires dev.kske.undoredo.core;
requires javafx.base;
requires transitive javafx.base;
}