On Mon, 4 May 2020 15:46:57 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> Ajit Ghaisas has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Make ChangeListener Final > > modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuButtonSkinBase.java > line 230: > >> 229: // Remove listeners >> 230: >> getSkinnable().sceneProperty().removeListener(sceneChangeListener); >> 231: getSkinnable().getItems().removeListener(itemsChangedListener); > > If scene is set to null (which can happen if this control or its parent is > removed from the scene graph) before dispose > is called, this will throw an NPE. I think it should be moved inside the > above if statement. > This raises the question: when the scene changes, are both the accelerators > and this listener removed from the old > scene? If MenuButton is removed from a scene : Valid scene -> null scene : sceneChangeListener is invoked to remove accelerators registered with old valid scene. If the skin is disposed after this - we have a guard to prevent removal of accelerators from a null Scene. Also, removing of the listener using sceneProperty() does not result in NPE. To your specific question : When the Scene changes, only the accelerators need to be removed from the old Scene without removing sceneChangeListener from skin. This is due to the fact that the Control (removed from old Scene) can be added to another Scene without recreating the skin. Please let me know if you have further questions. > modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuButtonSkinBase.java > line 77: > >> 76: private ListChangeListener<MenuItem> itemsChangedListener; >> 77: private ChangeListener<? super Scene> sceneChangeListener; >> 78: > > Can this be made final? Yes. I have made it. ------------- PR: https://git.openjdk.java.net/jfx/pull/205
