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
