Re: RFR: 8268225: Support :focus-visible and :focus-within CSS pseudoclasses [v9]

[Michael Strauß]

> This PR adds the `Node.focusVisible` and `Node.focusWithin` properties, as 
> well as the corresponding `:focus-visible` and `:focus-within` CSS 
> pseudo-classes.

Michael Strauß has updated the pull request with a new target base due to a 
merge or a rebase. The incremental webrev excludes the unrelated changes 
brought in by the merge/rebase. The pull request contains 23 additional commits 
since the last revision:

 - Merge branch 'master' into feature/focusvisible
 - Updated since tag
 - Update copyright headers
 - Merge branch 'master' into feature/focusvisible
 - fixed undeterministic test failures
 - minor wording change
 - restart github actions
 - Merge branch 'master' of https://github.com/mstr2/jfx into 
feature/focusvisible
 - changes per discussion, added test
 - wording
 - ... and 13 more: https://git.openjdk.java.net/jfx/compare/a6d4f5e5...0c7d6e72

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/475/files
  - new: https://git.openjdk.java.net/jfx/pull/475/files/bd57bbac..0c7d6e72

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=475=08
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=475=07-08

  Stats: 101583 lines in 580 files changed: 34334 ins; 31264 del; 35985 mod
  Patch: https://git.openjdk.java.net/jfx/pull/475.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/475/head:pull/475

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

Re: RFR: 8217853: Cleanup in the D3D native pipeline [v2]

[Michael Strauß]

On Mon, 2 May 2022 19:55:28 GMT, Nir Lisker  wrote:

>> Refactoring and renaming changes to some of the D3D pipeline files and a few 
>> changes on the Java side. These are various "leftovers" from previous issues 
>> that we didn't want to touch at the time in order to confine the scope of 
>> the changes. They will make future work easier.
>> 
>> Since there are many small changes, I'm giving a full list here:
>> 
>> **Java**
>> 
>> * `NGShape3D.java`
>>   * Extracted methods to help with the cumbersome lighting loop: one method 
>> per light type + empty light (reset light) + default point light. This 
>> section of the code would benefit from the upcoming pattern matching on 
>> `switch`.
>>   * Normalized the direction here instead of in the native code.
>>   * Ambient light is now only set when it exists (and is not black).
>> * `NGPointLight,java` - removed unneeded methods that were used by 
>> `NGShape3D` before the per-lighting methods were extracted (point light 
>> doesn't need spotlight-specific methods since they each have their own "add" 
>> method).
>> * `NGSpotLight.java` - removed `@Override` annotations as a result of the 
>> above change.
>> 
>> **Native C++**
>> 
>> * Initialized the class members of `D3DLight`, `D3DMeshView`  and 
>> `D3DPhongMaterial` in the header file instead of a more cumbersome 
>> initialization in the constructor (this is allowed since C++ 11). 
>> * `D3DLight`
>>   * Commented out unused methods. Were they supposed to be used at some 
>> point?
>>   * Renamed the `w` component to `lightOn` since it controls the number of 
>> lights for the special pixel shader variant and having it in the 4th 
>> position of the color array was confusing.
>> * `D3DPhongShader.h`
>>   * Renamed some of the register constants for more clarity.
>>   * Moved the ambient light color constant from the vertex shader to the 
>> pixel shader (see the shader section below). I don't understand the 
>> calculation of the number of registers in the comment there: "8 ambient 
>> points + 2 coords = 10". There is 1 ambient light, what are the ambient 
>> points and coordinates? In `vsConstants` there is `gAmbinetData[10]`, but it 
>> is unused.
>>   * Reduced the number of assigned vertex register for the `VSR_LIGHTS` 
>> constant since it included both position and color, but color was unused 
>> there (it was used directly in the pixel shader), so now it's only the 
>> position.
>> * `D3DMeshView.cc`
>>   * Unified the lighting loop that prepares the lights' 4-vetors that are 
>> passed to the shaders.
>>   * Removed the direction normalization as stated in the change for 
>> `NGShape3D.java`.
>>   * Reordered the shader constant assignment to be the same order as in 
>> `D3DPhongShader.h`.
>> 
>> **Native shaders**
>> * Renamed many of the variables to what I think are more descriptive names. 
>> This includes noting in which space they exist as some calculations are done 
>> in model space, some in world space, and we might need to do some in view 
>> space. For vectors, also noted if the vector is to or from (`eye` doesn't 
>> tell me if it's from or to the camera).
>> * Commented out many unused functions. I don't know what they are for, so I 
>> didn't remove them.
>> * `vsConstants`
>>   * Removed the light color from here since it's unused, only the position 
>> is.
>>   * Removed the ambient light color constant from here since it's unused, 
>> and added it to `psConstants` instead.
>> * `vs2ps`
>>   * Removed the ambient color interpolation, which frees a register (no 
>> change in performance).
>>   * Simplified the structure (what is `LocalBumpOut` and why is it called 
>> `light` and contains `LocalBump?).
>> * `Mtl1PS` and `psMath`
>>   * Moved the shader variant constants (`#ifndef`) to `Mtl1PS` where they 
>> are used for better clarity.
>>   * Moved the lights loop to `Mtl1PS`. The calculation itself remains in 
>> `psMath`.
>> 
>> No changes in performance were measured and the behavior stayed the same.
>
> Nir Lisker has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Remove unused comments, clean constructor

modules/javafx.graphics/src/main/native-prism-d3d/D3DLight.h line 34:

> 32: public:
> 33: D3DLight() = default;
> 34: virtual ~D3DLight() = default;

It doesn't seem like this class is supposed to be subclassable. I would suggest 
removing the constructor and destructor declarations and marking the class 
`final`.

modules/javafx.graphics/src/main/native-prism-d3d/D3DMeshView.cc line 149:

> 147: float spotLightsFactors[MAX_NUM_LIGHTS * 4];   // 2 angles + 1 
> falloff + 1 padding
> 148: for (int i = 0, d = 0, p = 0, c = 0, a = 0, r = 0, s = 0; i < 
> MAX_NUM_LIGHTS; i++) {
> 149: D3DLight light = lights[i];

You're invoking the auto-generated copy constructor of `D3DLight` here, where 
the original code didn't do that. Just making sure that that's what you 
intended.

Re: RFR: 8268225: Support :focus-visible and :focus-within CSS pseudoclasses [v8]

[Michael Strauß]

> This PR adds the `Node.focusVisible` and `Node.focusWithin` properties, as 
> well as the corresponding `:focus-visible` and `:focus-within` CSS 
> pseudo-classes.

Michael Strauß has updated the pull request with a new target base due to a 
merge or a rebase. The incremental webrev excludes the unrelated changes 
brought in by the merge/rebase. The pull request contains 21 additional commits 
since the last revision:

 - Update copyright headers
 - Merge branch 'master' into feature/focusvisible
 - fixed undeterministic test failures
 - minor wording change
 - restart github actions
 - Merge branch 'master' of https://github.com/mstr2/jfx into 
feature/focusvisible
 - changes per discussion, added test
 - wording
 - Re-ordered methods
 - remove code comment
 - ... and 11 more: https://git.openjdk.java.net/jfx/compare/129c1ffa...bd57bbac

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/475/files
  - new: https://git.openjdk.java.net/jfx/pull/475/files/d9a715e3..bd57bbac

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=475=07
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=475=06-07

  Stats: 473015 lines in 7841 files changed: 268935 ins; 144862 del; 59218 mod
  Patch: https://git.openjdk.java.net/jfx/pull/475.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/475/head:pull/475

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

Re: Promote addEventHandler/removeEventHandler to EventTarget interface

[Michael Strauß]

I think defaulting to UnsupportedOperationException is a good choice.
This shouldn't break existing usages of 3rd party implementations,
since they wouldn't be calling those APIs anyway.


On Fri, Mar 18, 2022 at 10:55 PM John Hendrikx  wrote:
>
> [...]
> 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: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v13]

[Michael Strauß]

On Mon, 21 Mar 2022 08:59:34 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:
> 
>   Small wording change in API of ObservableValue after proof reading

Marked as reviewed by mstrauss (Author).

-

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

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

[Michael Strauß]

On Sun, 20 Mar 2022 03:28:01 GMT, Nir Lisker  wrote:

>> 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  ?
>
> I read this comment after what I wrote about `flatMap`, so mstr2 also had the 
> idea of "More precisely", which is good :)
> 
> I would suggested something similar to what I did there:
> 
> 
> Creates a new {@code ObservableValue} that holds the value supplied by the 
> given mapping function. The result
> is updated when this {@code ObservableValue} changes.
> If this value is {@code null}...
> More precisely, the created {@code ObservableValue} holds the result of 
> applying a mapping on this
> {@code ObservableValue}'s value.
> 
> 
> Same comments about `@return` and `@throws` NPE as I had for `flatMap`.
> 
> `orElse` will also becomes something like
> 
> 
> Creates a new {@code ObservableValue} that holds this value, or the given 
> value if it is {@code null}. The
> result is updated when this {@code ObservableValue} changes.
> More precisely, the created {@code ObservableValue} holds this {@code 
> ObservableValue}'s value, or
> the given value if it is {@code null}.
> 
> 
> Also not sure if the "More precisely" description is needed here.

I think I've arrived at the conclusing that the "more precisely" description is 
basically rehashing the semantics of `ObservableValue` over and over again.
It should be clear for a reader who is familiar with ObservableValue that we're 
not suggesting that somehow the given ObservableValue _instance_ is stored in 
the new ObservableValue.

-

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

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

[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

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

[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: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v10]

[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 

Promote addEventHandler/removeEventHandler to EventTarget interface

[Michael Strauß]

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.

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.

Do you think this is a useful improvement?

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

[Michael Strauß]

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

-

Commit messages:
 - Optimize removeAll/retainAll for Observable{List/Set/Map}Wrapper
 - Failing test

Changes: https://git.openjdk.java.net/jfx/pull/751/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jfx=751=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8283063
  Stats: 293 lines in 6 files changed: 290 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jfx/pull/751.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/751/head:pull/751

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

Re: RFR: 8273336: Clicking a selected cell from a group of selected cells in a TableView clears the selected items list but remains selected [v3]

[Michael Strauß]

On Mon, 7 Feb 2022 18:46:55 GMT, Jose Pereda  wrote:

>> This PR adds a predicate to TableView and TreeTableView selection models 
>> order to remove rows from the selection only when there are no selected 
>> cells in that given row, when cell selection is enabled.
>> 
>> Two tests have been added as well, that fail without this PR and pass with 
>> it.
>
> Jose Pereda has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Address feedback from reviewer

Marked as reviewed by mstrauss (Author).

-

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

Re: RFR: 8187309: TreeCell must not change tree's data

[Michael Strauß]

On Wed, 2 Feb 2022 14:18:18 GMT, Jeanette Winzenburg  
wrote:

> Issue was TreeView commit editing implementation violated the spec'ed 
> mechanism:
> 
> - no default commit handler on TreeView
> - TreeCell modifying the data directly
> 
> Fix is to move the saving of the edited value from cell into a default commit 
> handler in tree and set that handler in the constructor.
> 
> Added tests that failed/passed before/after the fix (along with a sanity test 
> for default commit that passed also before). Also fixed a test bug (incorrect 
> registration of custom commit handler, see 
> [JDK-8280951)](https://bugs.openjdk.java.net/browse/JDK-8280951) in 
> TreeViewTest.test_rt_29650 to keep it passing.

Looks good to me.

-

Marked as reviewed by mstrauss (Author).

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

Integrated: 8277572: ImageStorage should correctly handle MIME types for images encoded in data URIs

[Michael Strauß]

On Mon, 22 Nov 2021 17:52:06 GMT, Michael Strauß  wrote:

> `com.sun.javafx.iio.ImageStorage` currently ignores the MIME image subtype 
> specified for images encoded in data URIs. This should be improved as follows:
> 
> 1. If the specified image subtype is not supported, an exception will be 
> thrown.
> 2. If the specified image subtype is supported, but the data contained in the 
> URI is of a different (but also supported) image format, the image will be 
> loaded and a warning will be logged. For example, if the MIME type is 
> "image/jpeg", but the image data is PNG, the following warning will be 
> generated:
> 
> 
> Image format 'PNG' does not match MIME type 'image/jpeg' in URI 
> 'data:image/jpeg;base64,iVBORw0KGgoAAA...AAAElFTkSuQmCC'
> 
> 
> Also, the javadoc of `javafx.scene.image.Image` incorrectly states:
> 
> 94* If a URL uses the "data" scheme, the data must be base64-encoded
> 95* and the MIME type must either be empty or a subtype of the
> 96* {@code image} type.
> 
> However, omitting the MIME type of a data URI is specified to imply 
> "text/plain" (RFC 2397, section 2). Since the `com.sun.javafx.util.DataURI` 
> class is implemented according to this specification, trying to load an image 
> without MIME type correctly fails with an `ImageStorageException`: 
> "Unexpected MIME type: text".
> 
> The solution is to fix the documentation:
> 
>   94* If a URL uses the "data" scheme, the data must be base64-encoded
> - 95* and the MIME type must either be empty or a subtype of the
> - 96* {@code image} type.
> + 95* and the MIME type must be a subtype of the {@code image} type.
> + 96* The MIME type must match the image format of the data contained 
> in
> + 97* the URL. In case of a mismatch between MIME type and image 
> format,
> + 98* the image will be loaded if the image format can be determined 
> by
> + 99* JavaFX, and a warning will be logged.

This pull request has now been integrated.

Changeset: f326e78f
Author:Michael Strauß 
Committer: Kevin Rushforth 
URL:   
https://git.openjdk.java.net/jfx/commit/f326e78ffdfcbbc9085bc50a38e0b4454b757157
Stats: 266 lines in 16 files changed: 170 ins; 8 del; 88 mod

8277572: ImageStorage should correctly handle MIME types for images encoded in 
data URIs

Reviewed-by: kcr, arapte

-

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

Re: RFR: 8277572: ImageStorage should correctly handle MIME types for images encoded in data URIs [v6]

[Michael Strauß]

On Mon, 7 Feb 2022 15:03:07 GMT, yosbits  wrote:

> At least the following method do not need to remove the static modifier, but 
> they do. Is there any benefit?
> 
> ```java
> public int getNumBands(ImageType type) {
> ```

I think the fact that the method doesn't access any instance fields is an 
arbitrary implementation detail. Making the method non-static aligns its usage 
with all other methods, which can only be called on a instance reference.

-

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

Re: RFR: 8273336: Clicking a selected cell from a group of selected cells in a TableView clears the selected items list but remains selected

[Michael Strauß]

On Fri, 7 Jan 2022 19:36:45 GMT, Jose Pereda  wrote:

> This PR adds a predicate to TableView and TreeTableView selection models 
> order to remove rows from the selection only when there are no selected cells 
> in that given row, when cell selection is enabled.
> 
> Two tests have been added as well, that fail without this PR and pass with it.

modules/javafx.controls/src/main/java/javafx/scene/control/ControlUtils.java 
line 172:

> 170: .map(TablePositionBase::getRow)
> 171: .filter(removeRowFilter)
> 172: .distinct()

Maybe `distinct` should be applied before `filter`. This can cut down the 
number of times the predicate is invoked (which iterates over all selected 
cells, so it may be a performance issue for large selections).

-

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

Re: [jfx18] RFR: 8279345: Realign class docs of LightBase and subclasses

[Michael Strauß]

On Thu, 27 Jan 2022 15:07:26 GMT, Nir Lisker  wrote:

>> Ambient light is a light that comes from all directions, scattered from 
>> different surfaces and it does not cast shadow. So I think the example of 
>> strong light and moon light should be avoided. Moon light is more like the 
>> Sun light, a directional light.
>
> What examples would you use?
> 
> If a light in a room is strong it will barely cast shadows since its 
> reflections from all directions eliminate them.
> 
> Maybe I should mention that Ambient light can be used with a dark color to 
> provide a base weak lighting of the environment, and on top of it use other 
> lights.

I think the description should focus on the meaning of the respective term in 
the lighting equation, and not on a non-technical analogy. In this case, the 
analogy is a bit misleading on several aspects, including the fact that ambient 
lighting is not dependent on an area being enclosed. Here's a suggestion:

Ambient lights add a constant term to the amount of light reflected by each 
point on
the surface of an object, thereby increasing the brightness of the object 
uniformly and
independently of the orientation of its surfaces. It is often used to represent 
the base
amount of illumination in a scene.

-

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

Re: AVIF ImageProvider registering

[Michael Strauß]

>From a quick look at the code, two things come to mind:

1. The file signature is specified as "001C667479706176",
which corresponds to "ftypav" followed by two null bytes. Maybe
"av" ist not a valid `major_brand`.

2. The first four bytes of the file specify the size of the header,
which can change depending on the number of `compatible_brands`.
That's bad news for JavaFX, since the current implementation cannot
account for variable fields in file signatures (it always matches all
bytes of the signature exactly).

On Thu, Jan 27, 2022 at 3:22 PM Scott Palmer  wrote:
>
> That link should be https://github.com/lanthale/LibHeifFX 
> 
>
>
> > On Jan 27, 2022, at 6:02 AM, Clemens Lanthaler 
> >  wrote:
> >
> > Hello everyone,
> >
> > as some of you already know I have created the project LibHeifFX 
> > (github.com/lanthaler/libheiffx) for adding HEIC (iPhone) and AVIF file 
> > format to the image class.
> >
> > Basically all is working with HEIC files, but as soons as I try to open a 
> > AVIF file OpenJFX gives me the error message "No imagedataprovider found!". 
> > But I have correctly registered AVIF as well.
> >
> > Tested OpenJFX: 17.0.2
> >
> > Could it be that I am in a conflict with an internal signature ? Because it 
> > seems that OpenJFX only is checking the file signature and ignores the 
> > extension.
> >
> > Thank you in advance.
> > Clemens

Re: AVIF ImageProvider registering

[Michael Strauß]

The error message of the exception that is thrown by `ImageStorage`
when attempting to load an unsupported image  type is: "No loader for
image data". If you're encountering a different exception, maybe it
originates from somewhere else.

That being said, you're right that OpenJFX doesn't care about file
extensions and only checks file signatures. For both HEIC and AVIF
files, that's "ftyp" followed by a `major_brand` identifier. Maybe
you're encountering files with different kinds of `major_brand`?

Note that image loaders are an implementation detail of OpenJFX and
not public API, which means that the classes could change at any time
without notice.


On Thu, Jan 27, 2022 at 12:04 PM Clemens Lanthaler
 wrote:
>
> Hello everyone,
>
> as some of you already know I have created the project LibHeifFX
> (github.com/lanthaler/libheiffx) for adding HEIC (iPhone) and AVIF file
> format to the image class.
>
> Basically all is working with HEIC files, but as soons as I try to open
> a AVIF file OpenJFX gives me the error message "No imagedataprovider
> found!". But I have correctly registered AVIF as well.
>
> Tested OpenJFX: 17.0.2
>
> Could it be that I am in a conflict with an internal signature ? Because
> it seems that OpenJFX only is checking the file signature and ignores
> the extension.
>
> Thank you in advance.
> Clemens

Re: RFR: 8279640: ListView with null SelectionModel/FocusModel throws NPE

[Michael Strauß]

On Tue, 11 Jan 2022 13:04:52 GMT, Marius Hanl  wrote:

> When a null value is possible, guards against are needed. Unfortunately there 
> is no built-in way to forbid null in Java as whole.

I think null checks should only be done to verify the preconditions of an 
operation, and not to guard against bugs. This means that, when `null` is not a 
_valid_ value for a property (regardless of whether it is a _possible_ value), 
no attempt should be made to guard against this in downstream code. In fact, I 
would argue that doing so is a strong code smell.

The question is whether or not `null` should be a valid value for the 
`selectionModel` and `focusModel` properties. I think there are good reasons to 
think that it shouldn't. The primitive property classes (`IntegerProperty`, 
`BooleanProperty`, etc.) have the same issue, and they deal with this situation 
by coercing the invalid `null` value to their respective default value.

It's not clear to me whether the `ListCellBehavior.anchor` attached property 
setter would need any special-casing. When `null` (or `-1` in case of 
`ListViewBehavior.setAnchor`) is passed in, it seems to remove the anchor. 
Isn't that what we'd expect?

-

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

Re: RFR: 8277572: ImageStorage should correctly handle MIME types for images encoded in data URIs [v4]

[Michael Strauß]

On Fri, 7 Jan 2022 00:01:24 GMT, Kevin Rushforth  wrote:

>> Michael Strauß has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Added test for image format without signature
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/iio/ImageStorage.java 
> line 490:
> 
>> 488: if (stream.read(header) <= 0) {
>> 489: return null;
>> 490: }
> 
> What was the reason for this change? The former would work even if the stream 
> returned less data that the size of the header in a single read, whereas the 
> latter would fail.

Well, almost. `ImageTools.readFully` throws `EOFException` if the input stream 
is empty. That doesn't make a lot of sense in the context of 
`getLoaderBySignature`, since the method should return `null` if the input 
stream doesn't contain enough data to detect a signature (and it doesn't seem 
useful to me to make a distinction between "not enough" bytes and zero bytes). 
I've opted to retain the use of `ImageTools.readFully`, but catch the 
`EOFException` and return `null` instead.

-

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

Re: RFR: 8277572: ImageStorage should correctly handle MIME types for images encoded in data URIs [v6]

[Michael Strauß]

> `com.sun.javafx.iio.ImageStorage` currently ignores the MIME image subtype 
> specified for images encoded in data URIs. This should be improved as follows:
> 
> 1. If the specified image subtype is not supported, an exception will be 
> thrown.
> 2. If the specified image subtype is supported, but the data contained in the 
> URI is of a different (but also supported) image format, the image will be 
> loaded and a warning will be logged. For example, if the MIME type is 
> "image/jpeg", but the image data is PNG, the following warning will be 
> generated:
> 
> 
> Image format 'PNG' does not match MIME type 'image/jpeg' in URI 
> 'data:image/jpeg;base64,iVBORw0KGgoAAA...AAAElFTkSuQmCC'
> 
> 
> Also, the javadoc of `javafx.scene.image.Image` incorrectly states:
> 
> 94* If a URL uses the "data" scheme, the data must be base64-encoded
> 95* and the MIME type must either be empty or a subtype of the
> 96* {@code image} type.
> 
> However, omitting the MIME type of a data URI is specified to imply 
> "text/plain" (RFC 2397, section 2). Since the `com.sun.javafx.util.DataURI` 
> class is implemented according to this specification, trying to load an image 
> without MIME type correctly fails with an `ImageStorageException`: 
> "Unexpected MIME type: text".
> 
> The solution is to fix the documentation:
> 
>   94* If a URL uses the "data" scheme, the data must be base64-encoded
> - 95* and the MIME type must either be empty or a subtype of the
> - 96* {@code image} type.
> + 95* and the MIME type must be a subtype of the {@code image} type.
> + 96* The MIME type must match the image format of the data contained 
> in
> + 97* the URL. In case of a mismatch between MIME type and image 
> format,
> + 98* the image will be loaded if the image format can be determined 
> by
> + 99* JavaFX, and a warning will be logged.

Michael Strauß has updated the pull request incrementally with one additional 
commit since the last revision:

  Don't let EOFException bubble up

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/676/files
  - new: https://git.openjdk.java.net/jfx/pull/676/files/35a41e06..3b086bd8

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=676=05
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=676=04-05

  Stats: 7 lines in 1 file changed: 6 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jfx/pull/676.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/676/head:pull/676

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

Re: RFR: 8277572: ImageStorage should correctly handle MIME types for images encoded in data URIs [v5]

[Michael Strauß]

> `com.sun.javafx.iio.ImageStorage` currently ignores the MIME image subtype 
> specified for images encoded in data URIs. This should be improved as follows:
> 
> 1. If the specified image subtype is not supported, an exception will be 
> thrown.
> 2. If the specified image subtype is supported, but the data contained in the 
> URI is of a different (but also supported) image format, the image will be 
> loaded and a warning will be logged. For example, if the MIME type is 
> "image/jpeg", but the image data is PNG, the following warning will be 
> generated:
> 
> 
> Image format 'PNG' does not match MIME type 'image/jpeg' in URI 
> 'data:image/jpeg;base64,iVBORw0KGgoAAA...AAAElFTkSuQmCC'
> 
> 
> Also, the javadoc of `javafx.scene.image.Image` incorrectly states:
> 
> 94* If a URL uses the "data" scheme, the data must be base64-encoded
> 95* and the MIME type must either be empty or a subtype of the
> 96* {@code image} type.
> 
> However, omitting the MIME type of a data URI is specified to imply 
> "text/plain" (RFC 2397, section 2). Since the `com.sun.javafx.util.DataURI` 
> class is implemented according to this specification, trying to load an image 
> without MIME type correctly fails with an `ImageStorageException`: 
> "Unexpected MIME type: text".
> 
> The solution is to fix the documentation:
> 
>   94* If a URL uses the "data" scheme, the data must be base64-encoded
> - 95* and the MIME type must either be empty or a subtype of the
> - 96* {@code image} type.
> + 95* and the MIME type must be a subtype of the {@code image} type.
> + 96* The MIME type must match the image format of the data contained 
> in
> + 97* the URL. In case of a mismatch between MIME type and image 
> format,
> + 98* the image will be loaded if the image format can be determined 
> by
> + 99* JavaFX, and a warning will be logged.

Michael Strauß has updated the pull request incrementally with one additional 
commit since the last revision:

  revert code change

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/676/files
  - new: https://git.openjdk.java.net/jfx/pull/676/files/4cb5678a..35a41e06

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=676=04
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=676=03-04

  Stats: 3 lines in 1 file changed: 0 ins; 2 del; 1 mod
  Patch: https://git.openjdk.java.net/jfx/pull/676.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/676/head:pull/676

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

Integrated: 8279328: CssParser uses default charset instead of UTF-8

[Michael Strauß]

On Wed, 29 Dec 2021 15:39:39 GMT, Michael Strauß  wrote:

> `CssParser.parse(URL)` is specified to assume UTF-8 file encoding, but 
> (implicitly) uses the default charset to read the file, potentially resulting 
> in incorrect interpretation of the file content.
> 
> This can be fixed by explicitly specifying the UTF-8 charset for 
> `InputStreamReader`.

This pull request has now been integrated.

Changeset: be3b3bd2
Author:Michael Strauß 
Committer: Kevin Rushforth 
URL:   
https://git.openjdk.java.net/jfx/commit/be3b3bd2a3af078b7c43a400014721efc6824efa
Stats: 31 lines in 2 files changed: 30 ins; 0 del; 1 mod

8279328: CssParser uses default charset instead of UTF-8

Reviewed-by: aghaisas

-

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

Integrated: 8267059: Gradle :clean and :apps tasks fail on Windows if ANT_HOME contains spaces

[Michael Strauß]

On Thu, 13 May 2021 00:07:15 GMT, Michael Strauß  wrote:

> This PR fixes an issue when building OpenJFX on Windows and command-line 
> arguments contain paths with spaces.
> 
> The problem is that on Windows, `ant` is invoked via `cmd`, which leads to 
> quotes being interpreted twice. This can be fixed with the option `cmd /s`, 
> which prevents interpreting quotes on the rest of the command line. The 
> result is as if the rest of the command line had been typed verbatim at the 
> command prompt.

This pull request has now been integrated.

Changeset: 487e4b17
Author:Michael Strauß 
Committer: Kevin Rushforth 
URL:   
https://git.openjdk.java.net/jfx/commit/487e4b17e6bfca754f8fccf2f720a1bf686d1102
Stats: 45 lines in 1 file changed: 27 ins; 15 del; 3 mod

8267059: Gradle :clean and :apps tasks fail on Windows if ANT_HOME contains 
spaces

Reviewed-by: kcr, sykora

-

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

Re: RFR: 8279328: CssParser uses default charset instead of UTF-8

[Michael Strauß]

On Thu, 6 Jan 2022 12:35:56 GMT, Ajit Ghaisas  wrote:

> The added test passes on my Mac without changing CssParser.java. It may be 
> due to the default charset being UTF-8. Which platform did you test on?

Yes, the default charset on macOS is UTF-8. I’ve tested this on Windows 11, 
which uses Windows-1252 as the default charset.

-

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

Re: RFR: 8277848 Binding and Unbinding to List leads to memory leak [v3]

[Michael Strauß]

On Wed, 5 Jan 2022 18:14:44 GMT, Florian Kirmaier  wrote:

>> Making the initial listener of the ListProperty weak fixes the problem.
>> The same is fixed for Set and Map.
>> Due to a smart implementation, this is done without any performance drawback.
>> (The trick is to have an object, which is both the WeakReference and the 
>> Changelistener)
>> By implying the same trick to the InvalidationListener, this should even 
>> improve the performance of the collection properties.
>
> Florian Kirmaier has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   JDK-8277848
>   Further optimization based code review.
>   This Bugfix should now event improve the performance

modules/javafx.base/src/main/java/javafx/beans/property/ListPropertyBase.java 
line 338:

> 336: public void onChanged(Change change) {
> 337: ListPropertyBase ref = get();
> 338: if(ref != null) {

Minor: space after `if`

modules/javafx.base/src/main/java/javafx/beans/property/MapPropertyBase.java 
line 339:

> 337: public void onChanged(Change change) {
> 338: MapPropertyBase ref = get();
> 339: if(ref != null) {

Minor: space after `if`

modules/javafx.base/src/main/java/javafx/beans/property/SetPropertyBase.java 
line 341:

> 339: public void onChanged(Change change) {
> 340: SetPropertyBase ref = get();
> 341: if(ref != null) {

Minor: space after `if`

-

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

Re: RFR: 8267059: Gradle :clean and :apps tasks fail on Windows if ANT_HOME contains spaces [v2]

[Michael Strauß]

On Fri, 27 Aug 2021 13:39:13 GMT, Kevin Rushforth  wrote:

>> Michael Strauß has updated the pull request with a new target base due to a 
>> merge or a rebase. The incremental webrev excludes the unrelated changes 
>> brought in by the merge/rebase. The pull request contains two additional 
>> commits since the last revision:
>> 
>>  - Merge branch 'master' into fixes/JDK-8267059
>>  - Use cmd /s option when building on Windows
>
> @tiainen or @arapte can one of you be the second reviewer on this?

@kevinrushforth
I think this is a very useful fix that has been tested by three people in the 
last months (of which only your review counts for Skara). Can you lower the 
number of required reviews to 1 in order for this PR to move forward to 
integration?

-

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

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

[Michael Strauß]

On Wed, 5 Jan 2022 12:08:01 GMT, John Hendrikx  wrote:

>> modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java 
>> line 188:
>> 
>>> 186:  * {@code ObservableValue} given by applying {@code mapper} on 
>>> the value
>>> 187:  * held by this {@code ObservableValue}, and is {@code null} 
>>> when this
>>> 188:  * {@code ObservableValue} holds {@code null}, never null
>> 
>> an {@code ObservableValue} that holds the value of the {@code 
>> ObservableValue} resulting
>> from applying a mapping on the value held by this {@code ObservableValue}; 
>> never {@code null} itself
>
> How about:
> 
> an {@code ObservableValue} holding the value of an {@code ObservableValue}
> resulting from a mapping of this {@code ObservableValue}'s value or
> holds {@code null} when the value is {@code null}; never returns {@code 
> null}
> 
> They are tough to describe; if you don't like the `or holds {@code null}` 
> parts I can remove those from all the functions.

Here's another attempt:

a new {@link ObservableValue} instance that holds the value that was obtained by
applying the mapping function on the value held by this {@code ObservableValue}.
If this {@code ObservableValue} holds {@code null}, the new {@code 
ObservableValue}
will also hold {@code null}.

-

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

Re: RFR: 8277848 Binding and Unbinding to List leads to memory leak [v2]

[Michael Strauß]

On Tue, 7 Dec 2021 15:21:56 GMT, Florian Kirmaier  wrote:

>> Making the initial listener of the ListProperty weak fixes the problem.
>> The same is fixed for Set and Map.
>> Due to a smart implementation, this is done without any performance drawback.
>> (The trick is to have an object, which is both the WeakReference and the 
>> Changelistener)
>> By implying the same trick to the InvalidationListener, this should even 
>> improve the performance of the collection properties.
>
> Florian Kirmaier has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   JDK-8277848
>   Added missing change

Why are the new listener imlementations called `BaseChangeListener` and 
`BaseInvalidationListener`, i.e. why the _Base_?

Also, if you're going to the trouble of refactoring the existing listener 
implementation, have you considered merging the very similar implementations 
into a single class? You can then re-use the listener instance and save another 
object allocation in this way:


private static class Listener extends WeakReference>
implements InvalidationListener, ListChangeListener, WeakListener {
Listener(ListPropertyBase ref) {
super(ref);
}

@Override
public boolean wasGarbageCollected() {
return get() == null;
}

@Override
public void onChanged(Change change) {
ListPropertyBase ref = get();
if(ref != null) {
ref.invalidateProperties();
ref.invalidated();
ref.fireValueChangedEvent(change);
}
}

@Override
public void invalidated(Observable observable) {
ListPropertyBase ref = get();
if (ref == null) {
observable.removeListener(this);
} else {
ref.markInvalid(ref.value);
}
}
}

-

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

RFR: 8279328: CssParser uses default charset instead of UTF-8

[Michael Strauß]

`CssParser.parse(URL)` is specified to assume UTF-8 file encoding, but 
(implicitly) uses the default charset to read the file, potentially resulting 
in incorrect interpretation of the file content.

This can be fixed by explicitly specifying the UTF-8 charset for 
`InputStreamReader`.

-

Commit messages:
 - Use UTF-8 charset to parse stylesheets
 - Failing test

Changes: https://git.openjdk.java.net/jfx/pull/705/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jfx=705=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8279328
  Stats: 31 lines in 2 files changed: 30 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jfx/pull/705.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/705/head:pull/705

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

Re: RFR: 8231601: Update CONTRIBUTING.md to clarify process for contributing features plus Skara changes [v9]

[Michael Strauß]

On Fri, 17 Dec 2021 15:18:51 GMT, Kevin Rushforth  wrote:

>> This PR updates the `CONTRIBUTING.md` guide to address the following:
>> 
>> 1. Clarify the process for adding new features / API changes, specifically 
>> that they must be discussed on the mailing list as the first step.
>> 2. Add a link to the mailing list in the section regarding contributing bug 
>> fixes.
>> 3. Remove the text about cross-linking the PR and JBS issue, and add a note 
>> that the Skara tooling takes care of this
>> 4. Remove the section about manually resolving the JBS issue, and add a note 
>> that the Skara bot automatically does this when the PR is integrated.
>> 5. Suggest the use of the "/reviewers 2" and "/csr" commands when appropriate
>> 6. Update the note regarding which JDK(s) to use.
>
> Kevin Rushforth has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Address review comments

Marked as reviewed by mstrauss (Author).

-

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

Re: RFR: 8231601: Update CONTRIBUTING.md to clarify process for contributing features plus Skara changes [v8]

[Michael Strauß]

On Fri, 17 Dec 2021 13:33:57 GMT, Kevin Rushforth  wrote:

>> This PR updates the `CONTRIBUTING.md` guide to address the following:
>> 
>> 1. Clarify the process for adding new features / API changes, specifically 
>> that they must be discussed on the mailing list as the first step.
>> 2. Add a link to the mailing list in the section regarding contributing bug 
>> fixes.
>> 3. Remove the text about cross-linking the PR and JBS issue, and add a note 
>> that the Skara tooling takes care of this
>> 4. Remove the section about manually resolving the JBS issue, and add a note 
>> that the Skara bot automatically does this when the PR is integrated.
>> 5. Suggest the use of the "/reviewers 2" and "/csr" commands when appropriate
>> 6. Update the note regarding which JDK(s) to use.
>
> Kevin Rushforth has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Minor updates

CONTRIBUTING.md line 18:

> 16: 
> 17: 
> 18: If you find yourself wishing for a feature that doesn't exist in OpenJFX, 
> you are probably not alone. There are bound to be others out there with 
> similar needs. Many of the features that OpenJFX has today have been added 
> because our users saw the need.

I think you should restore this sentence. It feels uplifting and encouraging, 
while the other added sections feel pretty discouraging at first sight for 
newcomers to the project.

-

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

Re: RFR: 8278260: JavaFX shared libraries not stripped on Linux or macOS [v2]

[Michael Strauß]

On Wed, 15 Dec 2021 22:57:26 GMT, Kevin Rushforth  wrote:

>> Build change to strip the non-global symbols from native shared libraries on 
>> Linux and macOS by running `strip -x`, unless doing a `-PCONF=DebugNative` 
>> build.
>> 
>> Here is a before / after size comparison. All sizes in KBytes:
>> 
>> ### Linux
>> 
>> | Native Library | Current | Stripped |
>> | --- | - |  |
>> | libavplugin-54.so | 52 | 48 |
>> | libavplugin-56.so | 52 | 48 |
>> | libavplugin-57.so | 52 | 48 |
>> | libavplugin-ffmpeg-56.so | 52 | 48 |
>> | libavplugin-ffmpeg-57.so | 52 | 48 |
>> | libavplugin-ffmpeg-58.so | 52 | 48 |
>> | libdecora_sse.so | 76 | 72 |
>> | libfxplugins.so | 56 | 52 |
>> | libglass.so | 16 | 12 |
>> | libglassgtk2.so | 932 | 324 |
>> | libglassgtk3.so | 932 | 324 |
>> | libgstreamer-lite.so | 2,280 | 2,100 |
>> | libjavafx_font.so | 20 | 16 |
>> | libjavafx_font_freetype.so | 28 | 28 |
>> | libjavafx_font_pango.so | 28 | 24 |
>> | libjavafx_iio.so | 148 | 140 |
>> | libjfxmedia.so | 2,048 | 516 |
>> | libjfxwebkit.so | 106,696 | 88,428 |
>> | libprism_common.so | 8 | 8 |
>> | libprism_es2.so | 64 | 64 |
>> | libprism_sw.so | 68 | 64 |
>> 
>> ### macOS
>> 
>> | Native Library | Current | Stripped |
>> | --- | - |  |
>> | libdecora_sse.dylib | 88 | 88 |
>> | libfxplugins.dylib | 88 | 84 |
>> | libglass.dylib | 360 | 324 |
>> | libglib-lite.dylib | 1,192 | 1,148 |
>> | libgstreamer-lite.dylib | 1,708 | 1584 |
>> | libjavafx_font.dylib | 64 | 64 |
>> | libjavafx_iio.dylib | 308 | 300 |
>> | libjfxmedia.dylib | 212 | 200 |
>> | libjfxmedia_avf.dylib | 100 | 88 |
>> | libjfxwebkit.dylib | 85,896 | 71,636 |
>> | libprism_common.dylib | 20 | 20 |
>> | libprism_es2.dylib | 64 | 64 |
>> | libprism_sw.dylib | 88 | 88 |
>
> Kevin Rushforth has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Only strip binaries when doing a production build (`-PCONF=Release`)

Here are my results when building with `-PCONF=Release` on macOS 11.6.1:

| Native Library  | current | kevinrushforth | mstr2  |
|-|-|||
| libdecora_sse.dylib | 88  | 88 | 87 |
| libfxplugins.dylib  | 88  | 84 | 83 |
| libglass.dylib  | 360 | 324| 331|
| libglib-lite.dylib  | 1,192   | 1,148  | 1,171  |
| libgstreamer-lite.dylib | 1,708   | 1584   | 1,615  |
| libjavafx_font.dylib| 64  | 64 | 64 |
| libjavafx_iio.dylib | 308 | 300| 306|
| libjfxmedia.dylib   | 212 | 200| 204|
| libjfxmedia_avf.dylib   | 100 | 88 | 89 |
| libjfxwebkit.dylib  | 85,896  | 71,636 | 72,905 |
| libprism_common.dylib   | 20  | 20 | 16 |
| libprism_es2.dylib  | 64  | 64 | 61 |
| libprism_sw.dylib   | 88  | 88 | 87 |

-

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

Re: RFR: 8278860: Streamline properties for Monocle

[Michael Strauß]

On Wed, 15 Dec 2021 20:39:54 GMT, Johan Vos  wrote:

> Base decisions in prism for embedded cases on the same glass.platform 
> property that is also used in glass.
> This PR replaces the property `embedded` with the property `glass.platform` .
> 
> There is only 1 place where the property `embedded` was read, which is in the 
> PlatformUtil.java.
> 
> There are only 2 places where the value of that property is used, and in both 
> cases the only check is to detect whether or not that property has a value of 
> "monocle".

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

> 65: 
> 66: @SuppressWarnings("removal")
> 67: String str2 = 
> AccessController.doPrivileged((PrivilegedAction) () -> 
> System.getProperty("glass.platform","").toLowerCase(Locale.ROOT));

Minor: maybe add a space after the comma

-

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

Re: RFR: 8278260: JavaFX shared libraries not stripped on Linux or macOS [v2]

[Michael Strauß]

On Wed, 15 Dec 2021 22:57:26 GMT, Kevin Rushforth  wrote:

>> Build change to strip the non-global symbols from native shared libraries on 
>> Linux and macOS by running `strip -x`, unless doing a `-PCONF=DebugNative` 
>> build.
>> 
>> Here is a before / after size comparison. All sizes in KBytes:
>> 
>> ### Linux
>> 
>> | Native Library | Current | Stripped |
>> | --- | - |  |
>> | libavplugin-54.so | 52 | 48 |
>> | libavplugin-56.so | 52 | 48 |
>> | libavplugin-57.so | 52 | 48 |
>> | libavplugin-ffmpeg-56.so | 52 | 48 |
>> | libavplugin-ffmpeg-57.so | 52 | 48 |
>> | libavplugin-ffmpeg-58.so | 52 | 48 |
>> | libdecora_sse.so | 76 | 72 |
>> | libfxplugins.so | 56 | 52 |
>> | libglass.so | 16 | 12 |
>> | libglassgtk2.so | 932 | 324 |
>> | libglassgtk3.so | 932 | 324 |
>> | libgstreamer-lite.so | 2,280 | 2,100 |
>> | libjavafx_font.so | 20 | 16 |
>> | libjavafx_font_freetype.so | 28 | 28 |
>> | libjavafx_font_pango.so | 28 | 24 |
>> | libjavafx_iio.so | 148 | 140 |
>> | libjfxmedia.so | 2,048 | 516 |
>> | libjfxwebkit.so | 106,696 | 88,428 |
>> | libprism_common.so | 8 | 8 |
>> | libprism_es2.so | 64 | 64 |
>> | libprism_sw.so | 68 | 64 |
>> 
>> ### macOS
>> 
>> | Native Library | Current | Stripped |
>> | --- | - |  |
>> | libdecora_sse.dylib | 88 | 88 |
>> | libfxplugins.dylib | 88 | 84 |
>> | libglass.dylib | 360 | 324 |
>> | libglib-lite.dylib | 1,192 | 1,148 |
>> | libgstreamer-lite.dylib | 1,708 | 1584 |
>> | libjavafx_font.dylib | 64 | 64 |
>> | libjavafx_iio.dylib | 308 | 300 |
>> | libjfxmedia.dylib | 212 | 200 |
>> | libjfxmedia_avf.dylib | 100 | 88 |
>> | libjfxwebkit.dylib | 85,896 | 71,636 |
>> | libprism_common.dylib | 20 | 20 |
>> | libprism_es2.dylib | 64 | 64 |
>> | libprism_sw.dylib | 88 | 88 |
>
> Kevin Rushforth has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Only strip binaries when doing a production build (`-PCONF=Release`)

I don't think that stripping unused symbols is necessary for debug builds, and 
it's okay to only enable such optimizations for release builds.

-

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

Re: RFR: 8268225: Support :focus-visible and :focus-within CSS pseudoclasses [v2]

[Michael Strauß]

On Mon, 6 Dec 2021 13:05:16 GMT, Jeanette Winzenburg  
wrote:

> might also need a test that verifies the focusWithin of a parent added 
> somewhere above the focused node? hmm .. or maybe not, that would require to 
> re-arrange a complete subtree ..

Inserting a parent into a scene graph such that the existing subtree at that 
position becomes a subtree of the newly inserted parent can't be done as an 
atomic operation. First, you'll need to remove the subtree at the insertion 
point from the existing scene graph (otherwise you'll get an exception saying 
that a node can only appear once in a scene graph). Then you can add the new 
parent with the removed subtree as its child.

But what happens if the removed subtree contains a focused node? Since we can't 
know whether the removed subtree will ever be re-attached to the scene graph, 
we probably shouldn't keep its focus flags set. Moreover, `Scene.focusOwner` 
probably also should not refer to a node that is not part of the scene graph 
anymore.

But that's not what happens:

// Create a focusable rect
var rect = new Rectangle();
rect.setFocusTraversable(true);

// Add the rect to a group and requestFocus on the rect
var root = new Group(rect);
scene.setRoot(root);
rect.requestFocus();

// Now, the rect is focused and it is the focus owner of the scene
assertTrue(rect.isFocused());
assertSame(rect, scene.getFocusOwner());

// Remove the rect from the scene graph
root.getChildren().clear();

// This is what I would now assume to be true (but isn't):
assertFalse(rect.isFocused());  // FAILED: rect.isFocused() == true
assertNotSame(rect, scene.getFocusOwner()); // FAILED: rect == 
scene.getFocusOwner()

I'm inclined to think that this behavior is a bug.

-

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

Integrated: 8276313: ScrollPane scroll delta incorrectly depends on content height

[Michael Strauß]

On Tue, 2 Nov 2021 10:49:45 GMT, Michael Strauß  wrote:

> This PR fixes an issue where the scroll delta of ScrollPane incorrectly 
> depends on the size of its content.
> This leads to extremely slow scrolling when the content is only slightly 
> larger than the ScrollPane.

This pull request has now been integrated.

Changeset: 5805bf8e
Author:Michael Strauß 
Committer: Johan Vos 
URL:   
https://git.openjdk.java.net/jfx/commit/5805bf8e5de90427a68164e52c1fbbd08bfd0aa4
Stats: 73 lines in 2 files changed: 53 ins; 11 del; 9 mod

8276313: ScrollPane scroll delta incorrectly depends on content height

Reviewed-by: kcr, aghaisas, jvos

-

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

Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589 [v4]

[Michael Strauß]

On Fri, 3 Dec 2021 20:46:46 GMT, Johan Vos  wrote:

>> After (re)setting the number of elements, make sure to do at least some 
>> estimation of the total size.
>> Added a testcase for this scenario.
>
> Johan Vos has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   add checks on positive values on counters

Marked as reviewed by mstrauss (Author).

-

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

Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589 [v3]

[Michael Strauß]

On Fri, 3 Dec 2021 11:25:43 GMT, Johan Vos  wrote:

>> After (re)setting the number of elements, make sure to do at least some 
>> estimation of the total size.
>> Added a testcase for this scenario.
>
> Johan Vos has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   process reviewer comment (simplify call)

Marked as reviewed by mstrauss (Author).

-

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

Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589 [v3]

[Michael Strauß]

On Fri, 3 Dec 2021 11:25:43 GMT, Johan Vos  wrote:

>> After (re)setting the number of elements, make sure to do at least some 
>> estimation of the total size.
>> Added a testcase for this scenario.
>
> Johan Vos has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   process reviewer comment (simplify call)

Would it be a good idea to fix the implementation of the `position` property as 
part of this PR (it has the same problem as `cellCount` before the fix)? If 
not, a follow-up ticket should be filed.

-

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

Re: RFR: 8276313: ScrollPane scroll delta incorrectly depends on content height [v3]

[Michael Strauß]

> This PR fixes an issue where the scroll delta of ScrollPane incorrectly 
> depends on the size of its content.
> This leads to extremely slow scrolling when the content is only slightly 
> larger than the ScrollPane.

Michael Strauß has updated the pull request incrementally with one additional 
commit since the last revision:

  Changes per review

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/660/files
  - new: https://git.openjdk.java.net/jfx/pull/660/files/147858d7..6b8985f7

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=660=02
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=660=01-02

  Stats: 12 lines in 1 file changed: 0 ins; 0 del; 12 mod
  Patch: https://git.openjdk.java.net/jfx/pull/660.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/660/head:pull/660

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

Re: RFR: 8276313: ScrollPane scroll delta incorrectly depends on content height [v2]

[Michael Strauß]

On Thu, 2 Dec 2021 21:51:59 GMT, Kevin Rushforth  wrote:

>> Michael Strauß has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Handle division by zero
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/ScrollPaneSkin.java
>  line 895:
> 
>> 893: double vRange = 
>> getSkinnable().getVmax()-getSkinnable().getVmin();
>> 894: double vPixelValue = vRange / (nodeHeight - 
>> contentHeight);
>> 895: vPixelValue = Double.isFinite(vPixelValue) ? 
>> vPixelValue : 0.0;
> 
> I liked the previous logic better and would have just changed the comparison 
> to check `(nodeHeight - contentHeight) > 0.0` (possibly saving the adjusted 
> width or height in a local various to avoid doing it twice). As it is, this 
> is replacing an explicit check on the source values with an out of range 
> check on the result, which seems less intuitive. Also, the previous code did 
> a `> 0` test and the new code effectively does a `!= 0` test.

I changed it to check the denominator values instead.

-

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

Re: RFR: 8276313: ScrollPane scroll delta incorrectly depends on content height [v2]

[Michael Strauß]

On Wed, 1 Dec 2021 09:45:40 GMT, Ajit Ghaisas  wrote:

>> Michael Strauß has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Handle division by zero
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/ScrollPaneSkin.java
>  line 896:
> 
>> 894: double vPixelValue;
>> 895: if (nodeHeight > 0.0) {
>> 896: vPixelValue = vRange / (nodeHeight - contentHeight);
> 
> This may result in divide by 0.

Good catch! I've also fixed a few similar issues in other places.

-

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

Re: RFR: 8276313: ScrollPane scroll delta incorrectly depends on content height [v2]

[Michael Strauß]

> This PR fixes an issue where the scroll delta of ScrollPane incorrectly 
> depends on the size of its content.
> This leads to extremely slow scrolling when the content is only slightly 
> larger than the ScrollPane.

Michael Strauß has updated the pull request incrementally with one additional 
commit since the last revision:

  Handle division by zero

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/660/files
  - new: https://git.openjdk.java.net/jfx/pull/660/files/1605eec4..147858d7

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=660=01
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=660=00-01

  Stats: 19 lines in 1 file changed: 4 ins; 11 del; 4 mod
  Patch: https://git.openjdk.java.net/jfx/pull/660.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/660/head:pull/660

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

Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589

[Michael Strauß]

On Mon, 29 Nov 2021 11:56:45 GMT, Johan Vos  wrote:

> After (re)setting the number of elements, make sure to do at least some 
> estimation of the total size.
> Added a testcase for this scenario.

modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
 line 903:

> 901: recalculateAndImproveEstimatedSize(2);
> 902: 
> 903: cellCount.set(value);

Can this be implemented in a way that doesn't violate the property contract? 
Since this property is public API, `setCellCount(int)` should just call 
`cellCountProperty().set(int)`. Maybe you can extract the composite operation 
here into a private method and use that instead of `setCellCount(int)`.

`position` has the same problem.

-

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

Integrated: 8276206: Rename TextBinding class to better express its purpose

[Michael Strauß]

On Wed, 24 Nov 2021 04:23:59 GMT, Michael Strauß  wrote:

> This PR renames `TextBinding` to `MnemonicInfo` and adds the following text 
> to its javadoc:
> 
>   /**
> 33 +* Provides information about mnemonics contained within a string.

This pull request has now been integrated.

Changeset: fc3792d5
Author:    Michael Strauß 
Committer: Jeanette Winzenburg 
URL:   
https://git.openjdk.java.net/jfx/commit/fc3792d5cb39cd3706f28953c3f97bdecc07bba6
Stats: 320 lines in 6 files changed: 146 ins; 146 del; 28 mod

8276206: Rename TextBinding class to better express its purpose

Reviewed-by: kcr, fastegal

-

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

Cross-compiling JavaFX for Apple ARM64

[Michael Strauß]

JavaFX can be cross-compiled for Apple ARM64 on an Intel Mac by
specifying TARGET_ARCH=arm64 for the Gradle build.
This works for all native libraries except for WebKit, because
TARGET_ARCH is not passed to WebKit's cmake script. Are there any
plans to support this particular scenario?

Re: Cleaning up warnings in the Mac glass code

[Michael Strauß]

That's a good idea. In general, warnings should always be treated as errors.


On Thu, Nov 25, 2021 at 2:01 AM Martin Fox  wrote:
>
> The Mac glass code generates a lot of compiler warnings. I tried cleaning 
> this up and discovered that two of the warnings are brand new, courtesy of 
> me. One of the headers in PR #651 has a typo that I didn’t catch and neither 
> did the two reviewers. The compiler generated two warnings but they were lost 
> in the sea.
>
> I turned on warnings-as-errors for the Mac glass code and waded through the 
> results. There are a couple of minor coding errors in addition to mine which 
> should be cleaned up (like passing NO to a function that requires a non-null 
> pointer). There’s also a few deprecated calls across a small handful of files 
> which have easy replacements and are probably worth fixing.
>
> Unfortunately Apple re-named a bunch of constants in 10.12 and deprecated the 
> older forms (I think this was to rationalize the naming with Swift). These 
> form the bulk of the warnings and affect multiple files. Not a fan of that 
> sort of code churn but the alternative is to live with a slew of warnings 
> forever. Apple is not going to un-deprecate those constants.
>
> You can see the changes I made in my github repository (beldenfox/jfx) in the 
> branch ‘macwarnings’. There’s multiple commits, the first and biggest 
> catching most of the easy stuff like constant renaming.
>
> In the short term it might be worth disabling deprecation warnings and 
> turning on warnings-as-errors. Then at least outright coding errors (like 
> mine!) have a chance of being caught.
>
>
>
>

RFR: 8276206: Rename TextBinding class to better express its purpose

[Michael Strauß]

This PR renames `TextBinding` to `MnemonicInfo` and adds the following text to 
its javadoc:

  /**
33 +* Provides information about mnemonics contained within a string.

-

Commit messages:
 - Rename TextBinding to MnemonicInfo

Changes: https://git.openjdk.java.net/jfx/pull/678/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jfx=678=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8276206
  Stats: 320 lines in 6 files changed: 146 ins; 146 del; 28 mod
  Patch: https://git.openjdk.java.net/jfx/pull/678.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/678/head:pull/678

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

Re: RFR: 8277572: ImageStorage should correctly handle MIME types for images encoded in data URIs [v4]

[Michael Strauß]

> `com.sun.javafx.iio.ImageStorage` currently ignores the MIME image subtype 
> specified for images encoded in data URIs. This should be improved as follows:
> 
> 1. If the specified image subtype is not supported, an exception will be 
> thrown.
> 2. If the specified image subtype is supported, but the data contained in the 
> URI is of a different (but also supported) image format, the image will be 
> loaded and a warning will be logged. For example, if the MIME type is 
> "image/jpeg", but the image data is PNG, the following warning will be 
> generated:
> 
> 
> Image format 'PNG' does not match MIME type 'image/jpeg' in URI 
> 'data:image/jpeg;base64,iVBORw0KGgoAAA...AAAElFTkSuQmCC'
> 
> 
> Also, the javadoc of `javafx.scene.image.Image` incorrectly states:
> 
> 94* If a URL uses the "data" scheme, the data must be base64-encoded
> 95* and the MIME type must either be empty or a subtype of the
> 96* {@code image} type.
> 
> However, omitting the MIME type of a data URI is specified to imply 
> "text/plain" (RFC 2397, section 2). Since the `com.sun.javafx.util.DataURI` 
> class is implemented according to this specification, trying to load an image 
> without MIME type correctly fails with an `ImageStorageException`: 
> "Unexpected MIME type: text".
> 
> The solution is to fix the documentation:
> 
>   94* If a URL uses the "data" scheme, the data must be base64-encoded
> - 95* and the MIME type must either be empty or a subtype of the
> - 96* {@code image} type.
> + 95* and the MIME type must be a subtype of the {@code image} type.
> + 96* The MIME type must match the image format of the data contained 
> in
> + 97* the URL. In case of a mismatch between MIME type and image 
> format,
> + 98* the image will be loaded if the image format can be determined 
> by
> + 99* JavaFX, and a warning will be logged.

Michael Strauß has updated the pull request incrementally with one additional 
commit since the last revision:

  Added test for image format without signature

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/676/files
  - new: https://git.openjdk.java.net/jfx/pull/676/files/0167532b..4cb5678a

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=676=03
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=676=02-03

  Stats: 89 lines in 8 files changed: 51 ins; 3 del; 35 mod
  Patch: https://git.openjdk.java.net/jfx/pull/676.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/676/head:pull/676

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

Re: RFR: 8274274: Update JUnit to version 5.8.1 [v9]

[Michael Strauß]

On Mon, 22 Nov 2021 13:43:34 GMT, John Hendrikx  wrote:

>> I've added JUnit 5 as a test dependency and made sure that the JUnit 4 tests 
>> still work.  Also added a single JUnit 5 tests, and confirmed it works.
>> 
>> I've updated the Eclipse project file for the base module only.
>
> John Hendrikx has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Remove remaining references to old JUnit 4.8.2
>  - Upgrade to JUnit 4.13.2

The changes look good to me. The tests work as expected.

-

Marked as reviewed by mstrauss (Author).

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

Re: RFR: 8277572: ImageStorage should correctly handle MIME types for images encoded in data URIs [v2]

[Michael Strauß]

On Tue, 23 Nov 2021 15:33:49 GMT, Kevin Rushforth  wrote:

> This seems like an odd behavior to me, but if this is common practice among 
> browsers, then it seems OK for us to do the same. In this case, it should be 
> documented in the class docs of `Image`.

I've tested this with Chrome and Edge, and these browsers go even further. If 
the image format has a detectable signature, the image will be displayed 
regardless of the specified MIME type (even if it is not an `image` type). The 
MIME type only matters for image formats without signature, like SVG. In this 
case, the image will not be displayed if the MIME type is incorrect.

I think that a compromise works best for JavaFX. We probably don't want to 
allow plainly incorrect MIME types, but in cases where we know what the 
developer should have specified, a warning seems like a good nudge in the right 
direction.

-

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

Re: RFR: 8277572: ImageStorage should correctly handle MIME types for images encoded in data URIs [v3]

[Michael Strauß]

> `com.sun.javafx.iio.ImageStorage` currently ignores the MIME image subtype 
> specified for images encoded in data URIs. This should be improved as follows:
> 
> 1. If the specified image subtype is not supported, an exception will be 
> thrown.
> 2. If the specified image subtype is supported, but the data contained in the 
> URI is of a different (but also supported) image format, the image will be 
> loaded and a warning will be logged. For example, if the MIME type is 
> "image/jpeg", but the image data is PNG, the following warning will be 
> generated:
> 
> 
> Image format 'PNG' does not match MIME type 'image/jpeg' in URI 
> 'data:image/jpeg;base64,iVBORw0KGgoAAA...AAAElFTkSuQmCC'
> 
> 
> Also, the javadoc of `javafx.scene.image.Image` incorrectly states:
> 
> 94* If a URL uses the "data" scheme, the data must be base64-encoded
> 95* and the MIME type must either be empty or a subtype of the
> 96* {@code image} type.
> 
> However, omitting the MIME type of a data URI is specified to imply 
> "text/plain" (RFC 2397, section 2). Since the `com.sun.javafx.util.DataURI` 
> class is implemented according to this specification, trying to load an image 
> without MIME type correctly fails with an `ImageStorageException`: 
> "Unexpected MIME type: text".
> 
> The solution is to fix the documentation:
> 
>   94* If a URL uses the "data" scheme, the data must be base64-encoded
> + 95* and the MIME type must be a subtype of the {@code image} type.
> - 95* and the MIME type must either be empty or a subtype of the
> - 96* {@code image} type.

Michael Strauß has updated the pull request incrementally with one additional 
commit since the last revision:

  Added documentation for MIME/data mismatch

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/676/files
  - new: https://git.openjdk.java.net/jfx/pull/676/files/73f1bb13..0167532b

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=676=02
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=676=01-02

  Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jfx/pull/676.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/676/head:pull/676

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

Re: RFR: 8277572: ImageStorage should correctly handle MIME types for images encoded in data URIs [v2]

[Michael Strauß]

> `com.sun.javafx.iio.ImageStorage` currently ignores the MIME image subtype 
> specified for images encoded in data URIs. This should be improved as follows:
> 
> 1. If the specified image subtype is not supported, an exception will be 
> thrown.
> 2. If the specified image subtype is supported, but the data contained in the 
> URI is of a different (but also supported) image format, the image will be 
> loaded and a warning will be logged. For example, if the MIME type is 
> "image/jpeg", but the image data is PNG, the following warning will be 
> generated:
> 
> 
> Image format 'PNG' does not match MIME type 'image/jpeg' in URI 
> 'data:image/jpeg;base64,iVBORw0KGgoAAA...AAAElFTkSuQmCC'
> 
> 
> Also, the javadoc of `javafx.scene.image.Image` incorrectly states:
> 
> 94* If a URL uses the "data" scheme, the data must be base64-encoded
> 95* and the MIME type must either be empty or a subtype of the
> 96* {@code image} type.
> 
> However, omitting the MIME type of a data URI is specified to imply 
> "text/plain" (RFC 2397, section 2). Since the `com.sun.javafx.util.DataURI` 
> class is implemented according to this specification, trying to load an image 
> without MIME type correctly fails with an `ImageStorageException`: 
> "Unexpected MIME type: text".
> 
> The solution is to fix the documentation:
> 
>   94* If a URL uses the "data" scheme, the data must be base64-encoded
> + 95* and the MIME type must be a subtype of the {@code image} type.
> - 95* and the MIME type must either be empty or a subtype of the
> - 96* {@code image} type.

Michael Strauß has updated the pull request incrementally with one additional 
commit since the last revision:

  ImageStorage correctly handles MIME types in data URIs

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/676/files
  - new: https://git.openjdk.java.net/jfx/pull/676/files/4abb9b47..73f1bb13

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=676=01
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=676=00-01

  Stats: 152 lines in 9 files changed: 85 ins; 8 del; 59 mod
  Patch: https://git.openjdk.java.net/jfx/pull/676.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/676/head:pull/676

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

RFR: 8277572: Image class documentation incorrect for images encoded in data URIs

[Michael Strauß]

The javadoc of `javafx.scene.image.Image` incorrectly states:

94* If a URL uses the "data" scheme, the data must be base64-encoded
95* and the MIME type must either be empty or a subtype of the
96* {@code image} type.

However, omitting the MIME type of a data URI is specified to imply 
"text/plain" (RFC 2397, section 2). Since the `com.sun.javafx.util.DataURI` 
class is implemented according to this specification, trying to load an image 
without MIME type correctly fails with an `ImageStorageException`: "Unexpected 
MIME type: text".

The solution is to fix the documentation:

  94* If a URL uses the "data" scheme, the data must be base64-encoded
+ 95* and the MIME type must be a subtype of the {@code image} type.
- 95* and the MIME type must either be empty or a subtype of the
- 96* {@code image} type.

-

Commit messages:
 - Fixed incorrect documentation of image data URIs

Changes: https://git.openjdk.java.net/jfx/pull/676/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jfx=676=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8277572
  Stats: 32 lines in 2 files changed: 30 ins; 1 del; 1 mod
  Patch: https://git.openjdk.java.net/jfx/pull/676.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/676/head:pull/676

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

Re: RFR: 8197991: Selecting many items in a TableView is very slow

[Michael Strauß]

On Mon, 22 Nov 2021 15:55:03 GMT, Abhinay Agarwal  wrote:

>> the for-loop is certainly faster and would allocate less memory - i find the 
>> `for(int i = 0, max = size())`-style a bit odd
>
> I could move `int max = size();` outside the loop

But why? The initialization block of a `for` statement is exactly where you'd 
put loop-scoped variables.

-

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

Re: RFR: 8274274: Update JUnit to version 5.8.1 [v5]

[Michael Strauß]

On Thu, 11 Nov 2021 15:18:25 GMT, Kevin Rushforth  wrote:

> I left a few comments on the dependencies. Will review / test the PR later.
> 
> One comment about adding new JUnit 5 tests and migrating existing tests. I 
> think there could be value in organizing the tests such that all of the JUnit 
> 5 tests are grouped, rather than mixing tests in the same directory such that 
> some use JUnit 5 and others use JUnit 4. What do you (or others) think? We 
> could either do this with a new JUnit 5 source set in each project, or by 
> using a package naming convention for JUnit 5 tests like we do for robot 
> tests. Maybe `test5.some.pkg`. This needs more thought.

I don't like the idea of separating source sets or packages just based on the 
version of our unit test framework. In my opinion, sources should be organized 
by function, and not based on a technicality. A very common case we'll 
encounter will be adding a new unit test to an existing test class. What's our 
guidance in this case? If we continue to use JUnit4 for new unit tests, that 
means we'll effectively persist this version of the framework forever in the 
test sources. Another option would be to migrate the entire test class once 
it's updated with new code. However, that would be a significant amount of work 
required from the author of a new unit test. A third option would be to 
actually migrate all unit test classes as a series of refactoring PRs.

-

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

Re: RFR: 8274022 Additional Memory Leak in ControlAcceleratorSupport [v5]

[Michael Strauß]

On Mon, 15 Nov 2021 08:24:04 GMT, Florian Kirmaier  
wrote:

>> This fixes the new ControlAcceleratorBug which was Introduced in JavaFX17.
>> To fix it, I've made the Value of the WeakHashMap also weak. 
>> We only keep this value to remove it as a listener later on. Therefore there 
>> shouldn't be issues by making this value weak.
>> 
>> 
>> I've seen this Bug very very often, in the last weeks. Most of the 
>> applications I've seen are somehow affected by this bug.
>> 
>> It basically **breaks every application with menu bars and multiple stages** 
>> - which is the majority of enterprise applications. It's especially annoying 
>> because it takes some time until the application gets unstable.
>> 
>> Therefore **I would recommend** after this fix is approved, **to make a new 
>> version for JavaFX17** with this fix because this bug is so severe.
>
> Florian Kirmaier has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8274022
>   the logic to remove the keys from the map, is now more consistent

Looks good to me.

-

Marked as reviewed by mstrauss (Author).

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

Re: RFR: 8268225: Support :focus-visible and :focus-within CSS pseudoclasses [v7]

[Michael Strauß]

On Thu, 9 Sep 2021 09:51:33 GMT, Jeanette Winzenburg  
wrote:

>>> Just curious: with this in place, would it be possible to use for 
>>> supporting [JDK-8087926](https://bugs.openjdk.java.net/browse/JDK-8087926) 
>>> (it's a bit vague, though, at least for me)?
>> 
>> Yes, `:focus-within` can be used to select an ancestor of the 
>> currently-focused node.
>
>> 
>> 
>> > Just curious: with this in place, would it be possible to use for 
>> > supporting [JDK-8087926](https://bugs.openjdk.java.net/browse/JDK-8087926) 
>> > (it's a bit vague, though, at least for me)?
>> 
>> Yes, `:focus-within` can be used to select an ancestor of the 
>> currently-focused node.
> 
> cool - thanks :)

@kleopatra Can you be a reviewer on this PR?

-

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

Re: RFR: 8264591: HBox/VBox child widths pixel-snap to wrong value

[Michael Strauß]

On Wed, 22 Sep 2021 18:48:13 GMT, Marius Hanl  wrote:

>> The children of HBox/VBox don't always pixel-snap to the same value as the 
>> container itself when a render scale other than 1 is used. This can lead to 
>> a visual glitch where the content bounds don't line up with the container 
>> bounds. In this case, the container will extend beyond its content, or vice 
>> versa.
>> 
>> The following program shows the problem for HBox:
>> 
>> Region r1 = new Region();
>> Region r2 = new Region();
>> Region r3 = new Region();
>> Region r4 = new Region();
>> Region r5 = new Region();
>> Region r6 = new Region();
>> r1.setBackground(new Background(new BackgroundFill(Color.GREY, null, null)));
>> r2.setBackground(new Background(new BackgroundFill(Color.DARKGRAY, null, 
>> null)));
>> r3.setBackground(new Background(new BackgroundFill(Color.BLACK, null, 
>> null)));
>> r4.setBackground(new Background(new BackgroundFill(Color.GREY, null, null)));
>> r5.setBackground(new Background(new BackgroundFill(Color.DARKGRAY, null, 
>> null)));
>> r6.setBackground(new Background(new BackgroundFill(Color.BLACK, null, 
>> null)));
>> r1.setPrefWidth(25.3);
>> r2.setPrefWidth(25.3);
>> r3.setPrefWidth(25.4);
>> r4.setPrefWidth(25.3);
>> r5.setPrefWidth(25.3);
>> r6.setPrefWidth(25.4);
>> r1.setMaxHeight(30);
>> r2.setMaxHeight(30);
>> r3.setMaxHeight(30);
>> r4.setMaxHeight(30);
>> r5.setMaxHeight(30);
>> r6.setMaxHeight(30);
>> HBox hbox1 = new HBox(r1, r2, r3, r4, r5, r6);
>> hbox1.setBackground(new Background(new BackgroundFill(Color.RED, null, 
>> null)));
>> hbox1.setPrefHeight(40);
>> 
>> r1 = new Region();
>> r2 = new Region();
>> r3 = new Region();
>> r4 = new Region();
>> r5 = new Region();
>> r6 = new Region();
>> r1.setBackground(new Background(new BackgroundFill(Color.GREY, null, null)));
>> r2.setBackground(new Background(new BackgroundFill(Color.DARKGRAY, null, 
>> null)));
>> r3.setBackground(new Background(new BackgroundFill(Color.BLACK, null, 
>> null)));
>> r4.setBackground(new Background(new BackgroundFill(Color.GREY, null, null)));
>> r5.setBackground(new Background(new BackgroundFill(Color.DARKGRAY, null, 
>> null)));
>> r6.setBackground(new Background(new BackgroundFill(Color.BLACK, null, 
>> null)));
>> r1.setPrefWidth(25.3);
>> r2.setPrefWidth(25.3);
>> r3.setPrefWidth(25.4);
>> r4.setPrefWidth(25.3);
>> r5.setPrefWidth(25.3);
>> r6.setPrefWidth(25.4);
>> r1.setMaxHeight(30);
>> r2.setMaxHeight(30);
>> r3.setMaxHeight(30);
>> r4.setMaxHeight(30);
>> r5.setMaxHeight(30);
>> r6.setMaxHeight(30);
>> HBox hbox2 = new HBox(r1, r2, r3, r4, r5, r6);
>> hbox2.setBackground(new Background(new BackgroundFill(Color.RED, null, 
>> null)));
>> hbox2.setPrefHeight(40);
>> hbox2.setPrefWidth(152);
>> 
>> VBox root = new VBox(new HBox(hbox1), new HBox(hbox2));
>> root.setSpacing(20);
>> Scene scene = new Scene(root, 500, 250);
>> 
>> primaryStage.setScene(scene);
>> primaryStage.show();
>> 
>> 
>> Here's a before-and-after comparison of the program above after applying the 
>> fix. Note that 'before', the backgrounds of the container (red) and its 
>> content (black) don't align perfectly. The render scale is 175% in this 
>> example.
>> ![pixel-glitch](https://user-images.githubusercontent.com/43553916/112891996-146e2d80-90d9-11eb-9d83-97754ae38dc1.png)
>> 
>> I've filed a bug report and will change the title of the GitHub issue as 
>> soon as it's posted.
>
> I had a look at the PR. But it took quite a while to fully understand the 
> changes (also the current implementation ). 
> I think it make sense to improve it a bit e.g. by adding javadoc to the new 
> methods, maybe also the existing? Other ideas are also welcome. 
> With that said maybe more people will review it then. I will have a full look 
> soon as well. :)

@Maran23 Would you be interested in reviewing the implementation and the added 
documentation?

-

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

Re: Bidirectional binding enhancement

[Michael Strauß]

If you're interested in the implementation, please have a look at the
draft PR: https://github.com/openjdk/jfx/pull/663

This feature is implemented on the FX beans level in javafx.base, so
there's no need to have access to javafx.graphics.

On Wed, Nov 10, 2021 at 8:52 AM Tom Schindl  wrote:
>
> Hi,
>
> We had something very similar in Eclipse-Databinding so I think
> something like that makes a lot of sense but I wonder how you want to
> implement this FOCUS, ACTION.
>
> Another thing we had was a delayed-Observable where the sync only
> happened if there hasn't been a change with a user specified timeout
> which fairly nice to implement undo/redo like stuff eg in TextAreas.
>
> As you don't have access to Node in javafx.base I'm not sure how you
> want to implement the trigger stuff. Just in case in Eclipse-Databinding
> world we had stuff like this in a module (in this case OSGi-Bundle) who
> has access to both the core-API and the ui-API.
>
> Tom

Re: Bidirectional binding enhancement

[Michael Strauß]

On Wed, Nov 10, 2021 at 12:12 PM John Hendrikx  wrote:
>
> Although I think you have a valid use case, I don't think JavaFX should
> facilitate this exact scenario; it is a high level concern that you want
> to solve in a very low level mechanism. A similar scenario also applies
> to uni-directional bindings, so I think it would have to apply there as
> well.

I don't think it would have to apply for unidirectional bindings, as
it is exceedingly rare that you would want to unidirectionally bind
the value of a model-layer property to a view-layer property, but not
vice versa. After all, you'd probably want your input controls to
reflect the initial state of the model.
In any way, it sure is a kind-of high level concern, but in my
experience, it's a very common problem at least for some software
architectures without a great solution.


> It also really depends on how you are doing the communication between
> view and model. Some system use models that are always valid, some
> systems use a view-model that contains a direct copy of what is in the
> UI controls. Some views allow users to type anything and do validation
> on submission or focus loss; some do per character validation and mark
> bad input; some will not even allow you to type bad input.
>
> For a scenario like you describe, which seems to be about delayed
> updates of bindings, I think you really want to use something like
> ReactFX's EventStreams.  These offer far more features, including
> timeouts, removal of duplicates, combining of values, conditional
> suspending, etc.  It might work like this for example:
>
>  EventStream.of(textInput.textProperty())
>  .conditionOn(textInput.focusedProperty().not())
>  .feedTo(model::valueProperty);
>
> Or with Val:
>
>  Val.of(textInput.textProperty())
>  .conditionOn(textInput.focusedProperty().not())
>  .subcribe(v -> updateModel(v));
>
> (Note: conditionOn is part of the fluent bindings proposal that Nir
> Lisker and me have been working on).

Well, that requires you to pull in another third-party dependency, and
even then you would need to roll your own bidirectional binding
implementation. It might work for the focus-lost behavior, but not for
the ActionEvent behavior. Of course, you can do all of that, but this
doesn't seem to me like a great value proposition for solving a very
common problem.


> Now, this isn't bidirectional, but I don't see how that will work in any
> case as there are some edge cases. For example, how would you handle a
> model update when the view is currently being edited? Delaying updates
> runs into issues where both may have changed, whereas currently bindings
> are resolved immediately on the FX thread.

There are no additional edge cases compared to vanilla bidirectional
bindings. Bidirectional bindings with an UpdateSourceTrigger will have
exactly the same concurrency behavior as vanilla bidirectional
bindings (i.e. they don't support concurrent modification at all). I'm
not sure how the FX thread relates to this, as bidirectional bindings
don't know about threads in any case.

Note that UpdateSourceTrigger only applies to the target->source
direction. The source->target direction, which is the scenario you
describe when the model is updated while the view is being edited, is
exactly the same as it is for vanilla bidirectional bindings.

Re: RFR: 8274929: Crash while reading specific clipboard content [v2]

[Michael Strauß]

On Wed, 10 Nov 2021 12:46:08 GMT, Kevin Rushforth  wrote:

>> This bug is caused by not sanity checking the data returned by a call to the 
>> Windows Clipboard `IDataObject::GetData` method. When requesting a file 
>> descriptor with a format of either `CFSTR_FILEDESCRIPTORA` or 
>> `CFSTR_FILEDESCRIPTORW`, which returns a list of file names, the first word 
>> of the returned data buffer is supposed to be the number of items that 
>> follow. Applications can put data on the clipboard in such a way that it 
>> will respond to a request to return the list of files from the clipboard 
>> with data that isn't formatted correctly, so we can't assume that the first 
>> word is a valid count.
>> 
>> The fix is to check the returned buffer size against the item count. I added 
>> a regression test that fails before and passes after the fix.
>
> Kevin Rushforth has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Update check to test that bufferSize is exactly the right size

Marked as reviewed by mstrauss (Author).

-

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

Bidirectional binding enhancement

[Michael Strauß]

JavaFX developers routinely use programming patterns like MVC, MVP, or
MVVM to separate views from their associated business logic. Bindings
can be used to connect the values of UI controls (like Label or
TextField) to properties on a business logic class.

A typical (simplified) scenario may look like this:

var valueField = new TextField();
valueField.textProperty().bindBidirectional(businessLogic.valueProperty());

The business logic class may perform data validation or other actions
on the value that was entered in the TextField. However, in many
cases, it is neither necessary nor desirable for the binding to update
the business-layer property on every single change (i.e. every single
character that was typed by a user). For example, if a business rule
verifies that the data entered by a user is formatted in a specific
way, it's usually not a great experience to yield a validation error
before the user has finished typing. Instead, it's often better to
wait until the user has significantly interacted with a UI control
before running business logic.

For this reason, I propose to add a new type of binding to the
javafx.beans.binding.Bindings class:

void bindBidirectional(Property target, Property source,
UpdateSourceTrigger trigger)

UpdateSourceTrigger is an enumeration that allows developers to
specify the condition on which changes of the target property will
update the source property. Its values are:

DEFAULT: Updates the source property on every change (this is the
default behavior of bidirectional bindings).
FOCUS: Updates the source property when the UI control loses input focus.
ACTION: Updates the source property when the UI control loses input
focus or when it receives an ActionEvent (in the case of TextField,
this corresponds to the ENTER key).

Note that this setting only applies to changes of the target property.
When the source property is changed instead, the target property is
always immediately updated.

Any feedback on this proposal is appreciated.

Re: RFR: 8274929: Crash while reading specific clipboard content

[Michael Strauß]

On Wed, 10 Nov 2021 00:31:05 GMT, Kevin Rushforth  wrote:

>> modules/javafx.graphics/src/main/native-glass/win/GlassClipboard.cpp line 
>> 1307:
>> 
>>> 1305: jsize bufferSize = me.size() - sizeof(UINT);
>>> 1306: if ((pdata->cItems > 0) &&
>>> 1307: (bufferSize / pdata->cItems >= itemSize))
>> 
>> Instead of discarding all the data, have you considered reading 
>> `min(pdata->cItems, bufferSize / itemSize)` items?
>
> I thought about it, but since failing this test means that `cItems` is 
> invalid, there is no reason to believe that the data that follows it is any 
> less invalid.

Then shouldn't we also not trust the data if `bufferSize` is larger than it 
needs to be? The documentation of `FILEGROUPDESCRIPTORA/W` says that `cItems` 
should correspond exactly to the numer of items in the array that follows.

-

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

Re: RFR: 8274929: Crash while reading specific clipboard content

[Michael Strauß]

On Tue, 9 Nov 2021 16:57:58 GMT, Kevin Rushforth  wrote:

> This bug is caused by not sanity checking the data returned by a call to the 
> Windows Clipboard `IDataObject::GetData` method. When requesting a file 
> descriptor with a format of either `CFSTR_FILEDESCRIPTORA` or 
> `CFSTR_FILEDESCRIPTORW`, which returns a list of file names, the first word 
> of the returned data buffer is supposed to be the number of items that 
> follow. Applications can put data on the clipboard in such a way that it will 
> respond to a request to return the list of files from the clipboard with data 
> that isn't formatted correctly, so we can't assume that the first word is a 
> valid count.
> 
> The fix is to check the returned buffer size against the item count. I added 
> a regression test that fails before and passes after the fix.

modules/javafx.graphics/src/main/native-glass/win/GlassClipboard.cpp line 1307:

> 1305: jsize bufferSize = me.size() - sizeof(UINT);
> 1306: if ((pdata->cItems > 0) &&
> 1307: (bufferSize / pdata->cItems >= itemSize))

Instead of discarding all the data, have you considered reading 
`min(pdata->cItems, bufferSize / itemSize)` items?

-

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

Integrated: 8274854: Mnemonics for menu containing numeric text not working

[Michael Strauß]

On Wed, 20 Oct 2021 16:54:35 GMT, Michael Strauß  wrote:

> This PR fixes an issue with mnemonic parsing by removing the restriction that 
> a mnemonic symbol must be a letter. Now, it can be any character except 
> whitespace.

This pull request has now been integrated.

Changeset: 6749ab60
Author:    Michael Strauß 
Committer: Kevin Rushforth 
URL:   
https://git.openjdk.java.net/jfx/commit/6749ab60b7673a0838d55fbd09cabf4232d5da60
Stats: 194 lines in 3 files changed: 162 ins; 22 del; 10 mod

8274854: Mnemonics for menu containing numeric text not working

Reviewed-by: aghaisas, kcr

-

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

Circling back to a new layout algorithm

[Michael Strauß]

I previously proposed a new iterative layout algorithm [1] that
supports baseline alignment and introduces new APIs to give developers
control over the way nodes are aligned. This is a solution to the
long-standing problem that JavaFX cannot reliably lay out nodes that
are aligned on their baseline [2]. The new layout algorithm might also
fix some issues where the scene graph layout only settles after
interacting with the controls (for example, by clicking).

I've created a small application that shows the new APIs and a
correctly working baseline-aligned layout [3]. In addition to that, I
also built SceneBuilder with both the old and new layout system, and
played around with it to find out whether there were any regressions
or visual differences. So far, I haven't found any.

In order to move this forward, I think it would be a good idea to test
the latest version of the new layout system in more real-world JavaFX
applications. Any help from JavaFX application developers is greatly
appreciated. It's as easy as checking out the JavaFX sources from the
PR [1], building a local SDK and linking your application with the
binaries.


Finally, here's a high-level overview of the new algorithm:

When Parent::layout() is called on a layout root (i.e. a scene root or
an unmanaged node), it will lay out its children in a loop until the
scene graph under the layout root is fully laid out, which means it is
clean and doesn't require further layout. The totality of layout
activity for a single layout root is called "layout cycle". A layout
cycle will often take a few layout passes to finish (but not more than
2 in most cases). There is no limit on how often Parent::layout() will
iterate to lay out its children, so in principle, this could lead to
an infinite layout loop.

One source of infinite layout loops are incorrectly implemented controls:

class PathologicalControl extends Region {
final Text text = new Text("foo");

PathologicalControl() {
getChildren().add(text);
}

@Override
protected void layoutChildren() {
text.relocate(0, text.getLayoutY() + 10);
}
}

In this example, each call to layoutChildren() moves down the text
node another 10 pixels from where it was, which causes the layout
algorithm to schedule yet another layout pass. It's an infinite loop.

The layout system detects this by tracking how often a node
invalidates the scene graph in a single layout cycle. If a node
exceeds a threshold of 100 invalidations, it will be suspended from
layout and can no longer invalidate the scene graph in the current
layout cycle. A warning will be logged to notify developers of that
fact.


[1] https://github.com/openjdk/jfx/pull/433
[2] https://bugs.openjdk.java.net/browse/JDK-8090261
[3] https://github.com/mstr2/jfx-layout-sample

Re: RFR: 8089589: [ListView] ScrollBar content moves toward-backward during scrolling. [v4]

[Michael Strauß]

On Fri, 16 Apr 2021 10:45:18 GMT, Johan Vos  wrote:

>> This PR introduces a refactory for VirtualFlow, fixing a number of issues 
>> reported about inconsistent scrolling speed (see 
>> https://bugs.openjdk.java.net/browse/JDK-8089589)
>> The problem mentioned in the JBS issue (and in related issues) is that the 
>> VirtualFlow implementation depends on cell index and cell count, instead of 
>> on pixel count. The latter is unknown when the VirtualFlow is created, and 
>> pre-calculating the size of a large set of items would be very expensive.
>> Therefore, this PR uses a combination of a gradual calculation of the total 
>> size in pixels (estimatedSize) and a smoothing part that prevents the 
>> scrollback to scroll in the reverse direction as the requested change.
>> This PR currently breaks a number of tests that hard-coded depend on a 
>> number of evaluations. This is inherit to the approach of this PR: if we 
>> want to estimate the total size, we need to do some additional calculations. 
>> In this PR, I try to balance between consistent behavior and performance.
>
> Johan Vos has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Process reviewer comments, uncomment commented test, remove stderr

modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
 line 926:

> 924: // the absolute offset is changed accordingly.
> 925: adjustAbsoluteOffset();
> 926: }

This seems to violate the invariant `setPosition(...)` ≙ 
`positionProperty().set(...)`.

Your comment indicates that this is by design, but how do you prevent people 
from invoking `positionProperty().set(...)` and getting a different behavior?

-

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

Re: RFR: 8264591: HBox/VBox child widths pixel-snap to wrong value

[Michael Strauß]

On Wed, 22 Sep 2021 18:48:13 GMT, Marius Hanl  wrote:

> I had a look at the PR. But it took quite a while to fully understand the 
> changes (also the current implementation ). I think it make sense to improve 
> it a bit e.g. by adding javadoc to the new methods, maybe also the existing? 
> Other ideas are also welcome. With that said maybe more people will review it 
> then. I will have a full look soon as well. :)

I've added a bit of documentation.

-

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

Re: RFR: 8264591: HBox/VBox child widths pixel-snap to wrong value [v2]

[Michael Strauß]

> The children of HBox/VBox don't always pixel-snap to the same value as the 
> container itself when a render scale other than 1 is used. This can lead to a 
> visual glitch where the content bounds don't line up with the container 
> bounds. In this case, the container will extend beyond its content, or vice 
> versa.
> 
> The following program shows the problem for HBox:
> 
> Region r1 = new Region();
> Region r2 = new Region();
> Region r3 = new Region();
> Region r4 = new Region();
> Region r5 = new Region();
> Region r6 = new Region();
> r1.setBackground(new Background(new BackgroundFill(Color.GREY, null, null)));
> r2.setBackground(new Background(new BackgroundFill(Color.DARKGRAY, null, 
> null)));
> r3.setBackground(new Background(new BackgroundFill(Color.BLACK, null, null)));
> r4.setBackground(new Background(new BackgroundFill(Color.GREY, null, null)));
> r5.setBackground(new Background(new BackgroundFill(Color.DARKGRAY, null, 
> null)));
> r6.setBackground(new Background(new BackgroundFill(Color.BLACK, null, null)));
> r1.setPrefWidth(25.3);
> r2.setPrefWidth(25.3);
> r3.setPrefWidth(25.4);
> r4.setPrefWidth(25.3);
> r5.setPrefWidth(25.3);
> r6.setPrefWidth(25.4);
> r1.setMaxHeight(30);
> r2.setMaxHeight(30);
> r3.setMaxHeight(30);
> r4.setMaxHeight(30);
> r5.setMaxHeight(30);
> r6.setMaxHeight(30);
> HBox hbox1 = new HBox(r1, r2, r3, r4, r5, r6);
> hbox1.setBackground(new Background(new BackgroundFill(Color.RED, null, 
> null)));
> hbox1.setPrefHeight(40);
> 
> r1 = new Region();
> r2 = new Region();
> r3 = new Region();
> r4 = new Region();
> r5 = new Region();
> r6 = new Region();
> r1.setBackground(new Background(new BackgroundFill(Color.GREY, null, null)));
> r2.setBackground(new Background(new BackgroundFill(Color.DARKGRAY, null, 
> null)));
> r3.setBackground(new Background(new BackgroundFill(Color.BLACK, null, null)));
> r4.setBackground(new Background(new BackgroundFill(Color.GREY, null, null)));
> r5.setBackground(new Background(new BackgroundFill(Color.DARKGRAY, null, 
> null)));
> r6.setBackground(new Background(new BackgroundFill(Color.BLACK, null, null)));
> r1.setPrefWidth(25.3);
> r2.setPrefWidth(25.3);
> r3.setPrefWidth(25.4);
> r4.setPrefWidth(25.3);
> r5.setPrefWidth(25.3);
> r6.setPrefWidth(25.4);
> r1.setMaxHeight(30);
> r2.setMaxHeight(30);
> r3.setMaxHeight(30);
> r4.setMaxHeight(30);
> r5.setMaxHeight(30);
> r6.setMaxHeight(30);
> HBox hbox2 = new HBox(r1, r2, r3, r4, r5, r6);
> hbox2.setBackground(new Background(new BackgroundFill(Color.RED, null, 
> null)));
> hbox2.setPrefHeight(40);
> hbox2.setPrefWidth(152);
> 
> VBox root = new VBox(new HBox(hbox1), new HBox(hbox2));
> root.setSpacing(20);
> Scene scene = new Scene(root, 500, 250);
> 
> primaryStage.setScene(scene);
> primaryStage.show();
> 
> 
> Here's a before-and-after comparison of the program above after applying the 
> fix. Note that 'before', the backgrounds of the container (red) and its 
> content (black) don't align perfectly. The render scale is 175% in this 
> example.
> ![pixel-glitch](https://user-images.githubusercontent.com/43553916/112891996-146e2d80-90d9-11eb-9d83-97754ae38dc1.png)
> 
> I've filed a bug report and will change the title of the GitHub issue as soon 
> as it's posted.

Michael Strauß has updated the pull request with a new target base due to a 
merge or a rebase. The incremental webrev excludes the unrelated changes 
brought in by the merge/rebase. The pull request contains four additional 
commits since the last revision:

 - added documentation, improved method names
 - Merge branch 'master' into fixes/box-snap-to-pixel
 - Fix pixel-snapping glitches in VBox/HBox
 - Failing test

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/445/files
  - new: https://git.openjdk.java.net/jfx/pull/445/files/c218b809..744f19f5

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=445=01
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=445=00-01

  Stats: 449813 lines in 8109 files changed: 251439 ins; 126257 del; 72117 mod
  Patch: https://git.openjdk.java.net/jfx/pull/445.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/445/head:pull/445

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

RFR: 8276313: ScrollPane scroll delta incorrectly depends on content height

[Michael Strauß]

This PR fixes an issue where the scroll delta of ScrollPane incorrectly depends 
on the size of its content.
This leads to extremely slow scrolling when the content is only slightly larger 
than the ScrollPane.

-

Commit messages:
 - Fixed scrolling speed for ScrollPaneSkin
 - failing test

Changes: https://git.openjdk.java.net/jfx/pull/660/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jfx=660=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8276313
  Stats: 52 lines in 2 files changed: 49 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jfx/pull/660.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/660/head:pull/660

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

Re: RFR: 8274022 fixing critical memory leak in the ControlAcceleratorSupport [v3]

[Michael Strauß]

On Tue, 2 Nov 2021 08:16:41 GMT, Florian Kirmaier  wrote:

>> This fixes the new ControlAcceleratorBug which was Introduced in JavaFX17.
>> To fix it, I've made the Value of the WeakHashMap also weak. 
>> We only keep this value to remove it as a listener later on. Therefore there 
>> shouldn't be issues by making this value weak.
>> 
>> 
>> I've seen this Bug very very often, in the last weeks. Most of the 
>> applications I've seen are somehow affected by this bug.
>> 
>> It basically **breaks every application with menu bars and multiple stages** 
>> - which is the majority of enterprise applications. It's especially annoying 
>> because it takes some time until the application gets unstable.
>> 
>> Therefore **I would recommend** after this fix is approved, **to make a new 
>> version for JavaFX17** with this fix because this bug is so severe.
>
> Florian Kirmaier has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8274022
>   Simplified code related to WeakHashMaps

Marked as reviewed by mstrauss (Author).

-

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

Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v2]

[Michael Strauß]

On Wed, 27 Oct 2021 09:56:46 GMT, Jeanette Winzenburg  
wrote:

>> Cleanup of Tree-/TableRowSkin to support switching skins
>> 
>> The misbehavior/s
>> - memory leaks due to manually registered listeners that were not removed
>> - side-effects due to listeners still active on old skin (like NPEs)
>> 
>> Fix
>> - use skin api for all listener registration (for automatic removal in 
>> dispose)
>> - do not install listeners that are not needed (fixedCellSize, same as in 
>> fix of ListCellSkin 
>> [JDK-8246745](https://bugs.openjdk.java.net/browse/JDK-8246745))
>> 
>> Added tests for each listener involved in the fix to guarantee it's still 
>> working and does not have unwanted side-effects after switching skins.
>> 
>> Note: there are pecularities in row skins (like not updating themselves on 
>> property changes of its control, throwing NPEs when not added to a 
>> VirtualFlow) which are not part of this issue but covered in 
>> [JDK-8274065](https://bugs.openjdk.java.net/browse/JDK-8274065)
>
> Jeanette Winzenburg has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   re-added forgotten code comments

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java
 line 299:

> 297: @Override protected ObjectProperty graphicProperty() {
> 298: TreeTableRow treeTableRow = getSkinnable();
> 299: // FIXME: illegal access if skinnable is null

What is the purpose of removing the null check, but leaving getSkinnable() in 
there?
Given the signature of the method, it seems that it shouldn't ever return 
`null` in any case.

-

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

Re: RFR: 8201538: Remove implementation support for applets from JavaFX

[Michael Strauß]

On Tue, 31 Aug 2021 16:28:53 GMT, Kevin Rushforth  wrote:

> This PR removes the obsolete applet implementation from JavaFX. It is an 
> ongoing maintenance burden to carry around this legacy code. Also, cleaning 
> this up could help in the implementation of GTK4, Wayland, and Metal, since 
> we won't have to account for the way applet windows are created and managed.
> 
> ## Notes to reviewers:
> 
> The first part of the removal was to eliminate the methods and classes on the 
> Java side that are associated with creating and managing an applet window, 
> and which are no longer called. After these were removed, I then removed the 
> corresponding methods and classes on the native side that are no longer 
> called.
> 
> ### Shared Code
> 
> The following were removed from the shared code.
> 
>  Removed Java classes
> 
> 
> com.sun.javafx.tk.AppletWindow
> com.sun.javafx.tk.quantum.GlassAppletWindow
> 
> 
>  Removed methods
> 
> The following methods were removed in the parent class and all subclasses.
> 
> 
> com.sun.glass.ui.Application:
> public abstract Window createWindow(long parent)
> 
> com.sun.glass.ui.Window:
> public boolean getAppletMode()
> public void setAppletMode(boolean appletMode)
> public void dispatchNpapiEvent(Map eventInfo)
> protected abstract long _createChildWindow(long parent)
> protected Window(long parent)
> protected abstract int _getEmbeddedX(long ptr)
> protected abstract int _getEmbeddedY(long ptr)
> 
> com.sun.javafx.tk.Toolkit:
> public abstract AppletWindow createAppletWindow(...)
> public abstract void closeAppletWindow()
> 
> com.sun.javafx.tk.quantum.WindowStage:
> static void setAppletWindow(GlassAppletWindow aw)
> static GlassAppletWindow getAppletWindow()
> 
> 
> 
> ### Linux (Gtk) Java code
> 
> The following classes or methods were removed:
> 
> 
> com.sun.glass.ui.gtk.GtkChildWindow (class removed)
> 
> com.sun.glass.ui.gtk.GtkWindow:
> protected GtkWindow(long parent)
> 
> 
> ### Linux (Gtk) native glass:
> 
> The following native classes were removed:
> 
> 
> WindowContextChild
> WindowContextPlug
> 
> 
> ### macOS Java code
> 
> The following classes or methods were removed:
> 
> 
> com.sun.glass.events.mac.NpapiEvent (class removed)
> 
> com.sun.glass.ui.mac.MacApplication:
> native protected String _getRemoteLayerServerName()
> 
> com.sun.glass.ui.View:
> public int getNativeRemoteLayerId(String serverName)
> 
> com.sun.glass.ui.mac.MacView:
> native protected int _getNativeRemoteLayerId(long ptr, String serverName)
> native protected void _hostRemoteLayerId(long ptr, int nativeLayerId)
> 
> com.sun.glass.ui.mac.MacWindow:
> protected MacWindow(long parent)
> 
> 
> ### macOS native code
> 
> The following native classes were removed:
> 
> 
> GlassEmbeddedWindow*
> GlassNSEvent
> GlassView3D+Remote
> RemoteLayerSupport
> 
> 
> I also removed the `jIsChild` parameter from the window creation code which 
> allowed for removing a lot of dead blocks of code. The main window creation 
> method was:
> 
> 
> - (id)_initWithContentRect:(NSRect)contentRect 
> styleMask:(NSUInteger)windowStyle
> screen:(NSScreen *)screen jwindow:(jobject)jwindow 
> jIsChild:(jboolean)jIsChild
> 
> 
> This created a `GlassEmbeddedWindow` iff `jIsChild == JNI_TRUE`. Since 
> `jIsChild` was only set to true by the (now removed) 
> `_createChildWindow(long)` method, we can remove the parameter, the 
> `GlassEmbeddedWindow*` classes, and all code blocks that are qualified by `if 
> (jIsChild)`.
> 
> ### Windows Java code 
> 
> The following classes or methods were removed:
> 
> 
> com.sun.glass.ui.win.WinChildWindow (class removed)
> 
> com.sun.glass.ui.win.WinWindow:
> protected WinWindow(long parent)
> 
> 
> ### Windows native code
> 
> After removing all references to `IsChild()`, which was only ever true for 
> `_createChildWindow()`, we can also remove the following:
> 
> 
> GlassApplication::InstallMouseLLHook
> GlassApplication::UninstallMouseLLHook
> 
> 
> ### iOS Java code
> 
> 
> com.sun.glass.ui.ios.IosWindow:
> protected IosWindow(long parent)
> 
> 
> ### iOS native code
> 
> With the removal of the `_createChildWindow` method, the following JNI method 
> in `IosWindow` can be removed:
> 
> 
> Java_com_sun_glass_ui_ios_IosWindow__1createChildWindow(JNIEnv *, jobject, 
> jlong)
> 
> 
> As a note, I don't have a setup to build this. It is a simple, safe change, 
> but should be double-checked by someone from Gluon.

This PR does what I would expect it to do. I tested it by building an 
application on all three desktop platforms and it works as usual.

-

Marked as reviewed by mstrauss (Author).

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

Re: RFR: 8274022 fixing critical memory leak in the ControlAcceleratorSupport

[Michael Strauß]

On Sat, 30 Oct 2021 10:56:40 GMT, Florian Kirmaier  
wrote:

> This fixes the new ControlAcceleratorBug which was Introduced in JavaFX17.
> To fix it, I've made the Value of the WeakHashMap also weak. 
> We only keep this value to remove it as a listener later on. Therefore there 
> shouldn't be issues by making this value weak.
> 
> 
> I've seen this Bug very very often, in the last weeks. Most of the 
> applications I've seen are somehow affected by this bug.
> 
> It basically **breaks every application with menu bars and multiple stages** 
> - which is the majority of enterprise applications. It's especially annoying 
> because it takes some time until the application gets unstable.
> 
> Therefore **I would recommend** after this fix is approved, **to make a new 
> version for JavaFX17** with this fix because this bug is so severe.

modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java
 line 31:

> 29: import javafx.beans.property.ReadOnlyObjectProperty;
> 30: import javafx.beans.value.ChangeListener;
> 31:  import javafx.beans.value.WeakChangeListener;

Minor: whitespace at the beginning of the line

modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java
 line 88:

> 86: // Remove previously added listener if any
> 87: if (sceneChangeListenerMap.containsKey(anchor)) {
> 88: ChangeListener listener = 
> sceneChangeListenerMap.get(anchor).get();

It seems unusual to check for the existence of a weak key (containsKey), and 
then just assume it still exists (get). You should probably get() the value 
directly without checking containsKey() first, and then check whether the 
returned value is null.

modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java
 line 89:

> 87: if (sceneChangeListenerMap.containsKey(anchor)) {
> 88: ChangeListener listener = 
> sceneChangeListenerMap.get(anchor).get();
> 89: if(listener != null) {

Minor: space before brace

modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java
 line 254:

> 252: // at the time of installing the accelerators.
> 253: if (sceneChangeListenerMap.containsKey(anchor)) {
> 254: ChangeListener listener = 
> sceneChangeListenerMap.get(anchor).get();

It seems unusual to check for the existence of a weak key (containsKey), and 
then just assume it still exists (get). You should probably get() the value 
directly without checking containsKey() first, and then check whether the 
returned value is null.

modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java
 line 255:

> 253: if (sceneChangeListenerMap.containsKey(anchor)) {
> 254: ChangeListener listener = 
> sceneChangeListenerMap.get(anchor).get();
> 255: if(listener != null) {

Minor: space before brace

-

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

Re: RFR: 8222455: JavaFX error loading glass.dll from cache

[Michael Strauß]

On Sat, 30 Oct 2021 14:54:16 GMT, Johan Vos  wrote:

> I don't think that would be needed. We currently use the same approach as 
> maven, and have a "permanent" cache for artifacts.

For users, JavaFX comes as an invisible part of an application in 99% of cases. 
That's different from Maven, which is consciously installed by users either 
manually or as part of a software development suite.
Most users of a JavaFX application did not consent to permanent changes to 
their system that persist even after they uninstall the application.

> There is a difference between the JavaFX libraries and application-specific 
> components.

No, not from the perspective of a user of an application that happens to use 
JavaFX. The same argument could be made for every third-party library used by 
an application, and you wouldn't want every one of those to make permanent 
changes to your system, too.

-

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

Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v4]

[Michael Strauß]

On Sat, 30 Oct 2021 14:28:09 GMT, Kevin Rushforth  wrote:

>> Michael Strauß has updated the pull request incrementally with four 
>> additional commits since the last revision:
>> 
>>  - revert rename
>>  - revert rename
>>  - test for KeyCombination
>>  - renamed TextBinding to MnemonicParser, refactored tests
>
> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/MnemonicParser.java
>  line 63:
> 
>> 61:  * 
>> 62:  */
>> 63: public class MnemonicParser {
> 
> As mentioned in the comments, I prefer this rename to be reverted. It could 
> be done separately as cleanup.

I created [JDK-8276206](https://bugs.openjdk.java.net/browse/JDK-8276206) to 
track this.

-

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

Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v4]

[Michael Strauß]

> This PR fixes an issue with mnemonic parsing by removing the restriction that 
> a mnemonic symbol must be a letter. Now, it can be any character except 
> whitespace.

Michael Strauß has updated the pull request incrementally with four additional 
commits since the last revision:

 - revert rename
 - revert rename
 - test for KeyCombination
 - renamed TextBinding to MnemonicParser, refactored tests

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/647/files
  - new: https://git.openjdk.java.net/jfx/pull/647/files/c4012056..5b5d720d

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=647=03
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=647=02-03

  Stats: 224 lines in 3 files changed: 146 ins; 72 del; 6 mod
  Patch: https://git.openjdk.java.net/jfx/pull/647.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/647/head:pull/647

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

Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v3]

[Michael Strauß]

On Sat, 30 Oct 2021 14:12:17 GMT, Jeanette Winzenburg  
wrote:

> there is no need for the class rename, is there?
> 
> Even though it's formally internal api, I think we shouldn't do code breaking 
> changes except if there's a very compelling reason - there are too many apps 
> out in the wild that use internal api.

I don't know how `TextBinding` describes in any way what this class is doing. 
Naming is important, and I think `MnemonicParser`, on the other hand, describes 
exactly what is going on. I would disagree that treating internal APIs as if 
they were public APIs is a good place to be. Cleaning up and refactoring 
internal APIs is important for the long-term health of a codebase.

-

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

Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v3]

[Michael Strauß]

On Fri, 29 Oct 2021 20:19:02 GMT, John Hendrikx  wrote:

>> Michael Strauß has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   changed javadoc
>
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/LabelSkinTest.java
>  line 2162:
> 
>> 2160: label.autosize();
>> 2161: skin.updateDisplayedText();
>> 2162: assertEquals("foo _bar _qux", 
>> LabelSkinBaseShim.getText(label).getText());
> 
> Can you verify here which letter / index is picked as mnemonic? (Also in the 
> other tests).

I don't see an easy way to do that, and I'm not in favor of making private 
implementation details package-public just to test some internal state. Of 
course, mnemonic support should have been designed in a way that is more easily 
testable, but this PR is not the place to do that.

-

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

Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v3]

[Michael Strauß]

On Fri, 29 Oct 2021 20:26:56 GMT, John Hendrikx  wrote:

>> Michael Strauß has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   changed javadoc
>
> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/TextBinding.java
>  line 188:
> 
>> 186: int i = 0;
>> 187: 
>> 188: // Parse the input string and stop after the first mnemonic.
> 
> This seems to do much more than just fixing numeric mnemonics.  Now it stops 
> after the first mnemonic, whereas it picked the last mnemonic before this 
> change (which seems to be a regression from what happened in jfx16).
> 
> I think the regression should be fixed first, then additional features can be 
> added?

This PR does not add any additional feature. It restores the behavior of jfx16, 
where any character is acceptable as a mnemonic, and the first mnemonic is 
selected.

-

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

Re: RFR: 8274699: Certain blend modes cannot be set from CSS [v2]

[Michael Strauß]

On Thu, 14 Oct 2021 21:47:16 GMT, Marius Hanl  wrote:

>> This PR fixes a bug where the blend mode will be falsely recognized as a 
>> color and therefore won't be set. 
>> Also a ClassCastException is thrown (The converter expects a String, but 
>> gets a Color).
>> 
>> Note: There is another similar bug but I can't reproduce it (Tried with 
>> 18-ea+3).
>> https://bugs.openjdk.java.net/browse/JDK-8268657
>> It also looks different so I don't think this PR fixes this, if it is still 
>> there.
>
> Marius Hanl has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - 8274699: Revert whitespace
>  - 8274699: Revert minor code change

Looks good, although the solution in itself remains fishy. Maybe create a 
ticket to implement a better solution in the future?

-

Marked as reviewed by mstrauss (Author).

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

Re: RFR: 8222455: JavaFX error loading glass.dll from cache

[Michael Strauß]

On Tue, 26 Oct 2021 12:09:19 GMT, Marius Hanl  wrote:

> This problem can happen when using multiple instances with different jfx 
> early access (ea) versions.
> 
> Example: 
> Instance 1 uses 18-ea+4 and Instance 2 uses 18-ea+1. 
> Instance 1 is started (and will cache and use libraries), then instance 2. 
> Now instance 2 detects via a hash comparison that the cached libraries are 
> not the same as the supplied ones. 
> With this information instance 2 tries to delete the old libraries but fails 
> while doing so (as instance 1 uses them currently) and will terminate right 
> after.
> 
> The problem here is that instance 1 and 2 are using the same cache folder: 
> **18-ea**. This is because the `NativeLibLoader` uses the `javafx.version` 
> property for determining the folder name, which in case of an ea version will 
> always be **18-ea** (for all ea versions starting with 18 obviously).
> 
> Fix as also mentioned in the ticket is to use the `javafx.runtime.version` 
> property instead. 
> With this the folders will be named 18-ea+1 and 18-ea+4.

Perhaps it would also be a good idea to move these files to the user's 
temporary directory, so that they can be cleaned up by the system. Otherwise 
they probably won't be removed in most cases when the user removes the 
application that created them.

-

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

Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v3]

[Michael Strauß]

On Fri, 29 Oct 2021 13:45:24 GMT, Ajit Ghaisas  wrote:

>> Done.
>
> The corresponding comment for the method isSimpleMnemonic() also needs 
> correction.
> You can limit it to -
> 
> /**
>  * Determines whether the string contains a simple mnemonic at the 
> specified position.
>  * A simple mnemonic is any two-character string similar to "_x", where x 
> is not an
>  * underscore or a whitespace character.
>  */

Done.

-

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

Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v3]

[Michael Strauß]

> This PR fixes an issue with mnemonic parsing by removing the restriction that 
> a mnemonic symbol must be a letter. Now, it can be any character except 
> whitespace.

Michael Strauß has updated the pull request incrementally with one additional 
commit since the last revision:

  changed javadoc

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/647/files
  - new: https://git.openjdk.java.net/jfx/pull/647/files/af670b07..c4012056

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=647=02
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=647=01-02

  Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod
  Patch: https://git.openjdk.java.net/jfx/pull/647.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/647/head:pull/647

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

Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v2]

[Michael Strauß]

On Fri, 29 Oct 2021 11:23:50 GMT, Ajit Ghaisas  wrote:

>> Michael Strauß has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Removed isExtendedMnemonic check from isSimpleMnemonic
>
> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/TextBinding.java
>  line 238:
> 
>> 236: }
>> 237: 
>> 238: return !isExtendedMnemonic(s, position);
> 
> I am not sure why do we need to check for isExtendedMnemonic() inside 
> isSimpleMnemonic() implementation.
> These two methods should be separate and it is up to the callers to check and 
> take appropriate action.

Done.

-

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

Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v2]

[Michael Strauß]

> This PR fixes an issue with mnemonic parsing by removing the restriction that 
> a mnemonic symbol must be a letter. Now, it can be any character except 
> whitespace.

Michael Strauß has updated the pull request incrementally with one additional 
commit since the last revision:

  Removed isExtendedMnemonic check from isSimpleMnemonic

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/647/files
  - new: https://git.openjdk.java.net/jfx/pull/647/files/7cc78d4a..af670b07

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=647=01
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=647=00-01

  Stats: 18 lines in 1 file changed: 5 ins; 9 del; 4 mod
  Patch: https://git.openjdk.java.net/jfx/pull/647.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/647/head:pull/647

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

Re: Improve property system to facilitate correct usage

[Michael Strauß]

I'd like to discuss the API changes surrounding content bindings for
this PR: https://github.com/openjdk/jfx/pull/590

Content bindings in the property system are semantically similar to
regular bindings:
1. You can only have a single content binding for a collection-type property
2. Unidirectional content bindings and bidirectional content bindings
are mutually exclusive

For this reason, and in order to support the behavioral changes
outlined in the PR, the content binding API (in ReadOnlyListProperty)
was changed as follows:

void bindContent(ObservableList source);
  + @Deprecated(since = "18", forRemoval = true)
void unbindContent(Object object);
  + void unbindContent();
  + boolean isContentBound();

This is not a breaking change, and allows application developers to
phase out the superfluous argument for `unbindContent` over time.

What might be more controversial is that this imposes a new
implementation requirement for some exotic implementations of
collection-type properties:

1. Right now, content bindings are implemented in
ReadOnlyListProperty, and every implementation of ROLP inherits the
default content binding implementation.

2. With the changed API, the implementation moves from
ReadOnlyListProperty to (ReadOnly)ListPropertyBase. This means that a
class that implements (RO)LP, but not (RO)LPBase, does not inherit the
default content binding implementation and needs to provide its own
implementation if it wants to support content bindings.

Of course, implementing (RO)LP without using (RO)LPBase is extremely
unusual. It requires the implementer to re-implement lots of code like
`ListExpressionHelper` and binding listeners.
Still, it's an additional requirement for these implementations that
also want to support content bindings. If content bindings are not
required, then the "old" (RO)LP implementation will continue to
compile and work.

What we get from this change are powerful correctness guarantees for
users of the API. Do you think this is a trade worth making?


On Wed, Jul 28, 2021 at 1:23 AM Michael Strauß  wrote:
>
> I propose a set of changes to the JavaFX property system that I've
> outlined in this PR: https://github.com/openjdk/jfx/pull/590
>
> The changes fall into two categories: enforcement of correct usage
> (there are several cases listed in the PR), and deprecating untyped
> APIs (for removal in a future version) so as to make the intent of the
> API more clear to developers.
>
> Even though there are breaking changes, the impact on application code
> should be minimal. I'd welcome any comments on this proposal.

Easier handling of ObservableList/Set/Map change events

[Michael Strauß]

I'd like to gauge interest on a small feature addition for JavaFX collections.

ObservableList (and similarly, ObservableSet/ObservableMap) allows
developers to register ListChangeListeners to observe changes to the
list. In some cases, these changes are applied to another list or
projected into a different form.

Implementing ListChangeListener correctly is quite tricky, especially
if the ObservableList implementation also fires "replace" and
"permutate" events. As a result, it is very easy to implement this
interface wrongly, which often goes undetected when the implementation
is only tested against basic operations like "add" and "remove".

Maybe we could make it easier for developers to get it right more of
the time, by offering pre-built adapters for ListChangeListener,
SetChangeListener and MapChangeListener.

Here's what that could look like:

public abstract class ListChangeListenerAdapter implements
ListChangeListener {
// Basic methods
public abstract void onAdded(int index, E element);
public abstract void onRemoved(int index);

// Optional methods
public void onAdded(int index, List elements);
public void onRemoved(int from, int to);
public void onReplaced(int index, E element);
public void onUpdated(int index, E element);
}

An implementation of this adapter must at the very least implement the
basic `onAdded` and `onRemoved` methods. The adapter will then
correctly map all other change events ("replace" and "permutate") to
these methods. All adapter methods will always be called in exactly
the right order, such that they always refer to the current state of
the list.

An adapter implementation can improve performance characteristics by
overriding the optional `onAdded`, `onRemoved` and `onReplaced`
methods (which map to `addAll`, `remove` and `set` of the List
interface). Optional methods are implemented by default to throw a
private exception in ListChangeListenerAdapter, which is used to
discover whether the methods are overridden and should be called
instead of the basic methods.

RFR: 8274854: Mnemonics for menu containing numeric text not working

[Michael Strauß]

This PR fixes an issue with mnemonic parsing by removing the restriction that a 
mnemonic symbol must be a letter. Now, it can be any character except 
whitespace.

-

Commit messages:
 - fixed mnemonic parsing
 - failing test

Changes: https://git.openjdk.java.net/jfx/pull/647/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jfx=647=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8274854
  Stats: 80 lines in 2 files changed: 72 ins; 1 del; 7 mod
  Patch: https://git.openjdk.java.net/jfx/pull/647.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/647/head:pull/647

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

Re: RFR: 8271091: Missing API docs in UI controls classes

[Michael Strauß]

On Wed, 20 Oct 2021 15:23:07 GMT, Nir Lisker  wrote:

>> This PR fixes javadoc warnings in javafx.controls and javafx.web modules.
>> Note : 
>> - The javadoc needs to be generated with the JDK 18 EA build.
>> - There are still 20 javadoc warnings remaining in javafx.controls module 
>> and 3 warnings remaining in javafx.web module. The root cause is different 
>> and they will be addressed under 
>> [JDK-8270996](https://bugs.openjdk.java.net/browse/JDK-8270996)
>
> modules/javafx.controls/src/main/java/javafx/scene/control/Control.java line 
> 874:
> 
>> 872: 
>> 873: /**
>> 874:  * Get the unmodifiable list of the controls css styleable 
>> properties.
> 
> Maybe "Gets the unmodifiable list of the control's css styleable 
> properties."? I'm not sure what this method really does, but if it relates to 
> a single control then that apostrophe is needed.

Also, "CSS" should probably be upper-cased in a sentence, like so: 
"CSS-styleable"

-

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

Re: [External] : Re: Eager Evaluation of Invalidation Listeners

[Michael Strauß]

1) The specification of Observable states that: "An implementation of
{@code Observable} may support lazy evaluation [...]"

It seems to me that this is an optional feature, and a user of this
API must be aware that any particular Observable implementation may
not support it, or that it chooses to implement a custom strategy to
reduce the number of invalidation events ("[...] should strive to
generate as few events as possible").

This means that a user of this API must expect that any particular
Observable implementation may generate an invalidation event for every
single change, or for changes in particular situations (for example,
after an InvalidationListener was added).

Let me summarize that as follows: Any well-behaved listener must
tolerate any lazy evaluation strategy.

I think this is important, because the "problem" that is being
discussed here can't, by definition, cause incorrect behavior for
users of the API.

2) Currently, adding an InvalidationListener validates the property.
This has implications on what I would see as "local reasoning" vs.
"global reasoning": developers can reason about their code in
isolation, and will generally get it right. Changing it means that
whether some listener code works can be dependent on other parts of
the system (parts that might validate the property, such that the
listener code will "accidentally" pick up the next change to the
property).

>From an API standpoint, I would consider this an unfortunate situation
because in many scenarios it will "just work" and not prompt any
further consideration from developers, even though the code that set
up the listener was incorrectly implemented. That's because a correct
implementation would be similar to the following "protocol":

prop.addListener(observable -> { ... prop.getValue(); ... });
prop.getValue();

Any code that doesn't use this protocol to set up an
InvalidationListener is now potentially incorrectly implemented, and
accidental validation will obscure this defect in most cases. Sure, we
can include this information in the specification of Observable, but
this is so easy to get wrong that even with explicitly mentioning it,
developers will routinely get it wrong. This makes for brittle
codebases and obscure defects, where changes to one part of a system
can unexpectedly and silently affect other parts of the system.

We can avoid all of that by changing the specification as follows: If
an Observable implementation supports lazy evaluation, it must not
elide an invalidation event after new listeners have been added.



On Thu, Oct 14, 2021 at 6:39 PM Nir Lisker  wrote:
>
> I disagree with this interpretation. Observable says
>
>>
>> Implementations in this library mark themselves as invalid when the first 
>> invalidation event occurs. They do not generate anymore invalidation events 
>> until their value is recomputed and valid again.
>
>
> And ObservableValue says
>
>> An ObservableValue generates two types of events: ... An invalidation event 
>> is generated if the current value is not valid anymore.
>
>
>> Implementations in this library mark themselves as invalid when the first 
>> invalidation event occurs. They do not generate any more invalidation events 
>> until their value is recomputed and valid again.
>
>
> It's clear that the validity is with respect to the value, not the listener. 
> There is 1 value and it is either valid or invalid.
>
> If we want to define validity on a per-listener basis, the docs would need to 
> be changed too. I don't know how much sense it makes practically because I 
> don't think anyone used them with this intention in mind. It could be a 
> middleground to bridge the current "negligence"  with the stricter docs, but 
> it's a more fundamental change conceptually.
>
> On Wed, Oct 6, 2021 at 8:02 PM Michael Strauß  wrote:
>>
>> The documentation of `Observable` states that "an implementation [...]
>> may support lazy evaluation" and that "implementations [...] should
>> strive to generate as few events as possible".
>> This seems to me like it would be within spec to fire an invalidation
>> event for every single change. It would also be within spec to fire
>> redundant invalidation events only in certain scenarios (like adding a
>> listener).
>>
>> The problem could also be approached from a different angle: what does
>> it mean for a property to be "valid"?
>> As implemented, the "valid" state is an opaque and unknowable
>> implementation detail. We could re-define "valid" to mean: valid from
>> the perspective of an InvalidationListener.
>> A newly-added InvalidationListener wouldn't know that the propert

Re: [External] : Re: Eager Evaluation of Invalidation Listeners

[Michael Strauß]

The documentation of `Observable` states that "an implementation [...]
may support lazy evaluation" and that "implementations [...] should
strive to generate as few events as possible".
This seems to me like it would be within spec to fire an invalidation
event for every single change. It would also be within spec to fire
redundant invalidation events only in certain scenarios (like adding a
listener).

The problem could also be approached from a different angle: what does
it mean for a property to be "valid"?
As implemented, the "valid" state is an opaque and unknowable
implementation detail. We could re-define "valid" to mean: valid from
the perspective of an InvalidationListener.
A newly-added InvalidationListener wouldn't know that the property is
invalid (and has no way of knowing), and can therefore reasonably
assume that, from its perspective, the property is valid. It would
receive an invalidation event when the property value is changed.
>From the perspective of pre-existing listeners, however, the property
could well have been invalid all the time, so they won't receive an
invalidation event.

On Wed, Oct 6, 2021 at 2:16 AM Kevin Rushforth
 wrote:
>
> Given that the intention of InvalidationListener was to be a
> light-weight way to listen to properties without causing a binding chain
> to be revalidated, it does seem reasonable to me that the listener is
> only fired on the valid --> invalid transition, which is the specified
> behavior, even if the implementation doesn't currently do that in all cases.
>
> The two related questions then are:
>
> 1. Should adding an invalidation listener to property do an immediate
> get(), which will ensure that the property is then valid? This will
> force an eager evaluation of the property and "arm" the property to fire
> an invalidation even the next time it is invalidated.
>
> 2. Should adding an invalidation listener to a currently invalid
> property cause the listener to be called (either immediately or the next
> time the object is invalidated)?
>
> I think the ideal answer to both questions is "no", although I share
> John's concern that changing this behavior for InvalidationListeners
> could break applications. So the question is: how likely do we think
> that changing this behavior will break existing applications?
>
> I think it's something we can and should consider changing. Unless there
> are serious concerns, I would support changing this behavior as part of
> avoiding eager evaluation when using invalidation listeners. It would
> need to be well tested (of course), and would need a CSR describing the
> compatibility risk, and should probably get a release note.
>
> Any concerns in doing this?
>
> On the related question, I like the idea of nulling out the current
> value when a property is bound.
>
> -- Kevin
>
>
> On 9/11/2021 9:41 AM, Nir Lisker wrote:
> > I think that we need your input on this to move it forward.
> >
> > On Fri, Sep 3, 2021 at 7:49 AM Nir Lisker  > > wrote:
> >
> > so the value field should perhaps be nulled out when bound.
> >
> >
> > There was a PR for something like that in the old repo:
> > https://github.com/javafxports/openjdk-jfx/pull/110
> > 
> > 
> >
> > On Fri, Sep 3, 2021 at 5:35 AM John Hendrikx  > > wrote:
> >
> >
> >
> > On 02/09/2021 11:57, Nir Lisker wrote:
> > > So in order
> > > to make sure that a new interested invalidation listener
> > does not miss
> > > the fact that a property was *already* invalid, the
> > easiest solution
> > > might have been to revalidate it upon registration of a
> > new listener
> > >
> > >
> > > But why does an invalidation listener need to know the past
> > state of the
> > > property? It is only interested in the valid -> invalid
> > transition. If
> > > the property was invalid then the listener (in theory)
> > shouldn't receive
> > > any events anyway on subsequent invalidations. (I understand
> > that you
> > > don't justify this, I'm posing it as a general question.)
> >
> > Strictly speaking, no, if users are using InvalidationListener
> > correctly
> > then this is definitely correct behavior. I'm not really
> > advocating a
> > change, and I'd even prefer that it be brought in line with the
> > documentation.
> >
> > I think however that many users are not using it correctly and
> > expect an
> > invalidation event always the next time the value changes (and
> > their
> > listener will read that value, validating it again), making it
> > act like
> > a 

Re: RFR: 8268225: Support :focus-visible and :focus-within CSS pseudoclasses [v7]

[Michael Strauß]

On Fri, 20 Aug 2021 05:15:49 GMT, Michael Strauß  wrote:

>> This PR adds the `Node.focusVisible` and `Node.focusWithin` properties, as 
>> well as the corresponding `:focus-visible` and `:focus-within` CSS 
>> pseudo-classes.
>
> Michael Strauß has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   fixed undeterministic test failures

I've created a [CSR](https://bugs.openjdk.java.net/browse/JDK-8274798) for this 
feature.

-

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

Re: Proof of concept for fluent bindings for ObservableValue

[Michael Strauß]

A little bit of bikeshedding here: I think this feature should be
thought of as a type-safe version of `Bindings.select`, which should
also be reflected in the terminology used (i.e. not map/flatMap). The
terminology, combined with the frequent comparisons to reactive
libraries might make it seem like an attempt to bring reactive
programming to JavaFX. That will certainly not happen, as JavaFX is
not (and never will be) a reactive framework. Naming it `select` will
convey the purpose and its place within JavaFX in a better and clearer
way.

On Wed, Mar 24, 2021 at 10:49 PM John Hendrikx  wrote:
>
> I just wanted to draw some attention to a recent proof of concept I made
> in this pull request: https://github.com/openjdk/jfx/pull/434
>
> It is based on the work I did in
> https://github.com/hjohn/hs.jfx.eventstream which is in part based on
> work done in ReactFX by Tomas Mikula. The PR itself however shares no
> code with ReactFX and is
> completely written by me.
>
> If there is interest, I'm willing to invest more time in smoothing out
> the API and documentation, investigating further how this would interact
> with the primitive types and adding unit test coverage (I have extensive
> tests, but thesea are written in JUnit 5, so they would require
> conversion or JavaFX could move to support JUnit 5).
>
> What follows below is the text of the PR for easy reading. Feedback is
> appreciated.
>
> 
>
> This is a proof of concept of how fluent bindings could be introduced to
> JavaFX. The main benefit of fluent bindings are ease of use, type safety
> and less surprises. Features:
>
> 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: #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 or orElseGet.
>
> Conditional Bindings
> Bindings can be made conditional using the conditionOn method. A
> conditional binding retains its last value when its condition is false.
> Conditional bindings donot observe their source when the condition is
> false, allowing developers to automatically stop listening to properties
> when a certain condition is met. A major use of this feature is to have
> UI components that need to keep models updated which may outlive the UI
> conditionally update the long lived model only when the UI is showing.
>
> 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")
>);
> }
>
> void 

Re: Re: JavaFX snapping and scale

[Michael Strauß]

No, we don't always render at integer coordinates. If you place a
control at 10,10 and disable pixel snapping, it'll end up at 12.5,12.5
(with 125% scaling) in render coordinates.

The process of coloring pixels on the output texture based on triangle
polygons is called rasterization. For each pixel, the rasterizer
determines whether it's inside of the polygon to be drawn. If the
pixel is inside, it will be colored according to the output of a
shader program; if it's not inside, it will be unaffected. More
precisely, a pixel is inside of a polygon if its center point is
inside of the polygon.

So we do render at non-integer coordinates, but it's the
Direct3D/OpenGL/etc. rasterization rules that decide whether or not a
pixel will appear on the output texture.

Regarding the PR: that's a solution for a different problem.


On Tue, Sep 28, 2021 at 5:53 PM Marius Hanl  wrote:
>
> Thanks for your reply.
>
> Does that mean that we will always render at a round number, e.g. 13, never 
> on a decimal number?
> Because then I'm wondering how we can render at a decimal number given we 
> can't split a pixel.
>
> If this actually fixes the problem it might be worth a revisit.
> Can I try this out as is?
> If so and it actually fixes my problem/makes it a lot easier this PR will 
> probably get more attention and can benefit from it.
>

Re: RFR: 8268225: Support :focus-visible and :focus-within CSS pseudoclasses [v7]

[Michael Strauß]

On Thu, 9 Sep 2021 09:15:06 GMT, Jeanette Winzenburg  
wrote:

> Just curious: with this in place, would it be possible to use for supporting 
> [JDK-8087926](https://bugs.openjdk.java.net/browse/JDK-8087926) (it's a bit 
> vague, though, at least for me)?

Yes, `:focus-within` can be used to select an ancestor of the currently-focused 
node.

-

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

Integrated: 8273324: IllegalArgumentException: fromIndex(0) > toIndex(-1) after clear and select TableCell

[Michael Strauß]

On Fri, 3 Sep 2021 18:19:45 GMT, Michael Strauß  wrote:

> This PR fixes the exception thrown by the sample code in 
> [8273324](https://bugs.openjdk.java.net/browse/JDK-8273324), while retaining 
> the incorrect behavior in the scenario described.

This pull request has now been integrated.

Changeset: a272c4f6
Author:    Michael Strauß 
Committer: Kevin Rushforth 
URL:   
https://git.openjdk.java.net/jfx/commit/a272c4f6bd08fee8928c78e17428574aec485cfd
Stats: 53 lines in 2 files changed: 50 ins; 0 del; 3 mod

8273324: IllegalArgumentException: fromIndex(0) > toIndex(-1) after clear and 
select TableCell

Reviewed-by: jpereda, kcr

-

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