On Tue, 2 Nov 2021 08:16:41 GMT, Florian Kirmaier <[email protected]> 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
The fix looks good, although I left one question inline (also there is an
unused import).
The test looks good. I verified that it fails without your fix and passes with
your fix.
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;
Unused import.
modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java
line 291:
> 289: WeakReference<ChangeListener<KeyCombination>> listenerW
> = changeListenerMap.get(menuitem);
> 290: ChangeListener<KeyCombination> listener = listenerW ==
> null ? null : listenerW.get();
> 291: if (listener != null) {
This will fail to remove a weak reference to a listener that has been
collected. It is a `WeakHashMap`, so the `WeakReference` to the listener will
be reclaimed as soon as the `menuItem` is no longer reachable, but if you used
the same logic as you did for the scene listener (e.g., on lines 254-261), it
would be removed sooner. I don't know whether it matters in this case.
-------------
PR: https://git.openjdk.java.net/jfx/pull/659
