On Fri, 24 Feb 2023 19:09:03 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> 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. We use this pattern of having methods with the same name and overloaded args, some of which are not public, in many places. In general this seems fine. In specific cases where it might help alleviate confusion, using "Impl" as a suffix is OK (and better than "Private" since we might make some impl methods package scope). ------------- PR: https://git.openjdk.org/jfx/pull/1037
