On Tue, 3 Sep 2024 19:09:15 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
> Public APIs for focus traversal and the focus traversal policy: > > https://github.com/andy-goryachev-oracle/Test/blob/main/doc/FocusTraversal/FocusTraversal.md > > This work is loosely based on the patch > https://cr.openjdk.org/~jgiles/8061673/ I did a first pass of the public API and left a few comments. I'll need a second pass and a more careful look at the docs. modules/javafx.graphics/src/main/java/javafx/scene/Parent.java line 850: > 848: /** > 849: * The {@link TraversalPolicy} to be used by this Parent to provide > assistance to the > 850: * JavaFX focus traversal subsystem. What assistance does this policy provide? When is it used? modules/javafx.graphics/src/main/java/javafx/scene/Parent.java line 853: > 851: * > 852: * @defaultValue null > 853: * @since 999 TODO Don't forget to update this (probably to jfx24, since there is plenty of time to get this in). modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 31: > 29: import java.io.File; > 30: import java.lang.ref.WeakReference; > 31: import java.security.AccessControlContext; Meta-comment: This is the sort of bulk reordering that _really_ makes me want a prescribed ordering that we all use. modules/javafx.graphics/src/main/java/javafx/scene/traversal/FocusTraversal.java line 34: > 32: > 33: /** > 34: * Provides the mechanism for focus traversal in the JavaFX application. I'd like to see additional documentation here describing focus traversal. modules/javafx.graphics/src/main/java/javafx/scene/traversal/FocusTraversal.java line 40: > 38: public final class FocusTraversal { > 39: /** > 40: * Traverses focus to the adjacent node as specified by the direction. What does it do if the `node` to traverse from is not the currently focused node? How does traversal differ if the method is `KEY` vs `DEFAULT`? modules/javafx.graphics/src/main/java/javafx/scene/traversal/FocusTraversal.java line 63: > 61: > 62: /** > 63: * Traverse focus downward as a response to pressing a key. Is this a convenience method like the ones that follow? If so, add docs (and if not, why not?). modules/javafx.graphics/src/main/java/javafx/scene/traversal/FocusTraversal.java line 65: > 63: * Traverse focus downward as a response to pressing a key. > 64: * > 65: * @param node the origin node What is the "origin" node? Is it is the same as the "node to traverse focus from" in the `traverse` method? If so, use the same language. If not, clearly define the difference. modules/javafx.graphics/src/main/java/javafx/scene/traversal/TraversalDirection.java line 35: > 33: */ > 34: public enum TraversalDirection { > 35: /** Moving focus downward. */ Suggestion: `Moving` --> `Moves` modules/javafx.graphics/src/main/java/javafx/scene/traversal/TraversalMethod.java line 26: > 24: */ > 25: > 26: package javafx.scene.traversal; You need an `@since` javadoc tag on this class. modules/javafx.graphics/src/main/java/javafx/scene/traversal/TraversalPolicy.java line 51: > 49: * @since 999 TODO > 50: */ > 51: public abstract class TraversalPolicy { Who subclasses this? And for what purpose? This should be documented in the class docs. modules/javafx.graphics/src/main/java/javafx/scene/traversal/TraversalPolicy.java line 53: > 51: public abstract class TraversalPolicy { > 52: /** > 53: * Traverse from owner, in direction dir. Is the "owner" the "focus owner"? What is its relationship to the "node" parameter? modules/javafx.graphics/src/main/java/javafx/scene/traversal/TraversalPolicy.java line 63: > 61: * <ol> > 62: * <li>Find the nearest parent of the "owner" that is handled by this > TraversalPolicy (i.e. it's a direct child of the root). > 63: * <li>select the next node within this direct child using the > context.selectInSubtree() and return it I'm going to need a second pass to understand what this does, but one thing that jumps out at me: what is "context" ? and what is the "selectInSubtree" method? I see no such method in the public API, so this description is not comprehensible (nor suitable for public documentation). modules/javafx.graphics/src/main/java/javafx/scene/traversal/TraversalPolicy.java line 96: > 94: * The constructor. > 95: */ > 96: public TraversalPolicy() { Abstract classes should have a `protected` not public constructor. And the docs should say "Constructor for subclasses to call." modules/javafx.graphics/src/main/java/javafx/scene/traversal/package-info.java line 28: > 26: /** > 27: * <p>Provides the set of classes for focus traversal.</p> > 28: */ This should have an `@since ` tag. Minor: I don't think the `<p>` HTML tag is needed. ------------- PR Review: https://git.openjdk.org/jfx/pull/1555#pullrequestreview-2293014279 PR Review Comment: https://git.openjdk.org/jfx/pull/1555#discussion_r1752279424 PR Review Comment: https://git.openjdk.org/jfx/pull/1555#discussion_r1752281034 PR Review Comment: https://git.openjdk.org/jfx/pull/1555#discussion_r1752283448 PR Review Comment: https://git.openjdk.org/jfx/pull/1555#discussion_r1752285689 PR Review Comment: https://git.openjdk.org/jfx/pull/1555#discussion_r1752288335 PR Review Comment: https://git.openjdk.org/jfx/pull/1555#discussion_r1752292570 PR Review Comment: https://git.openjdk.org/jfx/pull/1555#discussion_r1752292924 PR Review Comment: https://git.openjdk.org/jfx/pull/1555#discussion_r1752297334 PR Review Comment: https://git.openjdk.org/jfx/pull/1555#discussion_r1752302045 PR Review Comment: https://git.openjdk.org/jfx/pull/1555#discussion_r1752306901 PR Review Comment: https://git.openjdk.org/jfx/pull/1555#discussion_r1752308905 PR Review Comment: https://git.openjdk.org/jfx/pull/1555#discussion_r1752320230 PR Review Comment: https://git.openjdk.org/jfx/pull/1555#discussion_r1752305206 PR Review Comment: https://git.openjdk.org/jfx/pull/1555#discussion_r1752337828
