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

JavaFX Integration #4

Merged
kske merged 4 commits from f/javafx into develop 2021-12-16 12:05:50 +01:00
Conversation 0 Commits 4 Files Changed 11 +328 -6
kske commented 2021-12-14 20:48:30 +01:00
Owner
Copy Link

Closes #3

Closes #3
kske added the
enhancement
 label 2021-12-14 20:48:30 +01:00
kske self-assigned this 2021-12-14 20:48:30 +01:00
kske added 3 commits 2021-12-14 20:48:31 +01:00
zdm/undo-redo/pipeline/head There was a failure building this commit Details
d49772a127
Add javafx module 
The undo-redo-javafx module will contain the JavaFX-compatible API for
Undo-Redo.
kske requested review from delvh 2021-12-14 20:48:34 +01:00
kske requested review from mpk 2021-12-14 20:48:34 +01:00
mpk started working 2021-12-15 11:42:42 +01:00
mpk approved these changes 2021-12-15 11:44:02 +01:00
mpk left a comment
Owner

Looks good

Looks good
mpk stopped working 2021-12-15 11:44:04 +01:00
1min 22s
delvh started working 2021-12-15 12:00:14 +01:00
delvh requested changes 2021-12-15 14:09:33 +01:00
core/src/main/java/dev/kske/undoredo/core/UnlimitedChangeManager.java Outdated
@ -24,1 +24,4 @@
@Override
public Optional<C> getLastChange() {
return index == -1 ? Optional.empty() : Optional.of(changes.get(index));
delvh commented 2021-12-15 12:00:38 +01:00
Owner

index < 0

`index < 0`
kske marked this conversation as resolved
core/src/test/java/dev/kske/undoredo/core/ChangeManagerTest.java Outdated
@ -44,0 +53,4 @@
* @since 0.0.1
*/
@Test
@Order(3)
delvh commented 2021-12-15 12:12:57 +01:00
Owner

I see you noticed the problem when adding a new test that has to be executed earlier:
You have to increment all subsequent priorities.
For this reason I propose another mechanism for using @Order: Instead of @Order(n), we should use @Order(10^n) for the tests that are created when creating the class.
Adding newer tests is then as simple as using @Order(10^n / i) where i is the amount of other elements with that order.
That way, when we have the basic tests, we don't have to subsequently change all indices below them when adding newer tests.

Take the following example:
@Order(1), @Order(2), @Order(3), @Order(4), @Order(5) are the tests we created initially.
Now, we add @Order(2b), @Order(3a), @Order(3c) and @Order(6). With our mechanism, that is pretty inefficient.
With my method, we would have initially
@Order(10), @Order(100), @Order(1 000), @Order(10 000), @Order(100 000).
Now, we simply add @Order(150), @Order(500), @Order(1 500) and @Order(1 000 000).
And then we're done.

I see you noticed the problem when adding a new test that has to be executed earlier: You have to increment all subsequent priorities. For this reason I propose another mechanism for using `@Order`: Instead of `@Order(n)`, we should use `@Order(10^n)` for the tests that are created when creating the class. Adding newer tests is then as simple as using `@Order(10^n / i)` where `i` is the amount of other elements with that order. That way, when we have the basic tests, we don't have to subsequently change all indices below them when adding newer tests. Take the following example: `@Order(1)`, `@Order(2)`, `@Order(3)`, `@Order(4)`, `@Order(5)` are the tests we created initially. Now, we add `@Order(2b)`, `@Order(3a)`, `@Order(3c)` and `@Order(6)`. With our mechanism, that is pretty inefficient. With my method, we would have initially `@Order(10)`, `@Order(100)`, `@Order(1 000)`, `@Order(10 000)`, `@Order(100 000)`. Now, we simply add `@Order(150)`, `@Order(500)`, `@Order(1 500)` and `@Order(1 000 000)`. And then we're done.
kske commented 2021-12-15 15:07:55 +01:00
Author
Owner

How would you deal with a class that has more than nine tests?

How would you deal with a class that has more than nine tests?
delvh commented 2021-12-15 15:20:54 +01:00
Owner

Bring out my "division by 2" skills again.
Also, you first need to have 9 tests that depend on order.
In most cases, tests should not depend on a specific order.

Bring out my "division by 2" skills again. Also, you first need to have 9 tests that depend on order. In most cases, tests should not depend on a specific order.
kske commented 2021-12-15 15:27:17 +01:00
Author
Owner

I would prefer having gaps of 10, or maybe 100, but with exponential gaps, the rule is unnecessarily complex and we can easily surpass Integer.MAX_VALUE / 2, which is the default order for JUnit tests, leading to unexpected results for tests without an explicitly defined order.

I would prefer having gaps of 10, or maybe 100, but with exponential gaps, the rule is unnecessarily complex and we can easily surpass `Integer.MAX_VALUE / 2`, which is the default order for JUnit tests, leading to unexpected results for tests without an explicitly defined order.
delvh commented 2021-12-15 17:24:51 +01:00
Owner

That's another possibility.
I don't care.
Should we use the notation @Order(10n) then?

That's another possibility. I don't care. Should we use the notation `@Order(10n)` then?
👍 1
kske marked this conversation as resolved
javafx/pom.xml
@ -0,0 +21,4 @@
<dependency>
<groupId>org.openjfx</groupId>
<artifactId>javafx-base</artifactId>
<version>11</version>
delvh commented 2021-12-14 21:33:28 +01:00
Owner

Shouldn't it be provided?

Otherwise we run into version problems...

Shouldn't it be `provided`? Otherwise we run into version problems...
kske commented 2021-12-15 15:05:27 +01:00
Author
Owner

A project using this library should be able to use any JavaFX version >= 11. A fixed version would be defined like this: [11]. <scope>provided</scope> assumes that the dependency is present in some kind of runtime container, like the JDK or a Java EE container, which isn't the case here.

A project using this library should be able to use any JavaFX version >= 11. A fixed version would be defined like this: `[11]`. `<scope>provided</scope>` assumes that the dependency is present in some kind of runtime container, like the JDK or a Java EE container, which isn't the case here.
delvh commented 2021-12-15 15:19:37 +01:00
Owner

I thought that was exactly the goal behind provided: To declare that any version of JavaFX is possible, as long as it is present...

I thought that was exactly the goal behind `provided`: To declare that any version of JavaFX is possible, as long as it is present...
kske commented 2021-12-16 10:06:17 +01:00
Author
Owner

I don't think the scope has anything to do with the version: https://maven.apache.org/guides/introduction/introduction-to-dependency-mechanism.html#Dependency_Scope

There is a possibility to specify version ranger, but the default should be sufficient for us: https://docs.oracle.com/middleware/1212/core/MAVEN/maven_version.htm#MAVEN402

I don't think the scope has anything to do with the version: https://maven.apache.org/guides/introduction/introduction-to-dependency-mechanism.html#Dependency_Scope There is a possibility to specify version ranger, but the default should be sufficient for us: https://docs.oracle.com/middleware/1212/core/MAVEN/maven_version.htm#MAVEN402
javafx/src/main/java/dev/kske/undoredo/javafx/ChangeManagerWrapper.java
@ -0,0 +8,4 @@
/**
* Wraps an ordinary change manager into an observable change manager, providing the required
* properties for concrete implementations.
delvh commented 2021-12-15 13:49:59 +01:00
Owner 
<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. ```
kske marked this conversation as resolved
javafx/src/main/java/dev/kske/undoredo/javafx/ChangeManagerWrapper.java
@ -0,0 +18,4 @@
public class ChangeManagerWrapper<C extends Change, M extends ChangeManager<C>>
implements ObservableChangeManager<C> {
protected ReadOnlyObjectWrapper<C> lastChange =
delvh commented 2021-12-15 13:15:02 +01:00
Owner

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
👍 1
kske commented 2021-12-16 10:16:34 +01:00
Author
Owner

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.
javafx/src/main/java/dev/kske/undoredo/javafx/ChangeManagerWrapper.java
@ -0,0 +19,4 @@
implements ObservableChangeManager<C> {
protected ReadOnlyObjectWrapper<C> lastChange =
new ReadOnlyObjectWrapper<>(this, "lastChange");
delvh commented 2021-12-15 13:52:42 +01:00
Owner

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.
kske commented 2021-12-15 15:03:53 +01:00
Author
Owner

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?
delvh commented 2021-12-15 15:18:07 +01:00
Owner

Yes.

Yes.
kske commented 2021-12-15 15:27:38 +01:00
Author
Owner

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

But what purpose should this have? When would it be used?
delvh commented 2021-12-15 16:29:04 +01:00
Owner

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

`Bindings.bind(someProperty, ChangeManagerWrapper.UndoAvailable)` or however it is called.
kske marked this conversation as resolved
javafx/src/main/java/dev/kske/undoredo/javafx/ChangeManagerWrapper.java Outdated
@ -0,0 +29,4 @@
protected final M manager;
protected ChangeManagerWrapper(M manager) {
delvh commented 2021-12-15 12:19:47 +01:00
Owner

Wait, protected?
How do you instantiate one then? Do you really have to create your own class? Isn't that a bit overkill?

Wait, protected? How do you instantiate one then? Do you really have to create your own class? Isn't that a bit overkill?
kske commented 2021-12-15 15:03:17 +01:00
Author
Owner

I think the wrapper can be concrete, but should be open for extension in case we want a wrapper for a change manager with additional methods.

I think the wrapper can be concrete, but should be open for extension in case we want a wrapper for a change manager with additional methods.
👍 1
kske marked this conversation as resolved
javafx/src/main/java/dev/kske/undoredo/javafx/ChangeManagerWrapper.java
@ -0,0 +54,4 @@
updateProperties();
return true;
}
return false;
delvh commented 2021-12-15 12:35:32 +01:00
Owner

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.
kske commented 2021-12-15 15:02:22 +01:00
Author
Owner

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.
delvh marked this conversation as resolved
javafx/src/main/java/dev/kske/undoredo/javafx/ObservableChangeManager.java Outdated
@ -0,0 +7,4 @@
import dev.kske.undoredo.core.*;
/**
* @param <C> the change type to store in this change manager
delvh commented 2021-12-15 12:02:02 +01:00
Owner

Purpose of this interface?

Purpose of this interface?
kske marked this conversation as resolved
javafx/src/main/java/module-info.java Outdated
@ -0,0 +8,4 @@
exports dev.kske.undoredo.javafx;
requires dev.kske.undoredo.core;
delvh commented 2021-12-15 12:03:21 +01:00
Owner

requires transitive?

`requires transitive`?
kske commented 2021-12-15 15:00:23 +01:00
Author
Owner

The core module doesn't have any dependencies, so this shouldn't make a difference.

The core module doesn't have any dependencies, so this shouldn't make a difference.
delvh commented 2021-12-15 15:17:25 +01:00
Owner

I think transitive is meant for the other way around:
Not that we require all dependencies from that module,
but that modules requiring our module also automatically require the core module.

I think `transitive` is meant for the other way around: Not that we require all dependencies from that module, but that modules requiring our module also automatically require the `core` module.
kske marked this conversation as resolved
delvh canceled time tracking 2021-12-15 14:11:49 +01:00
delvh added spent time 2021-12-15 14:12:03 +01:00
35min
kske added 1 commit 2021-12-16 10:22:41 +01:00
zdm/undo-redo/pipeline/head This commit looks good Details
0f1c3e06d8
JavaFX reflective access + Javadoc + @delvh 
* Allow reflective access to the JavaFX module for javafx.base
* Add space to the unit test orders
* Improve Javadoc
* Add @delvh as developer
kske requested review from delvh 2021-12-16 10:22:56 +01:00
delvh approved these changes 2021-12-16 12:00:51 +01:00
kske merged commit fa5c2419bf into develop 2021-12-16 12:05:50 +01:00
kske deleted branch f/javafx 2021-12-16 12:05:50 +01:00
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
mpk
delvh
Labels
Clear labels   
1

1

  
13

13

  
2

2

  
21

21

  
3

3

  
34

34

  
5

5

  
55

55

  
8

8

  
bug

Something isn't working

  
could have

This doesn't hurt

  
documentation

Improvements or additions to documentation

  
duplicate

This issue or pull request already exists

  
enhancement

New feature or request

  
help wanted

Extra attention is needed

  
must have

This is absolutely necessary

  
question

Further information is requested

  
should have

This should be implemented

  
stale

This issue or pull request had no recent activity

  
wont have

This won't be implemented (for now)

  
bug

Something is not working

  
bugfix

Fixes a bug

  
discussion

This has to be discussed

  
documentation

Related to documentation

  
feature

Introduces a new feature

  
L

Branches where 300 < X <= 600 lines are added and more than one work day is needed

  
M

Branches where 100 < X <=300 lines are added

  
maintenance

Related to project maintenance

  
postponed

Won't be done in the immediate future

  
refactoring

Refactoring code or architectures

  
S

Branches where X <= 100 lines are added or less than 30 minutes are needed

  
wontfix

Will not be fixed in this state of development

  
XL

Branch where X > 600 lines are added and more than two work days are needed

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
bug
bugfix
discussion
documentation
feature
L
M
maintenance
postponed
refactoring
S
wontfix
XL
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
kske
3 Participants
Total Time Spent: 36 minutes 22 seconds
mpk
1 minute 22 seconds
delvh
35 minutes
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#4
No description provided.
Delete Branch "f/javafx"

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?