Basic API Structure #2

Merged
kske merged 12 commits from f/basics into develop 2021-12-11 21:45:59 +01:00
2 changed files with 133 additions and 0 deletions
Showing only changes of commit fe1009187c - Show all commits

View File

@ -0,0 +1,33 @@
package dev.kske.undoredo;
/**
* Base interface for changes to be registered in an undo manager.
*
* @author Maximilian Käfer
* @since 0.0.1
*/
public interface Change {
/**
* Performs the action implemented by this change.
*
* @since 0.0.1
*/
void apply();
/**
* Inverts this change.
*
* @implSpec This method is not supposed to alter the state of this change, but rather to create
* a new complementary change.
* @return the inverted change
* @since 0.0.1
*/
Change invert();
/**
* @return whether the application of this change would result in an identical state
* @since 0.0.1
*/
boolean isIdentity();
}

View File

@ -0,0 +1,100 @@
package dev.kske.undoredo;
import java.util.*;
/**
kske marked this conversation as resolved Outdated
Outdated
Review

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

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."
* @param <C> the change types to store in this change manager
kske marked this conversation as resolved Outdated
Outdated
Review

Replace types by type, as the user only specifies one parameter.

Replace `types` by `type`, as the user only specifies one parameter.
* @author Maximilian K&auml;fer
* @since 0.0.1
*/
public final class ChangeManager<C extends Change> {
private final List<C> changes = new LinkedList<>();
private int index;
kske marked this conversation as resolved Outdated
Outdated
Review

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.

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

Would it be possible to use the library without JavaFX in that case?

Would it be possible to use the library without JavaFX in that case?
Outdated
Review

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.

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

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.

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

Most likely a separate module.
Maven modules should help already a lot in this case.

Most likely a separate module. Maven modules should help already a lot in this case.
private int markedIndex;
/**
* Adds a change to the changes list.
kske marked this conversation as resolved Outdated
Outdated
Review

"Applies the given change and appends it to the change list."

"Applies the given change and appends it to the change list."
*
* @param change the change to add
* @since 0.0.1
*/
public void addChange(C change) {
change.apply();
changes.add(change);
}
/**
* Undoes the change at the current index position.
kske marked this conversation as resolved Outdated
Outdated
Review

"Undoes the current change."

"Undoes the current change."
*
* @return whether the operation could be executed due to one being currently available
kske marked this conversation as resolved Outdated
Outdated
Review

"whether an action was performed"

"whether an action was performed"
* @since 0.1.0
*/
public boolean undo() {
if (isUndoAvailable()) {
changes.get(index).invert().apply();
--index;
return true;
}
return false;
kske marked this conversation as resolved Outdated
Outdated
Review

If we use it like that, why the fuck is that a LinkedList and not an ArrayList?

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.

If we use it like that, why the fuck is that a `LinkedList` and not an `ArrayList`? 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.
Outdated
Review

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 think we should use an iterator here. An array list wouldn't be nice either because of the continuous growth of the list.
Outdated
Review

I've already thought about that. Do you know what speaks against an iterator?
ConcurrentModificationException.
So no iterator for us.

I've already thought about that. Do you know what speaks against an iterator? `ConcurrentModificationException`. So no iterator for us.
Outdated
Review

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.

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.
Outdated
Review
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/ListIterator.html#add(E)
Outdated
Review

@DieGurke and me decided to use the array list for simplicity's sake.

@DieGurke and me decided to use the array list for simplicity's sake.
}
mpk marked this conversation as resolved Outdated
Outdated
Review

boolean undoAvailable = ...;
if(undoAvailable) {
...
}
return undoAvailable;

boolean undoAvailable = ...; if(undoAvailable) { ... } return undoAvailable;
Outdated
Review

I think it is better to save a variable instead of saving a line.

I think it is better to save a variable instead of saving a line.
Outdated
Review

Agreed

Agreed
Outdated
Review

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.

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

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:

  • One variable less is required.
  • When looking at the return statements, it is immediately obvious what is returned.
  • Naming the variable undoAvailable implies that, at the point at which it is returned, its value is equivalent to that of isUndoAvailable(). 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 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: * One variable less is required. * When looking at the return statements, it is immediately obvious what is returned. * Naming the variable `undoAvailable` implies that, at the point at which it is returned, its value is equivalent to that of `isUndoAvailable()`. 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.
Outdated
Review

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.

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.
/**
* Applies the change that was undone before.
*
* @return whether the operation could be executed due to one being currently available
kske marked this conversation as resolved Outdated
Outdated
Review

"whether an action was performed"

"whether an action was performed"
* @since 0.0.1
*/
public boolean redo() {
if (isRedoAvailable()) {
changes.get(index + 1).apply();
++index;
return true;
}
return false;
}
mpk marked this conversation as resolved Outdated
Outdated
Review

Same as for undo.

Same as for undo.
Outdated
Review

Same as above

Same as above
/**
* Marks the current index.
kske marked this conversation as resolved Outdated
Outdated
Review

"Marks the current change."

"Marks the current change."
*
* @since 0.0.1
*/
public void mark() {
markedIndex = index;
}
/**
* @return whether the current index was marked
kske marked this conversation as resolved Outdated
Outdated
Review

"whether the current change is marked"

"whether the current change is marked"
* @since 0.0.1
*/
public boolean isAtMarkedIndex() {
return markedIndex == index;
}
/**
* @return whether the undo operation is currently available
kske marked this conversation as resolved Outdated
Outdated
Review

"whether a change is present that can be undone"

"whether a change is present that can be undone"
* @since 0.0.1
*/
public boolean isUndoAvailable() {
return index > 0;
}
/**
* @return whether the redo operation is currently available.
kske marked this conversation as resolved Outdated
Outdated
Review

"whether a change is present that can be redone"

"whether a change is present that can be redone"
* @since 0.0.1
*/
public boolean isRedoAvailable() {
return index < changes.size() - 1;
}
/**
* Provides an unmodifiable view of the changes stored in this change manager.
*
* @return all stored changes
* @since 0.0.1
*/
public List<C> getChanges() {
return Collections.unmodifiableList(changes);
}
}