On Thu, 24 Nov 2022 06:49:02 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
> The `Scene.KeyHandler` class doesn't seem to have a clear purpose, mixing > focus handling with event propagation. Since #852, > `KeyHandler.setFocusVisible` is also called from mouse and touch event > handlers, which makes the purpose of the class even less pronounced. > > Moving the focus-related functionality next to the other focus functions in > the `Scene` class makes it easier to work with the code in the future. > > With the focus-related functions gone, `KeyHandler` only contains a single, > small method that is called from `Scene.processKeyEvent`. For simplicity, > this code can be rolled into `Scene.processKeyEvent` and the now-empty > `KeyHandler` class can be removed. The changes look equivalent to the code before. Suggesting minor changes. Shall do some sanity testing with focus and update. modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 2091: > 2089: > **************************************************************************/ > 2090: > 2091: private void windowForSceneChanged(Window oldWindow, Window window) > { Minor: rename the second parameter `window` to `newWindow`. As the method source is shown modified in the patch it seems Okay to make this change. modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 2208: > 2206: > 2207: private class FocusOwnerProperty extends > ReadOnlyObjectWrapper<Node> { > 2208: Node oldValue; The variable name `oldFocusOwner` seems more readable and using the same name as before will reduce the diff. Could you please change the name to `oldFocusOwner` ------------- Changes requested by arapte (Reviewer). PR: https://git.openjdk.org/jfx/pull/962