zdm/undo-redo
Archived
3
Fork 0
Code Issues 2 Pull Requests Releases 1 Activity

Basic API Structure #2

Merged
kske merged 12 commits from f/basics into develop 2021-12-11 21:45:59 +01:00
Conversation 1 Commits 12 Files Changed 15 +81 -44
3 changed files with 81 additions and 44 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit f82df2b979 - Show all commits

54
src/main/java/dev/kske/undoredo/ChangeManager.java
View File

@ -1,6 +1,6 @@
package dev.kske.undoredo;
import java.util.*;
import java.util.List;
/**
kske marked this conversation as resolved Outdated
kske commented 2021-12-07 21:13:37 +01:00
Outdated
Review
Copy Link

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."
* A change manager keeps track of subsequent changes and allows un- and redoing them. A specific
kske marked this conversation as resolved Outdated
kske commented 2021-12-07 21:10:15 +01:00
Outdated
Review
Copy Link

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

Replace `types` by `type`, as the user only specifies one parameter.
@ -9,14 +9,10 @@ import java.util.*;
*
* @param <C> the change type to store in this change manager
* @author Maximilian K&auml;fer
* @author Kai S. K. Engelbart
* @since 0.0.1
*/
kske marked this conversation as resolved Outdated
delvh commented 2021-12-09 12:09:12 +01:00
Outdated
Review
Copy Link

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.
kske commented 2021-12-09 18:32:47 +01:00
Outdated
Review
Copy Link

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?
delvh commented 2021-12-09 21:00:33 +01:00
Outdated
Review
Copy Link

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.
kske commented 2021-12-10 10:48:10 +01:00
Outdated
Review
Copy Link

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.
delvh commented 2021-12-10 17:49:24 +01:00
Outdated
Review
Copy Link

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.
public final class ChangeManager<C extends Change> {
private final List<C> changes = new LinkedList<>();
private int index = -1;
private int markedIndex = -1;
public interface ChangeManager<C extends Change> {
/**
* Applies the given change and appends it to the change list.
kske marked this conversation as resolved Outdated
kske commented 2021-12-07 21:14:39 +01:00
Outdated
Review
Copy Link

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

"Applies the given change and appends it to the change list."
@ -24,11 +20,7 @@ public final class ChangeManager<C extends Change> {
* @param change the change to add
* @since 0.0.1
*/
public void addChange(C change) {
change.apply();
changes.add(change);
++index;
}
void addChange(C change);
/**
* Undoes the current change.
@ -36,14 +28,7 @@ public final class ChangeManager<C extends Change> {
* @return whether an action was performed
* @since 0.1.0
kske marked this conversation as resolved Outdated
kske commented 2021-12-07 21:16:59 +01:00
Outdated
Review
Copy Link

"Undoes the current change."

"Undoes the current change."
*/
public boolean undo() {
if (isUndoAvailable()) {
changes.get(index).invert().apply();
--index;
return true;
}
return false;
}
boolean undo();
kske marked this conversation as resolved Outdated
kske commented 2021-12-07 21:18:20 +01:00
Outdated
Review
Copy Link

"whether an action was performed"

"whether an action was performed"
/**
* Applies the change that was undone before.
@ -51,47 +36,32 @@ public final class ChangeManager<C extends Change> {
* @return whether an action was performed
* @since 0.0.1
*/
public boolean redo() {
if (isRedoAvailable()) {
changes.get(index + 1).apply();
++index;
return true;
}
return false;
}
boolean redo();
kske marked this conversation as resolved Outdated
delvh commented 2021-12-09 12:01:29 +01:00
Outdated
Review
Copy Link

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.
kske commented 2021-12-09 18:34:03 +01:00
Outdated
Review
Copy Link

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.
delvh commented 2021-12-09 20:54:32 +01:00
Outdated
Review
Copy Link

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.
delvh commented 2021-12-09 21:12:28 +01:00
Outdated
Review
Copy Link

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.
kske commented 2021-12-10 10:45:53 +01:00
Outdated
Review
Copy Link

https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/ListIterator.html#add(E)

https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/ListIterator.html#add(E)
👍 1
kske commented 2021-12-11 18:13:11 +01:00
Outdated
Review
Copy Link

@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.
/**
* Marks the current change.
mpk marked this conversation as resolved Outdated
delvh commented 2021-12-09 12:03:23 +01:00
Outdated
Review
Copy Link

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

boolean undoAvailable = ...; if(undoAvailable) { ... } return undoAvailable;
kske commented 2021-12-09 18:35:16 +01:00
Outdated
Review
Copy Link

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.
mpk commented 2021-12-09 20:13:14 +01:00
Outdated
Review
Copy Link

Agreed

Agreed
delvh commented 2021-12-09 21:03:51 +01:00
Outdated
Review
Copy Link

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.
kske commented 2021-12-10 10:57:57 +01:00
Outdated
Review
Copy Link

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.
mpk commented 2021-12-10 16:35:39 +01:00
Outdated
Review
Copy Link

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.
*
* @since 0.0.1
*/
public void mark() {
markedIndex = index;
}
void mark();
kske marked this conversation as resolved Outdated
kske commented 2021-12-07 21:18:55 +01:00
Outdated
Review
Copy Link

"whether an action was performed"

"whether an action was performed"
/**
* @return whether the current change is marked
* @since 0.0.1
*/
public boolean isAtMarkedIndex() {
return markedIndex == index;
}
boolean isAtMarkedIndex();
/**
* @return whether a change is present that can be undone
* @since 0.0.1
*/
mpk marked this conversation as resolved Outdated
delvh commented 2021-12-09 12:03:57 +01:00
Outdated
Review
Copy Link

Same as for undo.

Same as for undo.
mpk commented 2021-12-09 20:13:26 +01:00
Outdated
Review
Copy Link

Same as above

Same as above
public boolean isUndoAvailable() {
return index > -1;
}
boolean isUndoAvailable();
kske marked this conversation as resolved Outdated
kske commented 2021-12-07 21:19:58 +01:00
Outdated
Review
Copy Link

"Marks the current change."

"Marks the current change."
/**
* @return whether a change is present that can be redone
* @since 0.0.1
*/
public boolean isRedoAvailable() {
return index < changes.size() - 1;
}
boolean isRedoAvailable();
/**
* Provides an unmodifiable view of the changes stored in this change manager.
@ -99,7 +69,5 @@ public final class ChangeManager<C extends Change> {
* @return all stored changes
* @since 0.0.1
*/
public List<C> getChanges() {
return Collections.unmodifiableList(changes);
}
List<C> getChanges();
}

69
src/main/java/dev/kske/undoredo/UnlimitedChangeManager.java Normal file
View File

@ -0,0 +1,69 @@
package dev.kske.undoredo;
import java.util.*;
/**
* @param <C> the change type to store in this change manager
* @author Maximilian K&auml;fer
* @author Kai S. K. Engelbart
* @since 0.0.1
*/
public final class UnlimitedChangeManager<C extends Change> implements ChangeManager<C> {
private final List<C> changes = new LinkedList<>();
private int index = -1;
private int markedIndex = -1;
@Override
public void addChange(C change) {
change.apply();
changes.add(change);
++index;
}
@Override
public boolean undo() {
if (isUndoAvailable()) {
changes.get(index).invert().apply();
--index;
return true;
}
return false;
}
@Override
public boolean redo() {
if (isRedoAvailable()) {
changes.get(index + 1).apply();
++index;
return true;
}
return false;
}
@Override
public void mark() {
markedIndex = index;
}
@Override
public boolean isAtMarkedIndex() {
return markedIndex == index;
}
@Override
public boolean isUndoAvailable() {
return index > -1;
}
@Override
public boolean isRedoAvailable() {
return index < changes.size() - 1;
}
@Override
public List<C> getChanges() {
return Collections.unmodifiableList(changes);
}
}

2
src/test/java/dev/kske/undoredo/ChangeManagerTest.java
View File

@ -16,7 +16,7 @@ class ChangeManagerTest {
@BeforeEach
void prepareChangeManager() {
manager = new ChangeManager<>();
manager = new UnlimitedChangeManager<>();
wrapper = new IntWrapper();
change = new IntChange(wrapper, 1);
}