Re: RFR: 8290765: Remove parent disabled/treeVisible listeners [v4]
On Fri, 3 Feb 2023 22:27:26 GMT, Nir Lisker wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Initialize treeVisible field directly
>
> 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.
Done.
> 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.
Done.
> 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.
Done.
-
PR: https://git.openjdk.org/jfx/pull/841
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners [v4]
On Sat, 28 Jan 2023 16:24:30 GMT, Michael Strauß 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 I took a quick look and it seems fine to me. I recommend to wait for a day or two to see if @arapte has any additional comments on the changes (since his review is stale). - PR: https://git.openjdk.org/jfx/pull/841
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners [v4]
On Sat, 28 Jan 2023 16:24:30 GMT, Michael Strauß 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
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners [v4]
On Sat, 28 Jan 2023 16:24:30 GMT, Michael Strauß 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 Marked as reviewed by jhendrikx (Committer). - PR: https://git.openjdk.org/jfx/pull/841
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners [v4]
> `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 - Changes: - all: https://git.openjdk.org/jfx/pull/841/files - new: https://git.openjdk.org/jfx/pull/841/files/fe803883..02577bba Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=841&range=03 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=841&range=02-03 Stats: 50 lines in 3 files changed: 1 ins; 26 del; 23 mod Patch: https://git.openjdk.org/jfx/pull/841.diff Fetch: git fetch https://git.openjdk.org/jfx pull/841/head:pull/841 PR: https://git.openjdk.org/jfx/pull/841
