[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: RFR: 8283551: ControlAcceleratorSupport menu items listener causes memory leak [v2]
From:       Andy Goryachev <angorya () openjdk ! org>
Date:       2023-02-24 19:12:22
Message-ID: xJOow0QDp8_HIGVaw1uodHGnnBCBg2W8WMuLADfH0KI=.a335dff9-101e-4247-8325-1726977bc545 () github ! com
[Download RAW message or body]

On Thu, 23 Feb 2023 09:52:08 GMT, Dean Wookey <dwookey@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


[prev in list] [next in list] [prev in thread] [next in thread]