8296409: Multiple copies of accelerator change listeners are added to MenuItems, but only 1 is removed #937

DeanWookey · 2022-11-04T16:07:54Z

When menu buttons are added and removed from the scene, an accelerator change listener is added to each menu item in the menu. There is nothing stopping the same change listener being added multiple times.

MenuButtonSkinBase calls the ControlAcceleratorSupport.addAcceleratorsIntoScene(getSkinnable().getItems(), getSkinnable()); method each time the button is added to the scene, but that method itself also registers a listener to call itself. Each time the button is added to the scene, the method is called at least twice.

When it's removed from the scene, the remove accelerator method is also called twice, but only the first call removes a change listener attached to the accelerator because the first call removes the entry from the hashmap changeListenerMap. The second call finds nothing in the map, and doesn't remove the additional instance.

This pull request just removes the redundant code in the MenuButtonSkinBase.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8296409: Multiple copies of accelerator change listeners are added to MenuItems, but only 1 is removed

Reviewers

Andy Goryachev (@andy-goryachev-oracle - Committer)
Ambarish Rapte (@arapte - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx pull/937/head:pull/937
$ git checkout pull/937

Update a local copy of the PR:
$ git checkout pull/937
$ git pull https://git.openjdk.org/jfx pull/937/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 937

View PR using the GUI difftool:
$ git pr show -t 937

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/937.diff

bridgekeeper · 2022-11-04T16:10:13Z

👋 Welcome back dwookey! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

mlbridge · 2022-11-04T16:15:05Z

Webrevs

andy-goryachev-oracle · 2022-11-04T17:25:57Z

I wonder if changes in JDK-8295426: MenuButtonSkin: memory leak when changing skin would impact this PR or vice versa.

PR: #919

andy-goryachev-oracle · 2022-11-04T19:28:02Z

@DeanWookey : the newly added test still fails with JDK-8295426 changes, so I think we might want to address/integrate this PR first.

kevinrushforth · 2022-11-04T19:51:53Z

Reviewers: @arapte @andy-goryachev-oracle

andy-goryachev-oracle · 2022-11-04T21:14:20Z

modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuButtonSkinBase.java


        List<Mnemonic> mnemonics = new ArrayList<>();
        sceneChangeListener = (scene, oldValue, newValue) -> {
            if (oldValue != null) {
-                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: 80971d8
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.

please forgive me - it might take me a bit longer to review.

DeanWookey · 2022-11-08T10:11:40Z

@kevinrushforth, @arapte I clicked the request review on andy and it removed arapte for some reason. I guess that button shouldn't be clicked. Oops.

kevinrushforth · 2022-11-08T13:04:12Z

I clicked the request review on andy and it removed arapte for some reason. I guess that button shouldn't be clicked. Oops.

That seems like a GitHub UI problem. I'll re-request a review from @arapte

andy-goryachev-oracle · 2022-11-30T19:07:14Z

@DeanWookey :
Would it be possible to modify this PR to use the alternative solution 80971d8 ? I think the code in this PR as it stands right now removes a bit of functionality that is needed, while the alternative does not.

Also, would it be possible to base your changes on top of #919 since the skin got reworked to eliminate a memory leak, and it might be easier to base your changes on top of that rather than try to resolve non-trivial merge conflicts.

What do you think?

andy-goryachev-oracle

change requested:

use alternative code
base on top of 8295426: MenuButtonSkin: memory leak when changing skin #919

kevinrushforth · 2022-11-30T19:32:37Z

base on top of #919

Doing this makes it harder for reviewers to see what is actually being changed. Unless / until we enable the Skara support for dependent PRs (which the jdk repo enables, but so far we haven't), I would probably just recommend waiting on the final review until after the dependent PR, #919 in this case, is integrated and then merge master at that time.

andy-goryachev-oracle · 2022-11-30T19:41:05Z

Doing this makes it harder for reviewers to see what is actually being changed.

This is true, but it will the actual review process (especially, testing) move faster. Or, we can wait for #919 to get integrated first.

edit: I want to avoid non-trivial merges, they are harder to deal with than just ignoring changes from another PR.

kevinrushforth · 2022-11-30T19:52:19Z

Probably better to just wait then.

arapte · 2022-12-06T06:05:21Z

Now that #919 is integrated, the conflict needs to be resolved.

# Conflicts: # modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuButtonSkinBase.java

DeanWookey · 2022-12-10T10:32:43Z

I've put the alternative fix in on top of the latest changes. Sorry it took so long.

andy-goryachev-oracle

looks much leaner now, thanks!

openjdk · 2022-12-15T09:47:52Z

@DeanWookey This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8296409: Multiple copies of accelerator change listeners are added to MenuItems, but only 1 is removed

Reviewed-by: angorya, arapte

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 7 new commits pushed to the master branch:

ae86ed3: 8297067: Update Gradle to 7.6
30147d2: 8298496: IconifyTest fails intermittently on Linux
58376eb: 8187145: (Tree)TableView with null selectionModel: throws NPE on sorting
adfc022: 8290040: Provide simplified deterministic way to manage listeners
f217d5e: 8298200: Clean up raw type warnings in javafx.beans.property.* and com.sun.javafx.property.*
8763e8b: 8190411: NPE in SliderSkin:140 if Slider.Tooltip.autohide is true
dadfcaf: 8295339: DatePicker updates its value property with wrong date when dialog closes

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@andy-goryachev-oracle, @arapte) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

DeanWookey · 2022-12-19T11:59:29Z

/integrate

openjdk · 2022-12-19T12:00:43Z

@DeanWookey
Your change (at version e934696) is now ready to be sponsored by a Committer.

arapte · 2022-12-19T12:25:41Z

/sponsor

openjdk · 2022-12-19T12:27:35Z

Going to push as commit bac8ee8.
Since your change was applied there have been 7 commits pushed to the master branch:

ae86ed3: 8297067: Update Gradle to 7.6
30147d2: 8298496: IconifyTest fails intermittently on Linux
58376eb: 8187145: (Tree)TableView with null selectionModel: throws NPE on sorting
adfc022: 8290040: Provide simplified deterministic way to manage listeners
f217d5e: 8298200: Clean up raw type warnings in javafx.beans.property.* and com.sun.javafx.property.*
8763e8b: 8190411: NPE in SliderSkin:140 if Slider.Tooltip.autohide is true
dadfcaf: 8295339: DatePicker updates its value property with wrong date when dialog closes

Your commit was automatically rebased without conflicts.

openjdk · 2022-12-19T12:27:57Z

@arapte @DeanWookey Pushed as commit bac8ee8.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8296409: Stop additional change listeners being added

0dae693

DeanWookey changed the title ~~8296409: Stop additional change listeners being added~~ 8296409: Multiple copies of accelerator change listeners are added to MenuItems, but only 1 is removed Nov 4, 2022

openjdk bot added the rfr Ready for review label Nov 4, 2022

kevinrushforth requested a review from arapte November 4, 2022 19:51

andy-goryachev-oracle reviewed Nov 4, 2022

View reviewed changes

Added changing scene tests for accelerator change listeners

52a9881

DeanWookey requested review from andy-goryachev-oracle and removed request for arapte November 8, 2022 10:08

kevinrushforth requested review from arapte and removed request for andy-goryachev-oracle November 8, 2022 13:04

andy-goryachev-oracle suggested changes Nov 30, 2022

View reviewed changes

andy-goryachev-oracle mentioned this pull request Nov 30, 2022

8295426: MenuButtonSkin: memory leak when changing skin #919

Closed

3 tasks

DeanWookey added 2 commits December 10, 2022 12:02

Merge branch 'master' of https://github.com/openjdk/jfx into JDK-8296409

d083b05

# Conflicts: # modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuButtonSkinBase.java

Implemented alternative fix

e934696

andy-goryachev-oracle approved these changes Dec 12, 2022

View reviewed changes

arapte approved these changes Dec 15, 2022

View reviewed changes

openjdk bot added the ready Ready to be integrated label Dec 15, 2022

openjdk bot added the sponsor Ready to sponsor label Dec 19, 2022

openjdk bot added the integrated Pull request has been integrated label Dec 19, 2022

openjdk bot closed this Dec 19, 2022

openjdk bot removed ready Ready to be integrated rfr Ready for review sponsor Ready to sponsor labels Dec 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8296409: Multiple copies of accelerator change listeners are added to MenuItems, but only 1 is removed #937

8296409: Multiple copies of accelerator change listeners are added to MenuItems, but only 1 is removed #937

DeanWookey commented Nov 4, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Nov 4, 2022

mlbridge bot commented Nov 4, 2022 •

edited

Loading

andy-goryachev-oracle commented Nov 4, 2022 •

edited

Loading

andy-goryachev-oracle commented Nov 4, 2022

kevinrushforth commented Nov 4, 2022

andy-goryachev-oracle Nov 4, 2022

DeanWookey Nov 7, 2022

DeanWookey Nov 8, 2022

andy-goryachev-oracle Nov 9, 2022

DeanWookey commented Nov 8, 2022

kevinrushforth commented Nov 8, 2022

andy-goryachev-oracle commented Nov 30, 2022 •

edited

Loading

andy-goryachev-oracle left a comment

kevinrushforth commented Nov 30, 2022

andy-goryachev-oracle commented Nov 30, 2022 •

edited

Loading

kevinrushforth commented Nov 30, 2022

arapte commented Dec 6, 2022

DeanWookey commented Dec 10, 2022

andy-goryachev-oracle left a comment

openjdk bot commented Dec 15, 2022 •

edited

Loading

DeanWookey commented Dec 19, 2022

openjdk bot commented Dec 19, 2022

arapte commented Dec 19, 2022

openjdk bot commented Dec 19, 2022

openjdk bot commented Dec 19, 2022

8296409: Multiple copies of accelerator change listeners are added to MenuItems, but only 1 is removed #937

8296409: Multiple copies of accelerator change listeners are added to MenuItems, but only 1 is removed #937

Conversation

DeanWookey commented Nov 4, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 4, 2022

mlbridge bot commented Nov 4, 2022 • edited Loading

Webrevs

andy-goryachev-oracle commented Nov 4, 2022 • edited Loading

andy-goryachev-oracle commented Nov 4, 2022

kevinrushforth commented Nov 4, 2022

andy-goryachev-oracle Nov 4, 2022

Choose a reason for hiding this comment

DeanWookey Nov 7, 2022

Choose a reason for hiding this comment

DeanWookey Nov 8, 2022

Choose a reason for hiding this comment

andy-goryachev-oracle Nov 9, 2022

Choose a reason for hiding this comment

DeanWookey commented Nov 8, 2022

kevinrushforth commented Nov 8, 2022

andy-goryachev-oracle commented Nov 30, 2022 • edited Loading

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

kevinrushforth commented Nov 30, 2022

andy-goryachev-oracle commented Nov 30, 2022 • edited Loading

kevinrushforth commented Nov 30, 2022

arapte commented Dec 6, 2022

DeanWookey commented Dec 10, 2022

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

openjdk bot commented Dec 15, 2022 • edited Loading

DeanWookey commented Dec 19, 2022

openjdk bot commented Dec 19, 2022

arapte commented Dec 19, 2022

openjdk bot commented Dec 19, 2022

openjdk bot commented Dec 19, 2022

DeanWookey commented Nov 4, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 4, 2022 •

edited

Loading

andy-goryachev-oracle commented Nov 4, 2022 •

edited

Loading

andy-goryachev-oracle commented Nov 30, 2022 •

edited

Loading

andy-goryachev-oracle commented Nov 30, 2022 •

edited

Loading

openjdk bot commented Dec 15, 2022 •

edited

Loading