On Mon, 7 Nov 2022 18:11:28 GMT, Dean Wookey <dwoo...@openjdk.org> wrote:
>> modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuButtonSkinBase.java
>> line 154:
>>
>>> 152: sceneChangeListener = (scene, oldValue, newValue) -> {
>>> 153: if (oldValue != null) {
>>> 154:
>>> ControlAcceleratorSupport.removeAcceleratorsFromScene(getSkinnable().getItems(),
>>> oldValue);
>>
>> will it handle a case where the menu button gets attached to a different
>> scene?
>> could you add a second test for this scenario please?
>>
>> And i wonder if the problem is in ControlAcceleratorSupport rather than
>> here. We do have a similar code in Control:380, do we have a problem there?
>
> The code in ControlAcceleratorSupport adds its own scene listeners for each
> node added. MenuButtonSkinBase does the same, unlike Control which only calls
> it once.
>
> I think we could fix it in ControlAcceleratorSupport alternatively or in
> addition to this fix. The code in ControlAcceleratorSupport appears to do
> everything this would've done.
>
> I've added the test for changing scenes.
Here is an alternative fix:
https://github.com/openjdk/jfx/commit/80971d89c46f3f74cb8584d4907cc6818809e2f6
I just reverted the MenuButtonSkinBase changes. The tests pass with this fix,
or with both.
I'm a little more nervous about that one because I had to remove the code that
removed and then added a scene listener. I'm not sure why it was done because
the listener is independent of a specific scene.
Basically, that fix only does the install if the install wasn't already done on
that anchor meaning it can get called any number of times safely stopping
duplicate accelerators being added.
I would go with either both fixes together, or just the MenuButtonSkinBase one.
-------------
PR: https://git.openjdk.org/jfx/pull/937
