Basic API Structure #2

Merged
kske merged 12 commits from f/basics into develop 2021-12-11 21:45:59 +01:00
Owner

Imlpements the basic interfaces and classed needed to ensure the basic standard behaviour.

Imlpements the basic interfaces and classed needed to ensure the basic standard behaviour.
mpk added the
enhancement
label 2021-12-07 19:25:58 +01:00
kske was assigned by mpk 2021-12-07 19:25:58 +01:00
mpk self-assigned this 2021-12-07 19:25:58 +01:00
mpk added 1 commit 2021-12-07 19:25:58 +01:00
mpk requested review from kske 2021-12-07 19:26:02 +01:00
mpk requested review from delvh 2021-12-07 19:26:02 +01:00
mpk changed title from Basic API structure to Basic API Structure 2021-12-07 19:26:15 +01:00
kske requested changes 2021-12-07 21:22:04 +01:00
Dismissed
kske left a comment
Owner

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

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

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."
kske marked this conversation as resolved
@ -0,0 +3,4 @@
import java.util.*;
/**
* @param <C> the change types to store in this change manager

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

Replace `types` by `type`, as the user only specifies one parameter.
kske marked this conversation as resolved
@ -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."

"Applies the given change and appends it to the change list."
kske marked this conversation as resolved
@ -0,0 +26,4 @@
}
/**
* Undoes the change at the current index position.

"Undoes the current change."

"Undoes the current change."
kske marked this conversation as resolved
@ -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"

"whether an action was performed"
kske marked this conversation as resolved
@ -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"

"whether an action was performed"
kske marked this conversation as resolved
@ -0,0 +56,4 @@
}
/**
* Marks the current index.

"Marks the current change."

"Marks the current change."
kske marked this conversation as resolved
@ -0,0 +65,4 @@
}
/**
* @return whether the current index was marked

"whether the current change is marked"

"whether the current change is marked"
kske marked this conversation as resolved
@ -0,0 +73,4 @@
}
/**
* @return whether the undo operation is currently available

"whether a change is present that can be undone"

"whether a change is present that can be undone"
kske marked this conversation as resolved
@ -0,0 +81,4 @@
}
/**
* @return whether the redo operation is currently available.

"whether a change is present that can be redone"

"whether a change is present that can be redone"
kske marked this conversation as resolved
kske added 1 commit 2021-12-08 10:35:19 +01:00
kske dismissed kske’s review 2021-12-08 10:39:32 +01:00
Reason:

I will implement my suggestions myself, as we were pair coding on this branch anyways.

kske added 1 commit 2021-12-08 10:42:58 +01:00
zdm/undo-redo/pipeline/head There was a failure building this commit Details
dc74b78d20
Add Jenkinsfile
delvh started working 2021-12-09 11:50:18 +01:00
delvh requested changes 2021-12-09 12:21:29 +01:00
@ -0,0 +11,4 @@
* @author Maximilian K&auml;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.

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?

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.

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.

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.

Most likely a separate module. Maven modules should help already a lot in this case.
kske marked this conversation as resolved
@ -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 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.

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.

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.

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

@DieGurke and me decided to use the array list for simplicity's sake.
kske marked this conversation as resolved
@ -0,0 +39,4 @@
if (isUndoAvailable()) {
changes.get(index).invert().apply();
--index;
return true;

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

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

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.
Author
Owner

Agreed

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.

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:

  • 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.
Author
Owner

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.
mpk marked this conversation as resolved
@ -0,0 +54,4 @@
if (isRedoAvailable()) {
changes.get(index + 1).apply();
++index;
return true;

Same as for undo.

Same as for undo.
Author
Owner

Same as above

Same as above
mpk marked this conversation as resolved
delvh stopped working 2021-12-09 12:21:33 +01:00
31min 15s
kske added 1 commit 2021-12-10 18:40:00 +01:00
zdm/undo-redo/pipeline/head There was a failure building this commit Details
bb7c1690b2
Add change manager unit test skeleton
kske added 1 commit 2021-12-11 14:00:06 +01:00
zdm/undo-redo/pipeline/head There was a failure building this commit Details
2773d360fb
Implement change manager unit tests
kske added 1 commit 2021-12-11 14:11:19 +01:00
zdm/undo-redo/pipeline/head This commit looks good Details
ee6015b353
Fix index handling in change manager
mpk requested review from delvh 2021-12-11 16:56:36 +01:00
mpk added 1 commit 2021-12-11 17:30:50 +01:00
kske added 1 commit 2021-12-11 17:55:53 +01:00
zdm/undo-redo/pipeline/head There was a failure building this commit Details
c411eace81
Convert to multi-module project
kske added 1 commit 2021-12-11 17:58:12 +01:00
zdm/undo-redo/pipeline/head This commit looks good Details
4ef3c41251
Adapt Jenkinsfile to multi-module architecture
mpk added 1 commit 2021-12-11 18:09:55 +01:00
zdm/undo-redo/pipeline/head This commit looks good Details
93e177cc35
Changed LinkedList to ArrayList
kske started working 2021-12-11 18:13:25 +01:00
kske added 1 commit 2021-12-11 18:18:42 +01:00
zdm/undo-redo/pipeline/head There was a failure building this commit Details
23aa60e65e
Move everything into the core package
kske stopped working 2021-12-11 18:20:40 +01:00
7min 15s
kske added 1 commit 2021-12-11 18:22:09 +01:00
zdm/undo-redo/pipeline/head This commit looks good Details
a4695870c4
Fix core module name
kske approved these changes 2021-12-11 18:23:17 +01:00
delvh started working 2021-12-11 21:32:56 +01:00
Owner

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.

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.
delvh approved these changes 2021-12-11 21:40:34 +01:00
delvh stopped working 2021-12-11 21:40:40 +01:00
7min 44s
kske merged commit 4b07626155 into develop 2021-12-11 21:45:59 +01:00
kske deleted branch f/basics 2021-12-11 21:45:59 +01:00
This repo is archived. You cannot comment on pull requests.
No reviewers
No Milestone
No Assignees
3 Participants
Total Time Spent: 46 minutes 14 seconds
delvh
38 minutes 59 seconds
kske
7 minutes 15 seconds
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: zdm/undo-redo#2
No description provided.