Basic API Structure #2
Labels
No Label
1
13
2
21
3
34
5
55
8
bug
could have
documentation
duplicate
enhancement
help wanted
must have
question
should have
stale
wont have
L
M
S
XL
bug
bugfix
discussion
documentation
feature
maintenance
postponed
refactoring
wontfix
No Milestone
3 Participants
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: zdm/undo-redo#2
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "f/basics"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Imlpements the basic interfaces and classed needed to ensure the basic standard behaviour.
Basic API structureto Basic API StructureCode looks good, the Javadoc can be a bit more clear in some places. What we are missing are unit tests. Every method in the change manager should be thoroughly tested in a unit test.
@ -0,0 +2,4 @@
import java.util.*;
/**
The main description of the class is missing. I suggest something along the lines of:
"A change manager keeps track of subsequent changes and allows un- and redoing them. A specific position can be marked using {@link ...} to keep track of a saved state in the application that uses the manager."
@ -0,0 +3,4 @@
import java.util.*;
/**
* @param <C> the change types to store in this change manager
Replace
types
bytype
, as the user only specifies one parameter.@ -0,0 +15,4 @@
private int markedIndex;
/**
* Adds a change to the changes list.
"Applies the given change and appends it to the change list."
@ -0,0 +26,4 @@
}
/**
* Undoes the change at the current index position.
"Undoes the current change."
@ -0,0 +28,4 @@
/**
* Undoes the change at the current index position.
*
* @return whether the operation could be executed due to one being currently available
"whether an action was performed"
@ -0,0 +43,4 @@
/**
* Applies the change that was undone before.
*
* @return whether the operation could be executed due to one being currently available
"whether an action was performed"
@ -0,0 +56,4 @@
}
/**
* Marks the current index.
"Marks the current change."
@ -0,0 +65,4 @@
}
/**
* @return whether the current index was marked
"whether the current change is marked"
@ -0,0 +73,4 @@
}
/**
* @return whether the undo operation is currently available
"whether a change is present that can be undone"
@ -0,0 +81,4 @@
}
/**
* @return whether the redo operation is currently available.
"whether a change is present that can be redone"
I will implement my suggestions myself, as we were pair coding on this branch anyways.
@ -0,0 +11,4 @@
* @author Maximilian Käfer
* @since 0.0.1
*/
public final class ChangeManager<C extends Change> {
Do you want to keep it JavaFX free?
Are you sure that is the best way for what we intend to do?
Because if it stays like this we cannot use it in Taskforce as it simply does not do what we want because these things are not stored as properties.
I think it would be better if we used JavaFX as dependency with
<scope>provided</scope>
.That seems more expedient to me.
Would it be possible to use the library without JavaFX in that case?
Yes and no.
Without the dependency, we cannot use a property to automatically manage the changes, which is used i.e. to mark whether the graph is unsaved or automatically disable the action when no undo/ redo is available.
What we could instead do is offer our own Changelisteners for these things (simple consumers), and then emulate it for JavaFX by providing a change manager proxy based on JFX coding principles, meaning that it offers properties that automatically get updated with the new values.
So yes, it is possible, but a lot of overhead.
For that to work, would the JavaFX-based proxy simply be a separate class, or would it have to reside in a separate module? I thought about having such a module with JavaFX-based extensions of our API, but decided against it at first because of the the overhead.
Most likely a separate module.
Maven modules should help already a lot in this case.
@ -0,0 +37,4 @@
*/
public boolean undo() {
if (isUndoAvailable()) {
changes.get(index).invert().apply();
If we use it like that, why the fuck is that a
LinkedList
and not anArrayList
?This way, we run into serious performance issues when calling an element in the middle of the list. Let's just assume we want to retrieve the middle of an undo list that can store 1 000 000 entries. That means the list has to traverse 500 000 elements before getting to the requested element...
Also, if I see that correctly, our list is not limited in size currently.
I think we should use an iterator here. An array list wouldn't be nice either because of the continuous growth of the list.
I've already thought about that. Do you know what speaks against an iterator?
ConcurrentModificationException
.So no iterator for us.
Also, we can emulate the maximum growth by removing the oldest element before adding a new one when the maximum growth has been reached. This is needed anyway.
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/ListIterator.html#add(E)
@DieGurke and me decided to use the array list for simplicity's sake.
@ -0,0 +39,4 @@
if (isUndoAvailable()) {
changes.get(index).invert().apply();
--index;
return true;
boolean undoAvailable = ...;
if(undoAvailable) {
...
}
return undoAvailable;
I think it is better to save a variable instead of saving a line.
Agreed
Since when do we return literal boolean values when we already checked the condition needed to return them?
You, @kske, were the one who taught me this.
I'm not asking for it because it saves a line (which is a nice side effect I didn't even notice), I'm asking for it because you taught me that it is haram to use boolean literals as return values when there is no need for it.
I don't remember discussing this at length, but consider both approaches to be acceptable. I would have wrote the code myself like @DieGurke did because it is the straight-forward solution. If I would have to come up with actual reasons for it, the following ones come to mind:
undoAvailable
implies that, at the point at which it is returned, its value is equivalent to that ofisUndoAvailable()
. This is obviously not true, which could be considered confusing by some.In our case the method is quite small, so none of this really makes a difference. If you consider the other approach to be more elegant, which I can understand, we can use it instead.
I see both sides, but for the peace of this PR having to get merged, I ask you to resolve the open suggestions or write your answer if your are not willing to do so.
@ -0,0 +54,4 @@
if (isRedoAvailable()) {
changes.get(index + 1).apply();
++index;
return true;
Same as for undo.
Same as above
As already mentioned, I still think we should offer the change listeners even for the JavaFX free version, as they are useful for that kind of scenario as well.
If they are left unset, then it will be a null check that the JVM can optimize out.