On Mon, 28 Oct 2024 16:13:37 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> Public focus traversal API for use in custom controls >> >> https://github.com/andy-goryachev-oracle/Test/blob/main/doc/FocusTraversal/FocusTraversal-v3.md >> >> This work is loosely based on the patch >> https://cr.openjdk.org/~jgiles/8061673/ >> >> And is a scaled down version (with the public traversal policy API removed) >> of >> #1555 > > Andy Goryachev has updated the pull request incrementally with one additional > commit since the last revision: > > review comments The changes look good, left minor comments. I only reviewed the API. modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 10451: > 10449: */ > 10450: public final boolean requestFocusTraversal(TraversalDirection > direction) { > 10451: TraversalDirectionInternal d = switch(direction) { Minor: space after `switch`. modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 10458: > 10456: case RIGHT -> TraversalDirectionInternal.RIGHT; > 10457: case UP -> TraversalDirectionInternal.UP; > 10458: default -> null; Do we indent labels inside a switch? I find it more readable since `switch` opens a new context. This is also what I remember seeing in OpenJFX. Don't know if it's enforced. modules/javafx.graphics/src/main/java/javafx/scene/TraversalDirection.java line 38: > 36: /** Indicates a focus change to the node to the left of the currently > focused node. */ > 37: LEFT, > 38: /** Indicates a focus change to the next focusable Node, possibly > traversing into the children of the current parent. */ Minor: "Node" -> "node" like the other places it's used. modules/javafx.graphics/src/main/java/javafx/scene/TraversalDirection.java line 40: > 38: /** Indicates a focus change to the next focusable Node, possibly > traversing into the children of the current parent. */ > 39: NEXT, > 40: /** Indicates a focus change to the previous focusable node. */ `NEXT` talks about traversing children, does `PREVIOUS` need to mention it too? Could it traverse out of the children of the current parent, or is it not symmetric? ------------- PR Review: https://git.openjdk.org/jfx/pull/1604#pullrequestreview-2399576178 PR Review Comment: https://git.openjdk.org/jfx/pull/1604#discussion_r1819364915 PR Review Comment: https://git.openjdk.org/jfx/pull/1604#discussion_r1819366367 PR Review Comment: https://git.openjdk.org/jfx/pull/1604#discussion_r1819362996 PR Review Comment: https://git.openjdk.org/jfx/pull/1604#discussion_r1819364334
