On Fri, 25 Oct 2024 16:22:01 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 Took a quick look, mostly of the API. modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ListenerHelper.java line 466: > 464: callback.accept(n); > 465: } > 466: }; Any reason you're using a listener instead of a subscription, which is easier to manage and less prone to memory leaks? modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 10442: > 10440: > 10441: /** > 10442: * Requests focus traversal from this {@code Node} in the > specified direction. I think that explaining what the method does by repeating the method name can be improved upon. I would use a simple "Requests to move the focus from...", or "Tries to move the focus from..." in case the method name doesn't end up using "try" and you want to use it in the docs instead. modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 10443: > 10441: /** > 10442: * Requests focus traversal from this {@code Node} in the > specified direction. > 10443: * A successful traversal results in the newly focused {@code > Node} visibly indicating its focused state. A successful traversal also results in the newly focused node handling key events. Maybe add a bit of explanation on focus, like: "A focused node is the target of key events and has a visual indicator. A successful traversal results in a new {@code Node} being focused". Maybe even reverse the order of these sentences, this is just one example. modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 10446: > 10444: * > 10445: * @param direction the direction of focus traversal > 10446: * @return true if traversal was successful `true` in `@code` modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 10451: > 10449: public final boolean requestFocusTraversal(TraversalDirection > direction) { > 10450: TraversalDirectionInternal d; > 10451: switch (direction) { Using an expression switch will be less verbose: TraversalDirectionInternal d = switch (direction) { case DOWN -> TraversalDirectionInternal.DOWN; case LEFT -> TraversalDirectionInternal.LEFT; ... default -> null; }; return d == null ? false : TraversalUtils.traverse(this, d, true); modules/javafx.graphics/src/main/java/javafx/scene/TraversalDirection.java line 30: > 28: * Specifies the direction of focus traversal. > 29: * > 30: * @since 24 I would add `@see` for the method that uses this enum, otherwise it's rather disconnected from anything. modules/javafx.graphics/src/main/java/javafx/scene/TraversalDirection.java line 33: > 31: */ > 32: public enum TraversalDirection { > 33: /** Moves focus downward. */ The direction itself doesn't move the focus. I would use a different phrasing, maybe something like "Indicates a focus change to the node below the currently focused node". Same with the others. ------------- PR Review: https://git.openjdk.org/jfx/pull/1604#pullrequestreview-2397264970 PR Review Comment: https://git.openjdk.org/jfx/pull/1604#discussion_r1817862651 PR Review Comment: https://git.openjdk.org/jfx/pull/1604#discussion_r1817865185 PR Review Comment: https://git.openjdk.org/jfx/pull/1604#discussion_r1817865948 PR Review Comment: https://git.openjdk.org/jfx/pull/1604#discussion_r1817866007 PR Review Comment: https://git.openjdk.org/jfx/pull/1604#discussion_r1817867723 PR Review Comment: https://git.openjdk.org/jfx/pull/1604#discussion_r1817868020 PR Review Comment: https://git.openjdk.org/jfx/pull/1604#discussion_r1817868388
