On Sat, 30 Oct 2021 10:56:40 GMT, Florian Kirmaier <fkirma...@openjdk.org>
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<Scene> 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<Scene> 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<Scene> 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<Scene> listener =
> sceneChangeListenerMap.get(anchor).get();
> 255: if(listener != null) {
Minor: space before brace
-------------
PR: https://git.openjdk.java.net/jfx/pull/659
