On Sat, 28 Jan 2023 16:24:30 GMT, Michael Strauß <[email protected]> wrote:
>> `Node` adds InvalidationListeners to its parent's `disabled` and
>> `treeVisible` properties and calls its own `updateDisabled()` and
>> `updateTreeVisible(boolean)` methods when the property values change.
>>
>> These listeners are not required, since `Node` can easily call the
>> `updateDisabled()` and `updateTreeVisible(boolean)` methods on its children,
>> saving the memory cost of maintaining listeners and bindings.
>
> Michael Strauß has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Initialize treeVisible field directly
Looks good. I ran the tests and they pass. Left a few optional comments.
Maybe @kevinrushforth will want to have a look too before you merge.
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1917:
> 1915: for (Node child : parent.getChildren()) {
> 1916: child.updateDisabled();
> 1917: }
If you like this style, you could do
`parent.getChildren().forEach(Node::updateDisabled);`. I find these more
readable, but it's up to you.
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 2608:
> 2606: // PerformanceTracker.logEvent("Node.postinit " +
> 2607: // "for [{this}, id=\"{id}\"]
> finished");
> 2608: //}
Not sure if these comments are intended to be used for something. I doubt it
though.
modules/javafx.graphics/src/test/java/test/javafx/scene/NodeTest.java line 2024:
> 2022:
> 2023: }
> 2024:
Since you changed the copyright year on the other files, you might want to do
this one too. There is a script that will do it anyway, so it doesn't matter.
modules/javafx.graphics/src/test/java/test/javafx/scene/NodeTest.java line 2039:
> 2037: assertTrue(n2.isDisabled());
> 2038: assertTrue(n2.isDisabled());
> 2039: }
I suggest to continue this test with a `g.setDisable(false)` to check both
directions. The first group of assertions test the initial state, not a change
to `false`.
Same for the other test.
-------------
Marked as reviewed by nlisker (Reviewer).
PR: https://git.openjdk.org/jfx/pull/841
