On Fri, 18 Aug 2023 13:39:59 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
>> This PR fixes an issue with the way `focusWithin` bits are adjusted in the
>> scene graph. Previously, the `focusWithin` counts of all parents of a
>> removed node would be decreased if the removed node has a non-zero
>> `focusWithin` count. This PR adds logic for the opposite scenario: the
>> `focusWithin` count is increased on all parents of a newly-added node that
>> is already focused (or contains other focused nodes).
>
> Michael Strauß has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Correctly handle added node with multiple focuses
LGTM
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 8329:
> 8327: set(true);
> 8328: } else if (count == 0) {
> 8329: set(false);
This looks a bit odd due to the use of two different variables, but it works
(`count > 0` would also have worked). I personally would favor something
without any if statements as it leaves less room for misinterpretation in this
case:
count += change;
set(count > 0);
modules/javafx.graphics/src/test/java/test/javafx/scene/FocusTest.java line
1146:
> 1144: node3 = new N(
> 1145: node4 = new N());
> 1146:
ignore: I think this looks a bit inconsistent (in the first version not all
parenthesis are closed on the last line, while they are in the second version).
Personally I favor this, similar to how curly braces are used):
Suggestion:
scene.setRoot(
node1 = new N(
node2 = new N()
)
);
node3 = new N(
node4 = new N()
);
-------------
Marked as reviewed by jhendrikx (Committer).
PR Review: https://git.openjdk.org/jfx/pull/1210#pullrequestreview-1585180011
PR Review Comment: https://git.openjdk.org/jfx/pull/1210#discussion_r1298838771
PR Review Comment: https://git.openjdk.org/jfx/pull/1210#discussion_r1298840452
