On Thu, 23 Feb 2023 09:26:29 GMT, Marius Hanl <mh...@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
