Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v10]

2022-03-18 Thread Michael Strauß
On Thu, 10 Mar 2022 17:49:38 GMT, John Hendrikx  wrote:

>> This is an implementation of the proposal in 
>> https://bugs.openjdk.java.net/browse/JDK-8274771 that me and Nir Lisker 
>> (@nlisker) have been working on.  It's a complete implementation including 
>> good test coverage.  
>> 
>> This was based on https://github.com/openjdk/jfx/pull/434 but with a smaller 
>> API footprint.  Compared to the PoC this is lacking public API for 
>> subscriptions, and does not include `orElseGet` or the `conditionOn` 
>> conditional mapping.
>> 
>> **Flexible Mappings**
>> Map the contents of a property any way you like with `map`, or map nested 
>> properties with `flatMap`.
>> 
>> **Lazy**
>> The bindings created are lazy, which means they are always _invalid_ when 
>> not themselves observed. This allows for easier garbage collection (once the 
>> last observer is removed, a chain of bindings will stop observing their 
>> parents) and less listener management when dealing with nested properties.  
>> Furthermore, this allows inclusion of such bindings in classes such as 
>> `Node` without listeners being created when the binding itself is not used 
>> (this would allow for the inclusion of a `treeShowingProperty` in `Node` 
>> without creating excessive listeners, see this fix I did in an earlier PR: 
>> https://github.com/openjdk/jfx/pull/185)
>> 
>> **Null Safe**
>> The `map` and `flatMap` methods are skipped, similar to `java.util.Optional` 
>> when the value they would be mapping is `null`. This makes mapping nested 
>> properties with `flatMap` trivial as the `null` case does not need to be 
>> taken into account in a chain like this: 
>> `node.sceneProperty().flatMap(Scene::windowProperty).flatMap(Window::showingProperty)`.
>>   Instead a default can be provided with `orElse`.
>> 
>> Some examples:
>> 
>> void mapProperty() {
>>   // Standard JavaFX:
>>   label.textProperty().bind(Bindings.createStringBinding(() -> 
>> text.getValueSafe().toUpperCase(), text));
>> 
>>   // Fluent: much more compact, no need to handle null
>>   label.textProperty().bind(text.map(String::toUpperCase));
>> }
>> 
>> void calculateCharactersLeft() {
>>   // Standard JavaFX:
>>   
>> label.textProperty().bind(text.length().negate().add(100).asString().concat("
>>  characters left"));
>> 
>>   // Fluent: slightly more compact and more clear (no negate needed)
>>   label.textProperty().bind(text.orElse("").map(v -> 100 - v.length() + 
>> " characters left"));
>> }
>> 
>> void mapNestedValue() {
>>   // Standard JavaFX:
>>   label.textProperty().bind(Bindings.createStringBinding(
>> () -> employee.get() == null ? ""
>> : employee.get().getCompany() == null ? ""
>> : employee.get().getCompany().getName(),
>> employee
>>   ));
>> 
>>   // Fluent: no need to handle nulls everywhere
>>   label.textProperty().bind(
>> employee.map(Employee::getCompany)
>> .map(Company::getName)
>> .orElse("")
>>   );
>> }
>> 
>> void mapNestedProperty() {
>>   // Standard JavaFX:
>>   label.textProperty().bind(
>> Bindings.when(Bindings.selectBoolean(label.sceneProperty(), 
>> "window", "showing"))
>>   .then("Visible")
>>   .otherwise("Not Visible")
>>   );
>> 
>>   // Fluent: type safe
>>   label.textProperty().bind(label.sceneProperty()
>> .flatMap(Scene::windowProperty)
>> .flatMap(Window::showingProperty)
>> .orElse(false)
>> .map(showing -> showing ? "Visible" : "Not Visible")
>>   );
>> }
>> 
>> Note that this is based on ideas in ReactFX and my own experiments in 
>> https://github.com/hjohn/hs.jfx.eventstream.  I've come to the conclusion 
>> that this is much better directly integrated into JavaFX, and I'm hoping 
>> this proof of concept will be able to move such an effort forward.
>
> John Hendrikx has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Process review comments (2)

The code looks good. I've left some inline comments.

modules/javafx.base/src/main/java/com/sun/javafx/binding/Subscription.java line 
40:

> 38:  *
> 39:  * For example:
> 40:  * Subscription s = property.subscribe(System.out::println)

I believe our recommended pattern for example code is:

{@code
...
}

modules/javafx.base/src/main/java/com/sun/javafx/binding/Subscription.java line 
67:

> 65:  */
> 66: default Subscription and(Subscription other) {
> 67: Objects.requireNonNull(other);

This exception could be documented with `@throws NullPointerException if {@code 
other} is null`

modules/javafx.base/src/main/java/javafx/beans/value/FlatMappedBinding.java 
line 68:

> 66: };
> 67: }
> 68: }

Several files are missing newlines after the last closing brace. Do we enforce 
this?

Also, if there's a newline after the first line of a class d

[jfx17u] RFR: 8203463: [Accessibility, Narrator] NPE in TableView

2022-03-18 Thread Johan Vos
Reviewed-by: kcr

-

Commit messages:
 - 8203463: [Accessibility, Narrator] NPE in TableView

Changes: https://git.openjdk.java.net/jfx17u/pull/38/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jfx17u&pr=38&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8203463
  Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jfx17u/pull/38.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx17u pull/38/head:pull/38

PR: https://git.openjdk.java.net/jfx17u/pull/38


[jfx17u] RFR: 8197991: Selecting many items in a TableView is very slow

2022-03-18 Thread Johan Vos
Co-authored-by: Naohiro Yoshimoto 
Reviewed-by: kcr, aghaisas

-

Commit messages:
 - 8197991: Selecting many items in a TableView is very slow

Changes: https://git.openjdk.java.net/jfx17u/pull/39/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jfx17u&pr=39&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8197991
  Stats: 403 lines in 5 files changed: 357 ins; 40 del; 6 mod
  Patch: https://git.openjdk.java.net/jfx17u/pull/39.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx17u pull/39/head:pull/39

PR: https://git.openjdk.java.net/jfx17u/pull/39


[jfx11u] Integrated: 8197991: Selecting many items in a TableView is very slow

2022-03-18 Thread Johan Vos
On Thu, 17 Mar 2022 21:01:51 GMT, Johan Vos  wrote:

> Co-authored-by: Naohiro Yoshimoto 
> Reviewed-by: kcr, aghaisas

This pull request has now been integrated.

Changeset: 1c3d9666
Author:Johan Vos 
URL:   
https://git.openjdk.java.net/jfx11u/commit/1c3d9666884b6689d7b982a8541c692468512bf0
Stats: 403 lines in 5 files changed: 357 ins; 40 del; 6 mod

8197991: Selecting many items in a TableView is very slow

Backport-of: 8c4f966b163f68c517574e38c394b9349551ad08

-

PR: https://git.openjdk.java.net/jfx11u/pull/82


[jfx17u] Integrated: 8203463: [Accessibility, Narrator] NPE in TableView

2022-03-18 Thread Johan Vos
On Fri, 18 Mar 2022 07:42:11 GMT, Johan Vos  wrote:

> Reviewed-by: kcr

This pull request has now been integrated.

Changeset: 53d443bf
Author:Johan Vos 
URL:   
https://git.openjdk.java.net/jfx17u/commit/53d443bffa34c971c7c6192649caa9aba2acb5f5
Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod

8203463: [Accessibility, Narrator] NPE in TableView

Backport-of: c705bd493931d88650542be5466d6add359f45b9

-

PR: https://git.openjdk.java.net/jfx17u/pull/38


[jfx17u] Integrated: 8197991: Selecting many items in a TableView is very slow

2022-03-18 Thread Johan Vos
On Fri, 18 Mar 2022 07:42:37 GMT, Johan Vos  wrote:

> Co-authored-by: Naohiro Yoshimoto 
> Reviewed-by: kcr, aghaisas

This pull request has now been integrated.

Changeset: 4af7ee97
Author:Johan Vos 
URL:   
https://git.openjdk.java.net/jfx17u/commit/4af7ee97f37cb18b3433afc3b128b10d3401f29c
Stats: 403 lines in 5 files changed: 357 ins; 40 del; 6 mod

8197991: Selecting many items in a TableView is very slow

Backport-of: 8c4f966b163f68c517574e38c394b9349551ad08

-

PR: https://git.openjdk.java.net/jfx17u/pull/39


[jfx11u] Integrated: 8203463: [Accessibility, Narrator] NPE in TableView

2022-03-18 Thread Johan Vos
On Thu, 17 Mar 2022 20:56:32 GMT, Johan Vos  wrote:

> Reviewed-by: kcr

This pull request has now been integrated.

Changeset: bf32dd8a
Author:Johan Vos 
URL:   
https://git.openjdk.java.net/jfx11u/commit/bf32dd8a52876b0c0a53e9f73d64b0a986829626
Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod

8203463: [Accessibility, Narrator] NPE in TableView

Backport-of: c705bd493931d88650542be5466d6add359f45b9

-

PR: https://git.openjdk.java.net/jfx11u/pull/81


Question about fatal JavaFX crashes

2022-03-18 Thread Daniel Peintner
Hello,

I take the liberty to ask on the email reflector if there are other people
with similar problems.

Since updating my application to JavaFX18 I get random fatal crashes.
Unfortunately it is not predictable but after some time the app crashes
with "EXCEPTION_ACCESS_VIOLATION".

The hs_error_pidX reports the following.


---  T H R E A D  ---

Current thread (0x016be9c9b410):  JavaThread "C2 CompilerThread0"
daemon [_thread_in_native, id=3068,
stack(0x00393e80,0x00393e90)]

Current CompileTask:
C2:10515286 18185   4   javafx.scene.control.TableView$5::onChanged
(1049 bytes)


Am I the only one seeing this?
I am not sure if this relates to changing JavaFX 17 to 18 or whether it
makes it just more likely.

It seems to be related to "javafx.scene.control.TableView$5::onChanged"
since all crashes show this line.

Having said that, I cannot provide a test-case since it happens all of a
sudden and sometimes after hours using the application.

I am open to any feedback or input on how to proceed best.

Thanks!

-- Daniel


Re: Question about fatal JavaFX crashes

2022-03-18 Thread John Hendrikx
I haven't seen this one; the code in TableView in onChanged hasn't had 
any updates 6 years.


I have my doubts this is a JavaFX problem as it seems the Compiler is 
crashing here in a piece of native code.  It's possible this problem is 
only occuring on systems where the compiler decides it needs to compile 
(or recompile) this method, so at a minimum you'd need to make use of 
TableView in a way to hits that code path often enough to trigger 
compilation.


Oracle provides some documentation on how to deal with what looks like a 
compiler bug, please read here:


https://docs.oracle.com/en/java/javase/17/troubleshoot/troubleshoot-system-crashes.html

Further, I think you should report this problem with as much details as 
possible on bugs.openjdk.java.net -- you can look for similar bugs to 
see what kind of detail and settings would be helpful to debug this.


Other things you could try to solve it yourself:

- Run a different Java version
- Try switching to a different GC
- Use different VM options (are you using anything special?)
- Anything else that is not often used, non-standard or experimental, 
try going to a more common setup


--John

On 18/03/2022 09:43, Daniel Peintner wrote:

Hello,

I take the liberty to ask on the email reflector if there are other people
with similar problems.

Since updating my application to JavaFX18 I get random fatal crashes.
Unfortunately it is not predictable but after some time the app crashes
with "EXCEPTION_ACCESS_VIOLATION".

The hs_error_pidX reports the following.


---  T H R E A D  ---

Current thread (0x016be9c9b410):  JavaThread "C2 CompilerThread0"
daemon [_thread_in_native, id=3068,
stack(0x00393e80,0x00393e90)]

Current CompileTask:
C2:10515286 18185   4   javafx.scene.control.TableView$5::onChanged
(1049 bytes)


Am I the only one seeing this?
I am not sure if this relates to changing JavaFX 17 to 18 or whether it
makes it just more likely.

It seems to be related to "javafx.scene.control.TableView$5::onChanged"
since all crashes show this line.

Having said that, I cannot provide a test-case since it happens all of a
sudden and sometimes after hours using the application.

I am open to any feedback or input on how to proceed best.

Thanks!

-- Daniel


Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v10]

2022-03-18 Thread John Hendrikx
On Thu, 17 Mar 2022 20:09:23 GMT, Michael Strauß  wrote:

>> John Hendrikx has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Process review comments (2)
>
> modules/javafx.base/src/main/java/com/sun/javafx/binding/Subscription.java 
> line 40:
> 
>> 38:  *
>> 39:  * For example:
>> 40:  * Subscription s = property.subscribe(System.out::println)
> 
> I believe our recommended pattern for example code is:
> 
> {@code
> ...
> }

Fixed this everywhere.

> modules/javafx.base/src/main/java/com/sun/javafx/binding/Subscription.java 
> line 67:
> 
>> 65:  */
>> 66: default Subscription and(Subscription other) {
>> 67: Objects.requireNonNull(other);
> 
> This exception could be documented with `@throws NullPointerException if 
> {@code other} is null`

I've updated the docs a bit -- it hasn't received much attention because this 
is not going to be API for now

> modules/javafx.base/src/main/java/javafx/beans/value/FlatMappedBinding.java 
> line 68:
> 
>> 66: };
>> 67: }
>> 68: }
> 
> Several files are missing newlines after the last closing brace. Do we 
> enforce this?
> 
> Also, if there's a newline after the first line of a class declaration, 
> shouldn't there also be a newline before the last closing brace?

Let me add those new lines at the end of files (everywhere) as Github is also 
flagging it with an ugly red marker.  I tend to unconsciously remove them 
myself on longer files as it looks weird in editors to have an unused line at 
the bottom.

As for the newline before the last closing brace, that doesn't seem to be done 
a lot in the current code base.  I've added those newlines at the top as it 
seems fairly consistent in the code base, although I'm not a fan as I use empty 
lines only to separate things when there is no clear separation already (like 
an opening brace).

> modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java 
> line 146:
> 
>> 144:  * Creates an {@code ObservableValue} that holds the result of 
>> applying a
>> 145:  * mapping on this {@code ObservableValue}'s value. The result is 
>> updated
>> 146:  * when this {@code ObservableValue}'s value changes. If this value 
>> is
> 
> I think a lot of the new documentation in this class sacrifices 
> understandability for precision in trying to deal with the difference between 
> "this ObservableValue" and "this ObservableValue's value".
> 
> However, my feeling is that that's not helping users who are trying to 
> understand the purpose of the new APIs.
> What do you think about a simplified version like this:
> `Creates a new {@ObservableValue} that applies a mapping function to this 
> {@code ObservableValue}. The result is updated when this {@code 
> ObservableValue} changes.`
> 
> Sure, it's not literally mapping _this ObservableValue instance_, but would 
> this language really confuse readers more that the precise language?
> 
> Another option might be to combine both:
> `Creates a new {@ObservableValue} that applies a mapping function to this 
> {@code ObservableValue}. More precisely, it creates a new {@code 
> ObservableValue} that holds the result of applying a mapping function to the 
> value of this {@code ObservableValue}.`

Yeah, agreed, it is a bit annoying to have to deal with the fact that these 
classes are wrappers around an actual value and having to refer to them as such 
to be "precise".  I'm willing to make another pass at all of these to change 
the wording.  What do you think @nlisker  ?

> modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java 
> line 153:
> 
>> 151:  * 
>> 152:  * var text = new SimpleStringProperty("abcd");
>> 153:  * ObservableValue upperCase = 
>> text.map(String::toUpperCase);
> 
> Escaping `<` and `>` is not necessary if the code block is wrapped in 
> `{@code}`

Also fixed everywhere

> modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java 
> line 197:
> 
>> 195: /**
>> 196:  * Creates an {@code ObservableValue} that holds the value of an 
>> {@code ObservableValue}
>> 197:  * resulting from applying a mapping on this {@code 
>> ObservableValue}'s value. The result
> 
> While technically correct, I think the first sentence should focus more on 
> the purpose of this method.
> 
> How about something like this:
> `Creates a new {@code ObservableValue} that holds the value of a nested 
> {@code ObservableValue} by applying a mapping function to extract the nested 
> {@code ObservableValue}.`
> 
> That's not as precise, but it makes the purpose much more clear.

I've changed this to use your wording as I think it does read much better.

Perhaps also possible:

  Creates a new {@code ObservableValue} that holds the value of a nested 
{@code ObservableValue} supplied
  by the given mapping function.

?

> modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java 
> line 227:
> 
>> 

Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v11]

2022-03-18 Thread John Hendrikx
> This is an implementation of the proposal in 
> https://bugs.openjdk.java.net/browse/JDK-8274771 that me and Nir Lisker 
> (@nlisker) have been working on.  It's a complete implementation including 
> good test coverage.  
> 
> This was based on https://github.com/openjdk/jfx/pull/434 but with a smaller 
> API footprint.  Compared to the PoC this is lacking public API for 
> subscriptions, and does not include `orElseGet` or the `conditionOn` 
> conditional mapping.
> 
> **Flexible Mappings**
> Map the contents of a property any way you like with `map`, or map nested 
> properties with `flatMap`.
> 
> **Lazy**
> The bindings created are lazy, which means they are always _invalid_ when not 
> themselves observed. This allows for easier garbage collection (once the last 
> observer is removed, a chain of bindings will stop observing their parents) 
> and less listener management when dealing with nested properties.  
> Furthermore, this allows inclusion of such bindings in classes such as `Node` 
> without listeners being created when the binding itself is not used (this 
> would allow for the inclusion of a `treeShowingProperty` in `Node` without 
> creating excessive listeners, see this fix I did in an earlier PR: 
> https://github.com/openjdk/jfx/pull/185)
> 
> **Null Safe**
> The `map` and `flatMap` methods are skipped, similar to `java.util.Optional` 
> when the value they would be mapping is `null`. This makes mapping nested 
> properties with `flatMap` trivial as the `null` case does not need to be 
> taken into account in a chain like this: 
> `node.sceneProperty().flatMap(Scene::windowProperty).flatMap(Window::showingProperty)`.
>   Instead a default can be provided with `orElse`.
> 
> Some examples:
> 
> void mapProperty() {
>   // Standard JavaFX:
>   label.textProperty().bind(Bindings.createStringBinding(() -> 
> text.getValueSafe().toUpperCase(), text));
> 
>   // Fluent: much more compact, no need to handle null
>   label.textProperty().bind(text.map(String::toUpperCase));
> }
> 
> void calculateCharactersLeft() {
>   // Standard JavaFX:
>   
> label.textProperty().bind(text.length().negate().add(100).asString().concat(" 
> characters left"));
> 
>   // Fluent: slightly more compact and more clear (no negate needed)
>   label.textProperty().bind(text.orElse("").map(v -> 100 - v.length() + " 
> characters left"));
> }
> 
> void mapNestedValue() {
>   // Standard JavaFX:
>   label.textProperty().bind(Bindings.createStringBinding(
> () -> employee.get() == null ? ""
> : employee.get().getCompany() == null ? ""
> : employee.get().getCompany().getName(),
> employee
>   ));
> 
>   // Fluent: no need to handle nulls everywhere
>   label.textProperty().bind(
> employee.map(Employee::getCompany)
> .map(Company::getName)
> .orElse("")
>   );
> }
> 
> void mapNestedProperty() {
>   // Standard JavaFX:
>   label.textProperty().bind(
> Bindings.when(Bindings.selectBoolean(label.sceneProperty(), "window", 
> "showing"))
>   .then("Visible")
>   .otherwise("Not Visible")
>   );
> 
>   // Fluent: type safe
>   label.textProperty().bind(label.sceneProperty()
> .flatMap(Scene::windowProperty)
> .flatMap(Window::showingProperty)
> .orElse(false)
> .map(showing -> showing ? "Visible" : "Not Visible")
>   );
> }
> 
> Note that this is based on ideas in ReactFX and my own experiments in 
> https://github.com/hjohn/hs.jfx.eventstream.  I've come to the conclusion 
> that this is much better directly integrated into JavaFX, and I'm hoping this 
> proof of concept will be able to move such an effort forward.

John Hendrikx has updated the pull request incrementally with five additional 
commits since the last revision:

 - Reword flat map docs a bit and fixed a link
 - Add missing javadoc tags
 - Clean up docs in Subscription
 - Fix code blocks
 - Add line feed at last line where it was missing

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/675/files
  - new: https://git.openjdk.java.net/jfx/pull/675/files/8f9bf897..6a5358d7

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=675&range=10
 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=675&range=09-10

  Stats: 39 lines in 7 files changed: 11 ins; 0 del; 28 mod
  Patch: https://git.openjdk.java.net/jfx/pull/675.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/675/head:pull/675

PR: https://git.openjdk.java.net/jfx/pull/675


Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v11]

2022-03-18 Thread Tom Schindl
On Fri, 18 Mar 2022 10:32:36 GMT, John Hendrikx  wrote:

>> This is an implementation of the proposal in 
>> https://bugs.openjdk.java.net/browse/JDK-8274771 that me and Nir Lisker 
>> (@nlisker) have been working on.  It's a complete implementation including 
>> good test coverage.  
>> 
>> This was based on https://github.com/openjdk/jfx/pull/434 but with a smaller 
>> API footprint.  Compared to the PoC this is lacking public API for 
>> subscriptions, and does not include `orElseGet` or the `conditionOn` 
>> conditional mapping.
>> 
>> **Flexible Mappings**
>> Map the contents of a property any way you like with `map`, or map nested 
>> properties with `flatMap`.
>> 
>> **Lazy**
>> The bindings created are lazy, which means they are always _invalid_ when 
>> not themselves observed. This allows for easier garbage collection (once the 
>> last observer is removed, a chain of bindings will stop observing their 
>> parents) and less listener management when dealing with nested properties.  
>> Furthermore, this allows inclusion of such bindings in classes such as 
>> `Node` without listeners being created when the binding itself is not used 
>> (this would allow for the inclusion of a `treeShowingProperty` in `Node` 
>> without creating excessive listeners, see this fix I did in an earlier PR: 
>> https://github.com/openjdk/jfx/pull/185)
>> 
>> **Null Safe**
>> The `map` and `flatMap` methods are skipped, similar to `java.util.Optional` 
>> when the value they would be mapping is `null`. This makes mapping nested 
>> properties with `flatMap` trivial as the `null` case does not need to be 
>> taken into account in a chain like this: 
>> `node.sceneProperty().flatMap(Scene::windowProperty).flatMap(Window::showingProperty)`.
>>   Instead a default can be provided with `orElse`.
>> 
>> Some examples:
>> 
>> void mapProperty() {
>>   // Standard JavaFX:
>>   label.textProperty().bind(Bindings.createStringBinding(() -> 
>> text.getValueSafe().toUpperCase(), text));
>> 
>>   // Fluent: much more compact, no need to handle null
>>   label.textProperty().bind(text.map(String::toUpperCase));
>> }
>> 
>> void calculateCharactersLeft() {
>>   // Standard JavaFX:
>>   
>> label.textProperty().bind(text.length().negate().add(100).asString().concat("
>>  characters left"));
>> 
>>   // Fluent: slightly more compact and more clear (no negate needed)
>>   label.textProperty().bind(text.orElse("").map(v -> 100 - v.length() + 
>> " characters left"));
>> }
>> 
>> void mapNestedValue() {
>>   // Standard JavaFX:
>>   label.textProperty().bind(Bindings.createStringBinding(
>> () -> employee.get() == null ? ""
>> : employee.get().getCompany() == null ? ""
>> : employee.get().getCompany().getName(),
>> employee
>>   ));
>> 
>>   // Fluent: no need to handle nulls everywhere
>>   label.textProperty().bind(
>> employee.map(Employee::getCompany)
>> .map(Company::getName)
>> .orElse("")
>>   );
>> }
>> 
>> void mapNestedProperty() {
>>   // Standard JavaFX:
>>   label.textProperty().bind(
>> Bindings.when(Bindings.selectBoolean(label.sceneProperty(), 
>> "window", "showing"))
>>   .then("Visible")
>>   .otherwise("Not Visible")
>>   );
>> 
>>   // Fluent: type safe
>>   label.textProperty().bind(label.sceneProperty()
>> .flatMap(Scene::windowProperty)
>> .flatMap(Window::showingProperty)
>> .orElse(false)
>> .map(showing -> showing ? "Visible" : "Not Visible")
>>   );
>> }
>> 
>> Note that this is based on ideas in ReactFX and my own experiments in 
>> https://github.com/hjohn/hs.jfx.eventstream.  I've come to the conclusion 
>> that this is much better directly integrated into JavaFX, and I'm hoping 
>> this proof of concept will be able to move such an effort forward.
>
> John Hendrikx has updated the pull request incrementally with five additional 
> commits since the last revision:
> 
>  - Reword flat map docs a bit and fixed a link
>  - Add missing javadoc tags
>  - Clean up docs in Subscription
>  - Fix code blocks
>  - Add line feed at last line where it was missing

modules/javafx.base/src/main/java/com/sun/javafx/binding/Subscription.java line 
57:

> 55:  * Cancels this subscription.
> 56:  */
> 57: void unsubscribe();

to me `unsubscribe` feels not natural, I'm not unsubscribing a `Subscription` 
but I'm eg disposing it, hence to me `dispose` would feel better but I'm not a 
native speaker.

Another pattern I see often is that I store a subscription in a field and 
recreate it under certain conditions:


private Subscription mySubscription;

void refresh() {
   if( this.mySubscription != null ) {
 this.mySubscription.dispos();
   }
   this.mySubscription = 
}


if Subscription would provide

```java 
static void dispose(Subscription subscription) {
  if( su

Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v10]

2022-03-18 Thread Nir Lisker
On Fri, 18 Mar 2022 09:48:39 GMT, John Hendrikx  wrote:

>> modules/javafx.base/src/main/java/com/sun/javafx/binding/Subscription.java 
>> line 67:
>> 
>>> 65:  */
>>> 66: default Subscription and(Subscription other) {
>>> 67: Objects.requireNonNull(other);
>> 
>> This exception could be documented with `@throws NullPointerException if 
>> {@code other} is null`
>
> I've updated the docs a bit -- it hasn't received much attention because this 
> is not going to be API for now

Yes, in "phase 2" when this class is made public there will be a proper docs 
review.

-

PR: https://git.openjdk.java.net/jfx/pull/675


Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v10]

2022-03-18 Thread Nir Lisker
On Fri, 18 Mar 2022 09:32:18 GMT, John Hendrikx  wrote:

>> modules/javafx.base/src/main/java/javafx/beans/value/FlatMappedBinding.java 
>> line 68:
>> 
>>> 66: };
>>> 67: }
>>> 68: }
>> 
>> Several files are missing newlines after the last closing brace. Do we 
>> enforce this?
>> 
>> Also, if there's a newline after the first line of a class declaration, 
>> shouldn't there also be a newline before the last closing brace?
>
> Let me add those new lines at the end of files (everywhere) as Github is also 
> flagging it with an ugly red marker.  I tend to unconsciously remove them 
> myself on longer files as it looks weird in editors to have an unused line at 
> the bottom.
> 
> As for the newline before the last closing brace, that doesn't seem to be 
> done a lot in the current code base.  I've added those newlines at the top as 
> it seems fairly consistent in the code base, although I'm not a fan as I use 
> empty lines only to separate things when there is no clear separation already 
> (like an opening brace).

I don't think jcheck checks for newlines anywhere. Usually the style that I see 
is a newline after the definition of the class and at the end of the file 
(sometimes), but not before the last closing brace.

-

PR: https://git.openjdk.java.net/jfx/pull/675


Re: Question about fatal JavaFX crashes

2022-03-18 Thread Daniel Peintner
Hi John, all,

Thanks for your detailed reply.
I submitted a bug report with detailed information.

- Run a different Java version
>

I tried different versions and vendors with the same result.
* OpenJDK 17.0.1
* Zulu 17.0.2
* I wanted to check also JDK18-ea but gradle does not yet support it


> - Try switching to a different GC
> - Use different VM options (are you using anything special?)
> - Anything else that is not often used, non-standard or experimental,
> try going to a more common setup
>

I use no specific setting, all is default.
I have been reading that the -client flag might help in some cases but
unfortunately this flag is no longer taken into account for 64bit JVMS.

Thanks,

-- Daniel




>
> --John
>
> On 18/03/2022 09:43, Daniel Peintner wrote:
> > Hello,
> >
> > I take the liberty to ask on the email reflector if there are other
> people
> > with similar problems.
> >
> > Since updating my application to JavaFX18 I get random fatal crashes.
> > Unfortunately it is not predictable but after some time the app crashes
> > with "EXCEPTION_ACCESS_VIOLATION".
> >
> > The hs_error_pidX reports the following.
> >
> >
> > ---  T H R E A D  ---
> >
> > Current thread (0x016be9c9b410):  JavaThread "C2 CompilerThread0"
> > daemon [_thread_in_native, id=3068,
> > stack(0x00393e80,0x00393e90)]
> >
> > Current CompileTask:
> > C2:10515286 18185   4
>  javafx.scene.control.TableView$5::onChanged
> > (1049 bytes)
> >
> >
> > Am I the only one seeing this?
> > I am not sure if this relates to changing JavaFX 17 to 18 or whether it
> > makes it just more likely.
> >
> > It seems to be related to "javafx.scene.control.TableView$5::onChanged"
> > since all crashes show this line.
> >
> > Having said that, I cannot provide a test-case since it happens all of a
> > sudden and sometimes after hours using the application.
> >
> > I am open to any feedback or input on how to proceed best.
> >
> > Thanks!
> >
> > -- Daniel
>


Re: Question about fatal JavaFX crashes

2022-03-18 Thread Philip Race
I think it is probable that this is a hotspot VM problem in the C2 JIT 
compiler code.


I've moved your bug report to hotspot : 
https://bugs.openjdk.java.net/browse/JDK-8283386

The interesting question isn't about which version of FX used to work.
It is what was the last working version of the JDK.
It looks a bit like a JDK 17 bug from the evidence so far so if
you were running FX 17 on JDK 17 GA you maybe have picked up a later 
update release  of JDK 17 along with FX 18 ??


Regardless this doesn't look like an FX bug.
But unless you can actually provide a test case, or, by luck the hotspot 
folks recognise the issue,

I don't know what can/will happen.

-phil.

On 3/18/22 7:08 AM, Daniel Peintner wrote:

Hi John, all,

Thanks for your detailed reply.
I submitted a bug report with detailed information.

- Run a different Java version
I tried different versions and vendors with the same result.
* OpenJDK 17.0.1
* Zulu 17.0.2
* I wanted to check also JDK18-ea but gradle does not yet support it



- Try switching to a different GC
- Use different VM options (are you using anything special?)
- Anything else that is not often used, non-standard or experimental,
try going to a more common setup


I use no specific setting, all is default.
I have been reading that the -client flag might help in some cases but
unfortunately this flag is no longer taken into account for 64bit JVMS.

Thanks,

-- Daniel





--John

On 18/03/2022 09:43, Daniel Peintner wrote:

Hello,

I take the liberty to ask on the email reflector if there are other

people

with similar problems.

Since updating my application to JavaFX18 I get random fatal crashes.
Unfortunately it is not predictable but after some time the app crashes
with "EXCEPTION_ACCESS_VIOLATION".

The hs_error_pidX reports the following.


---  T H R E A D  ---

Current thread (0x016be9c9b410):  JavaThread "C2 CompilerThread0"
daemon [_thread_in_native, id=3068,
stack(0x00393e80,0x00393e90)]

Current CompileTask:
C2:10515286 18185   4

  javafx.scene.control.TableView$5::onChanged

(1049 bytes)


Am I the only one seeing this?
I am not sure if this relates to changing JavaFX 17 to 18 or whether it
makes it just more likely.

It seems to be related to "javafx.scene.control.TableView$5::onChanged"
since all crashes show this line.

Having said that, I cannot provide a test-case since it happens all of a
sudden and sometimes after hours using the application.

I am open to any feedback or input on how to proceed best.

Thanks!

-- Daniel




Re: Question about fatal JavaFX crashes

2022-03-18 Thread Kevin Rushforth
Thanks, Phil. That was my take as well. I don't see how this can be a 
JavaFX bug given where it is crashing. FWIW, I haven't ever seen 
anything like this.


-- Kevin


On 3/18/2022 10:25 AM, Philip Race wrote:
I think it is probable that this is a hotspot VM problem in the C2 JIT 
compiler code.


I've moved your bug report to hotspot : 
https://bugs.openjdk.java.net/browse/JDK-8283386

The interesting question isn't about which version of FX used to work.
It is what was the last working version of the JDK.
It looks a bit like a JDK 17 bug from the evidence so far so if
you were running FX 17 on JDK 17 GA you maybe have picked up a later 
update release  of JDK 17 along with FX 18 ??


Regardless this doesn't look like an FX bug.
But unless you can actually provide a test case, or, by luck the 
hotspot folks recognise the issue,

I don't know what can/will happen.

-phil.

On 3/18/22 7:08 AM, Daniel Peintner wrote:

Hi John, all,

Thanks for your detailed reply.
I submitted a bug report with detailed information.

- Run a different Java version
I tried different versions and vendors with the same result.
* OpenJDK 17.0.1
* Zulu 17.0.2
* I wanted to check also JDK18-ea but gradle does not yet support it



- Try switching to a different GC
- Use different VM options (are you using anything special?)
- Anything else that is not often used, non-standard or experimental,
try going to a more common setup


I use no specific setting, all is default.
I have been reading that the -client flag might help in some cases but
unfortunately this flag is no longer taken into account for 64bit JVMS.

Thanks,

-- Daniel





--John

On 18/03/2022 09:43, Daniel Peintner wrote:

Hello,

I take the liberty to ask on the email reflector if there are other

people

with similar problems.

Since updating my application to JavaFX18 I get random fatal crashes.
Unfortunately it is not predictable but after some time the app 
crashes

with "EXCEPTION_ACCESS_VIOLATION".

The hs_error_pidX reports the following.


---  T H R E A D  ---

Current thread (0x016be9c9b410):  JavaThread "C2 CompilerThread0"
daemon [_thread_in_native, id=3068,
stack(0x00393e80,0x00393e90)]

Current CompileTask:
C2:10515286 18185   4

  javafx.scene.control.TableView$5::onChanged

(1049 bytes)


Am I the only one seeing this?
I am not sure if this relates to changing JavaFX 17 to 18 or 
whether it

makes it just more likely.

It seems to be related to 
"javafx.scene.control.TableView$5::onChanged"

since all crashes show this line.

Having said that, I cannot provide a test-case since it happens all 
of a

sudden and sometimes after hours using the application.

I am open to any feedback or input on how to proceed best.

Thanks!

-- Daniel






Re: Question about fatal JavaFX crashes

2022-03-18 Thread Philip Race
I have at least seen  JIT compiler crashes like this in other unrelated 
cases ..


in theory you can use an option like

-XX:CompileCommand=exclude,javafx/scene/control/TableView$5::onChanged

although I am very unsure about the syntax for the last part of it 
especially with what looks like
some anonymous method or lambda expression. I can almost guarantee that 
my example is wrong.


Excluding the whole of TableView might be easier to specify but will 
slow you down a lot.


-phil.


On 3/18/22 10:37 AM, Kevin Rushforth wrote:
Thanks, Phil. That was my take as well. I don't see how this can be a 
JavaFX bug given where it is crashing. FWIW, I haven't ever seen 
anything like this.


-- Kevin


On 3/18/2022 10:25 AM, Philip Race wrote:
I think it is probable that this is a hotspot VM problem in the C2 
JIT compiler code.


I've moved your bug report to hotspot : 
https://bugs.openjdk.java.net/browse/JDK-8283386

The interesting question isn't about which version of FX used to work.
It is what was the last working version of the JDK.
It looks a bit like a JDK 17 bug from the evidence so far so if
you were running FX 17 on JDK 17 GA you maybe have picked up a later 
update release  of JDK 17 along with FX 18 ??


Regardless this doesn't look like an FX bug.
But unless you can actually provide a test case, or, by luck the 
hotspot folks recognise the issue,

I don't know what can/will happen.

-phil.

On 3/18/22 7:08 AM, Daniel Peintner wrote:

Hi John, all,

Thanks for your detailed reply.
I submitted a bug report with detailed information.

- Run a different Java version
I tried different versions and vendors with the same result.
* OpenJDK 17.0.1
* Zulu 17.0.2
* I wanted to check also JDK18-ea but gradle does not yet support it



- Try switching to a different GC
- Use different VM options (are you using anything special?)
- Anything else that is not often used, non-standard or experimental,
try going to a more common setup


I use no specific setting, all is default.
I have been reading that the -client flag might help in some cases but
unfortunately this flag is no longer taken into account for 64bit JVMS.

Thanks,

-- Daniel





--John

On 18/03/2022 09:43, Daniel Peintner wrote:

Hello,

I take the liberty to ask on the email reflector if there are other

people

with similar problems.

Since updating my application to JavaFX18 I get random fatal crashes.
Unfortunately it is not predictable but after some time the app 
crashes

with "EXCEPTION_ACCESS_VIOLATION".

The hs_error_pidX reports the following.


---  T H R E A D  ---

Current thread (0x016be9c9b410):  JavaThread "C2 CompilerThread0"
daemon [_thread_in_native, id=3068,
stack(0x00393e80,0x00393e90)]

Current CompileTask:
C2:10515286 18185   4

  javafx.scene.control.TableView$5::onChanged

(1049 bytes)


Am I the only one seeing this?
I am not sure if this relates to changing JavaFX 17 to 18 or 
whether it

makes it just more likely.

It seems to be related to 
"javafx.scene.control.TableView$5::onChanged"

since all crashes show this line.

Having said that, I cannot provide a test-case since it happens 
all of a

sudden and sometimes after hours using the application.

I am open to any feedback or input on how to proceed best.

Thanks!

-- Daniel








Aw: Re: ArrayIndexOutOfBoundsException when disconnecting screen

2022-03-18 Thread Marius Hanl
   Sorry for the delay.
   I tested a bit around, unfortunately this bug doesn't happen all the
   time.
   It looks like it can also happen when disconnecting just one screen.

   I have filed a ticket: https://bugs.openjdk.java.net/browse/JDK-8283401
   Also one time the JVM crashed, leaving a hs_err_pid file, which I
   attached as well.

   -- Marius

   Gesendet: Mittwoch, 23. Februar 2022 um 23:54 Uhr
   Von: "Kevin Rushforth" 
   An: "Marius Hanl" , "openjfx-dev"
   
   Betreff: Re: ArrayIndexOutOfBoundsException when disconnecting screen
   I spent a fair bit of time simulating and testing the
   D3DERR_DEVICEREMOVED cases for RDP disconnect to fix JDK-8239589 [1],
   but detaching a screen, which will change the number of active
   adapters,
   is a somewhat different case (I'm not sure how easy it would be to
   simulate it). It's very likely that it isn't handled in a robust
   manner.
   Do you know whether you need to disconnect 2 screens to reproduce this,
   or would disconnecting a single external monitor cause it?
   Based on the stack trace, it looks like the D3DResourceFactory array
   has
   been recreated with the reduced number of adapters (1), but the old
   screen is still being used by the instance of PPSRenderer.
   Can you file a bug with the stack trace, and as much information about
   how to reproduce it as you can.
   -- Kevin
   [1] [1]https://bugs.openjdk.java.net/browse/JDK-8239589
   On 2/23/2022 3:35 AM, Marius Hanl wrote:
   > I get an ArrayIndexOutOfBoundsException sometimes when I disconnect
   my
   > screen(s).
   > Setup: I have a laptop with a docking station attached. The docking
   > station is connected to two screen, so when I disconnect it 2 screen
   > will be 'lost'.
   >
   > The following stacktrace is visible and the application is completely
   > black and unresponsive:
   > java.lang.ArrayIndexOutOfBoundsException: Index 1 out of bounds for
   > length 1
   > at
   >
   com.sun.prism.d3d.D3DPipeline.getD3DResourceFactory(D3DPipeline.java:21
   > 7)
   > at
   >
   com.sun.prism.d3d.D3DPipeline.getResourceFactory(D3DPipeline.java:284)
   > at
   >
   com.sun.scenario.effect.impl.prism.ps.PPSRenderer.validate(PPSRenderer.
   > java:101)
   > at
   >
   com.sun.scenario.effect.impl.prism.ps.PPSRenderer.getCompatibleImage(PP
   > SRenderer.java:221)
   > at
   >
   com.sun.scenario.effect.impl.prism.ps.PPSRenderer.getCompatibleImage(PP
   > SRenderer.java:67)
   > at
   > com.sun.scenario.effect.Effect.getCompatibleImage(Effect.java:479)
   > at
   >
   com.sun.javafx.sg.prism.NodeEffectInput.getImageDataForBoundedNode(Node
   > EffectInput.java:228)
   > at
   >
   com.sun.javafx.sg.prism.NodeEffectInput.filter(NodeEffectInput.java:131
   > )
   > at
   > com.sun.scenario.effect.FilterEffect.filter(FilterEffect.java:185)
   > at com.sun.scenario.effect.Offset.filter(Offset.java:160)
   > at com.sun.scenario.effect.Merge.filter(Merge.java:148)
   > at
   > com.sun.scenario.effect.DelegateEffect.filter(DelegateEffect.java:70)
   > at
   >
   com.sun.scenario.effect.impl.prism.PrEffectHelper.render(PrEffectHelper
   > .java:166)
   > at
   > com.sun.javafx.sg.prism.EffectFilter.render(EffectFilter.java:61)
   > at com.sun.javafx.sg.prism.NGNode.renderEffect(NGNode.java:2384)
   > at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2069)
   > at com.sun.javafx.sg.prism.NGNode.render(NGNode.java:1964)
   > at com.sun.javafx.sg.prism.NGGroup.renderContent(NGGroup.java:270)
   > at
   > com.sun.javafx.sg.prism.NGRegion.renderContent(NGRegion.java:579)
   > at com.sun.javafx.sg.prism.NGNode.doRender(NGNode.java:2072)
   > ... ( a lot of doRender(..), renderContent(..) calls... )
   > at
   > com.sun.javafx.tk.quantum.ViewPainter.doPaint(ViewPainter.java:480)
   > at
   > com.sun.javafx.tk.quantum.ViewPainter.paintImpl(ViewPainter.java:329)
   > at
   >
   com.sun.javafx.tk.quantum.PresentingPainter.run(PresentingPainter.java:
   > 92)
   > at
   >
   java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors
   > .java:515)
   > at
   >
   java.base/java.util.concurrent.FutureTask.runAndReset$$$capture(FutureT
   > ask.java:305)
   > at
   >
   java.base/java.util.concurrent.FutureTask.runAndReset(FutureTask.java)
   > at com.sun.javafx.tk.RenderJob.run(RenderJob.java:58)
   > at
   >
   java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolE
   > xecutor.java:1128)
   > at
   >
   java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPool
   > Executor.java:628)
   > at
   >
   com.sun.javafx.tk.quantum.QuantumRenderer$PipelineRunnable.run(QuantumR
   > enderer.java:126)
   > at java.base/java.lang.Thread.run(Thread.java:829)
   >
   >
   ---
   > ---
   >
   > Now my question:
   > A naive fix would be an array check here but does someone has more
   > information about the code here?
   > And more 

Re: RFR: 8283346: Optimize observable ArrayList creation in FXCollections

2022-03-18 Thread yosbits
On Thu, 17 Mar 2022 21:10:14 GMT, Marius Hanl  wrote:

> This simple PR optimizes the observable `ArrayList` creation by using the 
> ArrayList constructor/array size so that the underlying array will be 
> initialized at the correct size which will speed up the creation as the array 
> does not need to grow as a result of the `addAll` call.
> 
> I also added tests which will succeed before and after to verify that nothing 
> got broken by this change.
> Also I made a benchmark test. Results:
> 
> | Benchmark | Mode| Cnt | Score | Error | Units
> | - | - | - | - | 
> - | - |
> | ListBenchmark OLD  | thrpt | 25 | 722,842 | ± 26,93 | ops/s
> | ListBenchmark NEW | thrpt  | 25 | 29262,274 | ± 2088,712 | ops/s
> 
> Benchmark code
> 
> 
> import javafx.collections.FXCollections;
> import javafx.collections.ObservableList;
> import org.openjdk.jmh.annotations.Benchmark;
> import org.openjdk.jmh.annotations.Scope;
> import org.openjdk.jmh.annotations.Setup;
> import org.openjdk.jmh.annotations.State;
> import org.openjdk.jmh.annotations.TearDown;
> 
> import java.util.ArrayList;
> import java.util.List;
> 
> @State(Scope.Benchmark)
> public class ListBenchmark {
> 
> List strings;
> 
> @Setup
> public void setup() {
> strings = new ArrayList<>();
> for(int i = 0; i< 10;i++) {
> strings.add("abc: " + i);
> }
> }
> 
> @TearDown
> public void tearDown() {
> strings = null;
> }
> 
> @Benchmark
> public ObservableList init() {
> return FXCollections.observableArrayList(strings);
> }
> }
> 
> 
> 

This PR appears to have two internal behavior changes.

A. Initial collection size (PR owner's description)
B. By adding the item directly to the backingList
Avoiding beginChange () --doAdd () --endChange ()

``` java
 @Override
 protected void doAdd (int index, E element) {
 if (elementObserver! = null)
 elementObserver.attachListener (element);
 backingList.add (index, element);
 }


Looking at the following code, it seems that there is no problem because 
attachListener () is called in the constructor.
We will need to make sure that the current unit tests cover B's changes.

ObservableListWrapper.java
``` java
public ObservableListWrapper(List list) {
backingList = list;
elementObserver = null;
}

public ObservableListWrapper(List list, Callback 
extractor) {
backingList = list;
this.elementObserver = new ElementObserver(extractor, new Callback() {

@Override
public InvalidationListener call(final E e) {
return new InvalidationListener() {

@Override
public void invalidated(Observable observable) {
beginChange();
int i = 0;
final int size = size();
for (; i < size; ++i) {
if (get(i) == e) {
nextUpdate(i);
}
}
endChange();
}
};
}
}, this);
final int sz = backingList.size();
for (int i = 0; i < sz; ++i) {
elementObserver.attachListener(backingList.get(i));
}
}

-

PR: https://git.openjdk.java.net/jfx/pull/758


Re: Promote addEventHandler/removeEventHandler to EventTarget interface

2022-03-18 Thread John Hendrikx

On 17/03/2022 21:01, Michael Strauß wrote:

I'm working on an application that uses the JavaFX event system
extensively, and I'm finding it quite hard to use common code for
event handler registrations.

The problem is that the `EventTarget` interface contains no
addEventHandler/removeEventHandler methods, and as a consequence of
that, code that uses `EventTarget` ends up requiring lots of brittle
instanceof tests to call these methods on all the different
implementations of `EventTarget`.

There are three buckets of `EventTarget` implementations:

1) Implementations that declare the following methods:
  void addEventHandler(EventType,
EventHandler);
  void removeEventHandler(EventType,
EventHandler);
  void addEventFilter(EventType, EventHandler);
  void removeEventFilter(EventType,
EventHandler);
--> Node, Scene, Window, Transform, Task, Service

2) Implementations that declare the following methods:
  void addEventHandler(EventType, EventHandler);
  void removeEventHandler(EventType, EventHandler);
--> MenuItem, TreeItem, TableColumnBase

(Note that the EventHandler argument ist parameterized as
EventHandler, not EventHandler as in the first set of
methods.)

3) Implementations that don't declare any methods to add or remove
event handlers:
--> Dialog, Tab

I think the situation can be improved by promoting the bucket 1
methods to the `EventTarget` interface, so they can be used
consistently across all implementations of `EventTarget`.

This works seamlessly for bucket 1 and bucket 3 implementations.
I think it would make good sense that they also have the 
EventHandler/Filters methods for consistency.  If they're targets for 
Events, then you should be able to add handlers or filters for them.

With bucket 2, there's the problem that, inconsistently, the
EventHandler argument is not a lower-bounded wildcard.
Unfortunately, a method with an EventHandler parameter cannot
implement an interface method that expects EventHandler.

However, since the erasure of the method remains the same, changing
the method signature would technically be a binary-compatible change.
They also forward directly to the internal EventHandlerManager which 
does accept EventHandler.  This seems like an oversight when 
those implementations were added to MenuItem, TreeItem and TableColumnBase.

Do you think this is a useful improvement?


I think that all the implementations should definitely be consistent; 
Dialog and Tab probably should have the means to add EventHandlers and 
EventFilters; and MenuItem, TreeItem, TableColumnBase could do with the 
EventFilter variants.


If they're going to be consistent, might as well add these methods to 
the EventTarget interface to enforce this for any future EventTarget 
implementations.


EventTarget is already public API, and so there might be 3rd party 
implementations.  This means that the methods added to the EventTarget 
interface must be default methods. It would be super if these default 
implementations would just work out of the box, but that would require 
exposing the internal class EventHandlerManager (and adding a 
`getEventHandlerManager` to the `EventTarget` interface).  If that's not 
realistic, then initially the default implementations would have to 
throw UnsupportedOperationException.


--John



Re: Question about fatal JavaFX crashes

2022-03-18 Thread Scott Palmer
I wonder, could you do the opposite and force compilation to trigger the bug 
more consistently?

Scott

> On Mar 18, 2022, at 2:03 PM, Philip Race  wrote:
> 
> I have at least seen  JIT compiler crashes like this in other unrelated 
> cases ..
> 
> in theory you can use an option like
> 
> -XX:CompileCommand=exclude,javafx/scene/control/TableView$5::onChanged
> 
> although I am very unsure about the syntax for the last part of it especially 
> with what looks like
> some anonymous method or lambda expression. I can almost guarantee that my 
> example is wrong.
> 
> Excluding the whole of TableView might be easier to specify but will slow you 
> down a lot.
> 
> -phil.
> 
> 
>> On 3/18/22 10:37 AM, Kevin Rushforth wrote:
>> Thanks, Phil. That was my take as well. I don't see how this can be a JavaFX 
>> bug given where it is crashing. FWIW, I haven't ever seen anything like this.
>> 
>> -- Kevin
>> 
>> 
>>> On 3/18/2022 10:25 AM, Philip Race wrote:
>>> I think it is probable that this is a hotspot VM problem in the C2 JIT 
>>> compiler code.
>>> 
>>> I've moved your bug report to hotspot : 
>>> https://bugs.openjdk.java.net/browse/JDK-8283386
>>> The interesting question isn't about which version of FX used to work.
>>> It is what was the last working version of the JDK.
>>> It looks a bit like a JDK 17 bug from the evidence so far so if
>>> you were running FX 17 on JDK 17 GA you maybe have picked up a later update 
>>> release  of JDK 17 along with FX 18 ??
>>> 
>>> Regardless this doesn't look like an FX bug.
>>> But unless you can actually provide a test case, or, by luck the hotspot 
>>> folks recognise the issue,
>>> I don't know what can/will happen.
>>> 
>>> -phil.
>>> 
>>> On 3/18/22 7:08 AM, Daniel Peintner wrote:
 Hi John, all,
 
 Thanks for your detailed reply.
 I submitted a bug report with detailed information.
 
 - Run a different Java version
 I tried different versions and vendors with the same result.
 * OpenJDK 17.0.1
 * Zulu 17.0.2
 * I wanted to check also JDK18-ea but gradle does not yet support it
 
 
> - Try switching to a different GC
> - Use different VM options (are you using anything special?)
> - Anything else that is not often used, non-standard or experimental,
> try going to a more common setup
> 
 I use no specific setting, all is default.
 I have been reading that the -client flag might help in some cases but
 unfortunately this flag is no longer taken into account for 64bit JVMS.
 
 Thanks,
 
 -- Daniel
 
 
 
 
> --John
> 
> On 18/03/2022 09:43, Daniel Peintner wrote:
>> Hello,
>> 
>> I take the liberty to ask on the email reflector if there are other
> people
>> with similar problems.
>> 
>> Since updating my application to JavaFX18 I get random fatal crashes.
>> Unfortunately it is not predictable but after some time the app crashes
>> with "EXCEPTION_ACCESS_VIOLATION".
>> 
>> The hs_error_pidX reports the following.
>> 
>> 
>> ---  T H R E A D  ---
>> 
>> Current thread (0x016be9c9b410):  JavaThread "C2 CompilerThread0"
>> daemon [_thread_in_native, id=3068,
>> stack(0x00393e80,0x00393e90)]
>> 
>> Current CompileTask:
>> C2:10515286 18185   4
>   javafx.scene.control.TableView$5::onChanged
>> (1049 bytes)
>> 
>> 
>> Am I the only one seeing this?
>> I am not sure if this relates to changing JavaFX 17 to 18 or whether it
>> makes it just more likely.
>> 
>> It seems to be related to "javafx.scene.control.TableView$5::onChanged"
>> since all crashes show this line.
>> 
>> Having said that, I cannot provide a test-case since it happens all of a
>> sudden and sometimes after hours using the application.
>> 
>> I am open to any feedback or input on how to proceed best.
>> 
>> Thanks!
>> 
>> -- Daniel
>>> 
>> 
> 


Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v10]

2022-03-18 Thread Michael Strauß
On Fri, 18 Mar 2022 10:17:01 GMT, John Hendrikx  wrote:

>> modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java 
>> line 197:
>> 
>>> 195: /**
>>> 196:  * Creates an {@code ObservableValue} that holds the value of an 
>>> {@code ObservableValue}
>>> 197:  * resulting from applying a mapping on this {@code 
>>> ObservableValue}'s value. The result
>> 
>> While technically correct, I think the first sentence should focus more on 
>> the purpose of this method.
>> 
>> How about something like this:
>> `Creates a new {@code ObservableValue} that holds the value of a nested 
>> {@code ObservableValue} by applying a mapping function to extract the nested 
>> {@code ObservableValue}.`
>> 
>> That's not as precise, but it makes the purpose much more clear.
>
> I've changed this to use your wording as I think it does read much better.
> 
> Perhaps also possible:
> 
>   Creates a new {@code ObservableValue} that holds the value of a nested 
> {@code ObservableValue} supplied
>   by the given mapping function.
> 
> ?

Both seem fine, I don't have any preference over one or the other.

-

PR: https://git.openjdk.java.net/jfx/pull/675


Re: RFR: 8283063: Optimize Observable{List/Set/Map}Wrapper.retainAll/removeAll

2022-03-18 Thread yosbits
On Sat, 12 Mar 2022 04:57:37 GMT, Michael Strauß  wrote:

> `Observable{List/Set/Map}Wrapper.retainAll/removeAll` can be optimized for 
> some edge cases.
> 
> 1. `removeAll(c)`:
> This is a no-op if 'c' is empty.
> For `ObservableListWrapper`, returning early skips an object allocation. For 
> `ObservableSetWrapper` and `ObservableMapWrapper`, returning early prevents 
> an enumeration of the entire collection.
> 
> 2. `retainAll(c)`:
> This is a no-op if the backing collection is empty, or equivalent to 
> `clear()` if `c` is empty.
> 
> I've added some tests to verify the optimized behavior for each of the three 
> classes.

This is an equivalent change that partially includes the fix I proposed in #305.
It is good to apply it to Map / Set.

-

PR: https://git.openjdk.java.net/jfx/pull/751


Re: RFR: 8283346: Optimize observable ArrayList creation in FXCollections

2022-03-18 Thread Michael Strauß
On Thu, 17 Mar 2022 21:10:14 GMT, Marius Hanl  wrote:

> This simple PR optimizes the observable `ArrayList` creation by using the 
> ArrayList constructor/array size so that the underlying array will be 
> initialized at the correct size which will speed up the creation as the array 
> does not need to grow as a result of the `addAll` call.
> 
> I also added tests which will succeed before and after to verify that nothing 
> got broken by this change.
> Also I made a benchmark test. Results:
> 
> | Benchmark | Mode| Cnt | Score | Error | Units
> | - | - | - | - | 
> - | - |
> | ListBenchmark OLD  | thrpt | 25 | 722,842 | ± 26,93 | ops/s
> | ListBenchmark NEW | thrpt  | 25 | 29262,274 | ± 2088,712 | ops/s
> 
> Benchmark code
> 
> 
> import javafx.collections.FXCollections;
> import javafx.collections.ObservableList;
> import org.openjdk.jmh.annotations.Benchmark;
> import org.openjdk.jmh.annotations.Scope;
> import org.openjdk.jmh.annotations.Setup;
> import org.openjdk.jmh.annotations.State;
> import org.openjdk.jmh.annotations.TearDown;
> 
> import java.util.ArrayList;
> import java.util.List;
> 
> @State(Scope.Benchmark)
> public class ListBenchmark {
> 
> List strings;
> 
> @Setup
> public void setup() {
> strings = new ArrayList<>();
> for(int i = 0; i< 10;i++) {
> strings.add("abc: " + i);
> }
> }
> 
> @TearDown
> public void tearDown() {
> strings = null;
> }
> 
> @Benchmark
> public ObservableList init() {
> return FXCollections.observableArrayList(strings);
> }
> }
> 
> 
> 

The following pieces of code should be identical:

var list = FXCollections.observableArrayList();
list.addAll(source);


var list = FXCollections.observableList(new ArrayList<>(source));

Any observable difference in behavior would be unspecified. When it comes to 
`modCount`, that's an internal field of `AbstractList`.
`FXCollections.observableArrayList()` and `FXCollections.observableList(...)` 
are not the right place to test this.

-

PR: https://git.openjdk.java.net/jfx/pull/758