On Thu, 23 Feb 2023 09:52:08 GMT, Dean Wookey <dwoo...@openjdk.org> wrote:
>> 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. I just want to avoid confusion when we have public and non-public methods with the same name. but it's a minor comment, especially if there will be subsequent rework later. ------------- PR: https://git.openjdk.org/jfx/pull/1037
