On Wed, 2 Oct 2024 17:33: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/ > > Andy Goryachev has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 14 commits: > > - Merge remote-tracking branch 'origin/master' into 8090456.focus.traversal > - removed traversal event > - javadoc > - remove bounds > - whitespace > - Merge remote-tracking branch 'origin/master' into 8090456.focus.traversal > - Merge remote-tracking branch 'origin/master' into 8090456.focus.traversal > - review comments > - Merge remote-tracking branch 'origin/master' into 8090456.focus.traversal > - review comments part 1 > - ... and 4 more: https://git.openjdk.org/jfx/compare/4d3c3661...9e1fa796 I haven't gotten involved in this PR so far, as I feel the discussion on the API on the mailinglist was left in an unfinished state, and I don't think there was much consensus. However, since I was asked to take a look, I've taken a few hours to look over the API only. I see two API's in this PR which I think can be provided one at a time (makes reviewing easier as well). First, an API to trigger a navigation action from anywhere. Second, the promotion of the internal traversal engine to "API". I have comments on both, but let me start with that I'm struggling to find the big use case for traversal policies that merits its inclusion. More on this later. # API to trigger navigation I think this API has merit. As it stands, FX is providing very nice navigation that can currently not be triggered programmatically, resulting in ugly work-arounds like re-implementing code that "hunts" for the correct `Node` or even uglier options like sending a fake `KeyEvent`. ## Implementation The implementation in this PR seems to suffer from promoting implementation to API. As a user, I don't see why `FocusTraversal` needs more than a `Node` and a `Direction`. `TraversalMethod` seems like an internal detail that I shouldn't have to care about. The API when called should always represent a programmatic navigation (which I guess is `DEFAULT`) and should not be used by FX when handling `KeyEvent`s (which I suppose it will then call with `KEY`). Furthermore, `TraversalDirection` has the odd `NEXT_IN_LINE` option which is also an exposed internal detail. It is hard to explain what it does, and why it would be useful, without getting into the internals, which is no good for an API. Finally, I think that this API should be on `Node`; this can be easily justified by the fact that nodes are focus aware, already have `requestFocus` and that this is using a static extension method pattern with `Node` as a required parameter. # TraversalPolicy API I struggle to see what problem this will be solving that isn't already solved with the above. Custom controls can listen to navigation keys (or leave either the logical or directional keys to bubble up) and call the above API to achieve their basic needs, while a completely different implementation would likely require manual node searching anyway, and so `Node::requestFocus` will suffice. - Redefining traversal on existing controls would make controls behave differently from the rest of the platform FX runs on - If such a redefinition was indeed a good idea still, then I'd expect this to be done globally (again as to not confuse users too much), and not on an instance basis; leaving it to the skin/behavior would make the most sense, and I'd argue that focus traversal is definitely behavior - Exposing TraversalPolicy separately adds another dimension to the Behavior/Control/Skin triad -- ie. you could have SkinA, BehaviorB and TraversalPolicyC -- I very much doubt that can be made to work without knowing intricate details of the Behavior and Skin - `TraversalPolicy` should be an interface with only a single method; an abstract partial implementation with helper methods can be built on top. It should be an interface rather than an abstract class since it has no state and defines only behavior. This is more flexible, and doesn't preclude providing an abstract base implementation. The interface definition will be much smaller and easier to understand as well. All in all, this part of the PR feels like an implementation being promoted to API, that primarily serves the needs of FX's current implementation, and not what users/developers need. ------------- PR Review: https://git.openjdk.org/jfx/pull/1555#pullrequestreview-2358571162
