RFR: 8283551: ControlAcceleratorSupport menu items listener causes memory leak

[Dean Wookey]

On Thu, 23 Feb 2023 09:26:29 GMT, Marius Hanl <mhanl at openjdk.org> wrote:

>> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java line 285:
>> 
>>> 283:     }
>>> 284: 
>>> 285:     private static void removeAcceleratorsFromScene(List<? extends MenuItem> items, Scene scene) {
>> 
>> May I suggest a change name: removeAcceleratorsFromScenePrivate() to avoid confusion?
>
> I'm not in favor of using `Private` in a method name. That is clear from the method signature and overloading methods is a valid choice  In my opinion, this is fine as is. 
> But we could also think about naming it: `removeAcceleratorsFromSceneImpl()`

I've been working on changes for a possible follow-up PR which address more bugs. For adding accelerators, there are 3 public methods, for removing there are currently 4.

I've looked at it, and it can/should be made to have 3 public remove methods that exactly mirror the public add methods (if you use a specific one to add then use the corresponding one to remove).

Further, for every private addAcceleratorsFromScene method and doAcceleratorInstall method I believe there should be a private corresponding method which reverses it. I had to rename a couple private removeAcceleratorsFromScene to doAcceleratorUninstall.

So yes, this is a very confusing class. Pairing up the add/remove methods make sense to me. Once that's done, we might want to rename some of the private ones just so it's easier to understand what each one is doing, but the public ones are fine I think.

-------------

PR: https://git.openjdk.org/jfx/pull/1037