RFR: 8290765: Remove parent disabled/treeVisible listeners [v4]

[Nir Lisker]

On Sat, 28 Jan 2023 16:24:30 GMT, Michael Strauß <mstrauss at openjdk.org> 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