On Mon, 14 Nov 2022 10:32:47 GMT, Ajit Ghaisas <aghai...@openjdk.org> wrote:
>> Andy Goryachev has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains 24 additional >> commits since the last revision: >> >> - 8294809: review comments >> - Merge remote-tracking branch 'origin/master' into 8294809.listener.helper >> - 8294809: whitespace >> - 8294809: no public api >> - 8294809: map change listener >> - Merge remote-tracking branch 'origin/master' into 8294809.listener.helper >> - 8294809: generics >> - 8294809: is alive >> - Revert "8294809: removed weak listeners support" >> >> This reverts commit 2df4a85db638d76cacaf6c54ba669cdb3dd91a18. >> - 8294809: removed weak listeners support >> - ... and 14 more: https://git.openjdk.org/jfx/compare/8cec4ed6...470f42c1 > > modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ListenerHelper.java > line 70: > >> 68: * </ul> >> 69: */ >> 70: public class ListenerHelper implements IDisconnectable { > > This class is mixing two things- > 1. Managing listeners in a generic way - by providing ability to add & > removeAll (via disconnect()) > 2. Trying to cater to *Skin specific requirement > > Ideally, (2) above should be done separately. I mean, the class > ListenerHelper should not use `SkinBase`. > What do you think? you are correct: the original intent for this class was to make it a general purpose facility to help with listeners, something that might be useful at the application level (and it used `ListenerHelper.get(Node)`). Since that would require CSR and a public discussion, we decided to hide it as an implementation detail in skins (to unblock skin memory leak fixes). Once we go through all the necessary discussions and everyone agrees, we could introduce it as a public API in the javafx.base module. ------------- PR: https://git.openjdk.org/jfx/pull/908
