Re: RFR: 8290765: Remove parent disabled/treeVisible listeners [v5]
On Fri, 3 Feb 2023 23:31:24 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: > > improved tests, changed foreach loop Looks good to me too. - Marked as reviewed by arapte (Reviewer). PR: https://git.openjdk.org/jfx/pull/841
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners [v5]
On Fri, 3 Feb 2023 23:31:24 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: > > improved tests, changed foreach loop Marked as reviewed by jhendrikx (Committer). - PR: https://git.openjdk.org/jfx/pull/841
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 [v5]
On Fri, 3 Feb 2023 23:31:24 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: > > improved tests, changed foreach loop I missed that you can also change the foreach loop on the treeVisible method too, but it's not important. - Marked as reviewed by nlisker (Reviewer). PR: https://git.openjdk.org/jfx/pull/841
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners [v5]
On Fri, 3 Feb 2023 23:31:24 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: > > improved tests, changed foreach loop I've extended the tests to check for the other direction as well. @hjohn @nlisker can you re-review? - PR: https://git.openjdk.org/jfx/pull/841
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners [v5]
> `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: improved tests, changed foreach loop - Changes: - all: https://git.openjdk.org/jfx/pull/841/files - new: https://git.openjdk.org/jfx/pull/841/files/02577bba..316ec939 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=841=04 - incr: https://webrevs.openjdk.org/?repo=jfx=841=03-04 Stats: 14 lines in 2 files changed: 10 ins; 2 del; 2 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
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 [v2]
On Fri, 27 Jan 2023 22:01:46 GMT, John Hendrikx wrote: > Can this not be done in a way that doesn't require this `underInitialization` > flag? That's a good idea, and a also a good observation that the constructor only really sets `treeVisible` to `true`. I've updated the PR accordingly, which removes some of the complexity. There's no need for additional unit tests, as the expected behavior is already covered by `NodeTest.testIsTreeVisible`. - 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=841=03 - incr: https://webrevs.openjdk.org/?repo=jfx=841=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
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners [v2]
On Fri, 26 Aug 2022 00:22:54 GMT, Michael Strauß wrote:
> It's true that `Parent.getChildren()` must not return `null`, and that we
> shouldn't add null checks to protect against bugs.
>
> However, in this specific instance, `Node.setTreeVisible` is called from the
> constructor of `Node`. At that point in time, the `Parent` constructor has
> not yet run, so its final fields are still unassigned (and therefore `null`).
>
> The same problem exists for `SubScene.getRoot()` a few lines earlier, which
> is specified to never return `null`, but will indeed return `null` if called
> when the `SubScene` is under construction.
>
> But there's a serious problem with the null checking approach: it's calling a
> protected method at an unexpected time, before the constructor of a derived
> class has run. Since that's not what derived classes would expect, I have
> instead opted for a different solution, which includes a
> `underInitialization` flag that is passed to `Node.setTreeVisible`. When the
> flag is set, the calls to `SubScene.getRoot()` and `Parent.getChildren()` are
> elided (they'd return `null` anyway, so there's no point in calling these
> methods).
Can this not be done in a way that doesn't require this `underInitialization`
flag? The call in the constructor to `updateTreeVisible` looks misplaced, or
at least, won't do much; visible is going to be `true`, parent is gonna be
`null`, sub scene is going to be `null`... all that it is going to do is set
`treeVisible` to `true`, so why not just initialize the field that way?
Result of this code for an uninitialized `Node` is going to be a call to
`setTreeVisible(true)`, it won't do anything else:
private void updateTreeVisible(boolean parentChanged) {
boolean isTreeVisible = isVisible(); // is going to be true
final Node parentNode = getParent() != null ? getParent() ://
parentNode is going to be null
clipParent != null ? clipParent :
getSubScene() != null ? getSubScene() : null;
if (isTreeVisible) {
isTreeVisible = parentNode == null || parentNode.isTreeVisible();
// is going to be true
}
// When the parent has changed to visible and we have unsynchronized
visibility,
// we have to synchronize, because the rendering will now pass through
the newly-visible parent
// Otherwise an invisible Node might get rendered
if (parentChanged && parentNode != null && parentNode.isTreeVisible()
&& isDirty(DirtyBits.NODE_VISIBLE)) { // won't enter this if
addToSceneDirtyList();
}
setTreeVisible(isTreeVisible); // called with true
}
Then `setTreeVisible`:
final void setTreeVisible(boolean value) {
if (treeVisible != value) {// always enters if (as initial value of
treeVisible is false, and called with true)
treeVisible = value;
updateCanReceiveFocus(); // doesn't do anything, canReceiveFocus
was false initially and will be false again
focusSetDirty(getScene()); // doesn't do anything, scene == null
if (getClip() != null) { // doesn't do anything, clip == null
getClip().updateTreeVisible(true);
}
if (treeVisible && !isDirtyEmpty()) { // doesn't do anything,
dirty is empty
addToSceneDirtyList();
}
((TreeVisiblePropertyReadOnly) treeVisibleProperty()).invalidate();
// doesn't do anything, there are no listeners yet
if (Node.this instanceof SubScene) {
Node subSceneRoot = ((SubScene)Node.this).getRoot();
if (subSceneRoot != null) { // will always be null, we're
still initializing
// SubScene.getRoot() is only null if it's constructor
// has not finished.
subSceneRoot.setTreeVisible(value &&
subSceneRoot.isVisible());
}
}
}
}
End result of running all that code... `treeVisible` goes from `false` to
`true`.
-
PR: https://git.openjdk.org/jfx/pull/841
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners [v2]
On Thu, 25 Aug 2022 22:18:44 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 with a new target base due to a > merge or a rebase. The pull request now contains five commits: > > - Replace null check by explicit underConstruction flag > - Merge branch 'master' into fixes/node-listener-cleanup > - Merge > - added tests > - Remove Node.disabled and Node.treeVisible listeners I've resolved some merge conflicts with the latest `master` branch. @arapte can you re-review? - PR: https://git.openjdk.org/jfx/pull/841
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners [v3]
> `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 with a new target base due to a merge or a rebase. The pull request now contains seven commits: - added javadoc - Merge branch 'master' into fixes/node-listener-cleanup # Conflicts: #modules/javafx.graphics/src/main/java/javafx/scene/Node.java - Replace null check by explicit underConstruction flag - Merge branch 'master' into fixes/node-listener-cleanup - Merge - added tests - Remove Node.disabled and Node.treeVisible listeners - Changes: https://git.openjdk.org/jfx/pull/841/files Webrev: https://webrevs.openjdk.org/?repo=jfx=841=02 Stats: 88 lines in 3 files changed: 62 ins; 10 del; 16 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
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners [v2]
On Fri, 26 Aug 2022 03:43:49 GMT, Nir Lisker wrote: > I haven't looked at the code yet, but in general it's not considered a good > idea to expose an object before it's instantiated. Not sure if we have a > choice here though. I agree, that's why the `underConstruction` flag is used to prevent calling `getRoot()` and `getChildren()`, which would leak a partially constructed instance to derived classes. - PR: https://git.openjdk.org/jfx/pull/841
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners [v2]
On Thu, 25 Aug 2022 22:18:44 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 with a new target base due to a > merge or a rebase. The pull request now contains five commits: > > - Replace null check by explicit underConstruction flag > - Merge branch 'master' into fixes/node-listener-cleanup > - Merge > - added tests > - Remove Node.disabled and Node.treeVisible listeners I haven't looked at the code yet, but in general it's not considered a good idea to expose an object before it's instantiated. Not sure if we have a choice here though. - PR: https://git.openjdk.org/jfx/pull/841
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners [v2]
On Thu, 25 Aug 2022 22:18:44 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 with a new target base due to a > merge or a rebase. The pull request now contains five commits: > > - Replace null check by explicit underConstruction flag > - Merge branch 'master' into fixes/node-listener-cleanup > - Merge > - added tests > - Remove Node.disabled and Node.treeVisible listeners It's true that `Parent.getChildren()` must not return `null`, and that we shouldn't add null checks to protect against bugs. However, in this specific instance, `Node.setTreeVisible` is called from the constructor of `Node`. At that point in time, the `Parent` constructor has not yet run, so its final fields are still unassigned (and therefore `null`). The same problem exists for `SubScene.getRoot()` a few lines earlier, which is specified to never return `null`, but will indeed return `null` if called when the `SubScene` is under construction. But there's a serious problem with the null checking approach: it's calling a protected method at an unexpected time, before the constructor of a derived class has run. Since that's not what derived classes would expect, I have instead opted for a different solution, which includes a `underConstruction` flag that is passed to `Node.setTreeVisible`. When the flag is set, the calls to `SubScene.getRoot()` and `Parent.getChildren()` are elided (they'd return `null` anyway, so there's no point in calling these methods). - PR: https://git.openjdk.org/jfx/pull/841
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners [v2]
> `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 with a new target base due to a merge or a rebase. The pull request now contains five commits: - Replace null check by explicit underConstruction flag - Merge branch 'master' into fixes/node-listener-cleanup - Merge - added tests - Remove Node.disabled and Node.treeVisible listeners - Changes: https://git.openjdk.org/jfx/pull/841/files Webrev: https://webrevs.openjdk.org/?repo=jfx=841=01 Stats: 81 lines in 4 files changed: 53 ins; 10 del; 18 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
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners
On Thu, 25 Aug 2022 13:50:19 GMT, Nir Lisker wrote: >> It's definitely a hypothetical case. `getChildren()` is called all over the >> place in JavaFX without a null check, so I see no reason for null checks >> here. > > Technically correct, although the [doc of > `getChildren()`](https://openjfx.io/javadoc/18/javafx.graphics/javafx/scene/Parent.html#getChildren()) > specifically says: > >> Note to subclasses: if you override this method, you must return from your >> implementation the result of calling this super method. The actual list >> instance returned from any getChildren() implementation must be the list >> owned and managed by this Parent. The only typical purpose for overriding >> this method is to promote the method to be public. > > So considering the case that an overriding method will return `null` is not > practical. > > As for a `null` child, I think that that's also not allowed. Considering that > a child can be added at most once to a scenegraph, it would mean that only 1 > `null` child is allowed. There might be more issues with `null` children. I > don't think there's a good reason to check for them. Yep, right. While it is possible to override `getChildren()` and return null, it will break everywhere so we can also omit the null check here. - PR: https://git.openjdk.org/jfx/pull/841
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners
On Thu, 25 Aug 2022 13:46:26 GMT, Kevin Rushforth wrote: >> Since `getChildren()` is not final, one can easily override it and return >> null. >> Therefore, this check should still be done here. >> Maybe we even need to check that every child is not null, since again I can >> override `getChildren()` and return a list that allows null. (The default >> implementation in `Parent` throws an exception when adding a null child) >> >> Interestingly, the javadoc of `getChildren()` states that one should call >> the super method when overriding (although this can't be enforced). So this >> is probably a hypothetical case. > > It's definitely a hypothetical case. `getChildren()` is called all over the > place in JavaFX without a null check, so I see no reason for null checks here. Technically correct, although the [doc of `getChildren()`](https://openjfx.io/javadoc/18/javafx.graphics/javafx/scene/Parent.html#getChildren()) specifically says: > Note to subclasses: if you override this method, you must return from your > implementation the result of calling this super method. The actual list > instance returned from any getChildren() implementation must be the list > owned and managed by this Parent. The only typical purpose for overriding > this method is to promote the method to be public. So considering the case that an overriding method will return `null` is not practical. As for a `null` child, I think that that's also not allowed. Considering that a child can be added at most once to a scenegraph, it would mean that only 1 `null` child is allowed. There might be more issues with `null` children. I don't think there's a good reason to check for them. - PR: https://git.openjdk.org/jfx/pull/841
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners
On Thu, 25 Aug 2022 13:36:01 GMT, Marius Hanl wrote:
>> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1919:
>>
>>> 1917: }
>>> 1918: }
>>> 1919: }
>>
>> Because we can use Java 17 now, you can use pattern matching for
>> `instanceof`. Also, from what I see, `getChildren()` can never return
>> `null`. So, we can write
>>
>> if (Node.this instanceof Parent parent) {
>> parent.getChildren().forEach(child -> child.updateDisabled());
>> }
>
> Since `getChildren()` is not final, one can easily override it and return
> null.
> Therefore, this check should still be done here.
> Maybe we even need to check that every child is not null, since again I can
> override `getChildren()` and return a list that allows null. (The default
> implementation in `Parent` throws an exception when adding a null child)
>
> Interestingly, the javadoc of `getChildren()` states that one should call the
> super method when overriding (although this can't be enforced). So this is
> probably a hypothetical case.
It's definitely a hypothetical case. `getChildren()` is called all over the
place in JavaFX without a null check, so I see no reason for null checks here.
-
PR: https://git.openjdk.org/jfx/pull/841
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners
On Thu, 25 Aug 2022 12:33:51 GMT, Nir Lisker 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.
>
> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1919:
>
>> 1917: }
>> 1918: }
>> 1919: }
>
> Because we can use Java 17 now, you can use pattern matching for
> `instanceof`. Also, from what I see, `getChildren()` can never return `null`.
> So, we can write
>
> if (Node.this instanceof Parent parent) {
> parent.getChildren().forEach(child -> child.updateDisabled());
> }
Since `getChildren()` is not final, one can easily override it and return null.
Therefore, this check should still be done here.
Maybe we even need to check that every child is not null, since again I can
override `getChildren()` and return a list that allows null. (The default
implementation in `Parent` throws an exception when adding a null child)
-
PR: https://git.openjdk.org/jfx/pull/841
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners
On Thu, 21 Jul 2022 04:43:15 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.
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1919:
> 1917: }
> 1918: }
> 1919: }
Because we can use Java 17 now, you can use pattern matching for `instanceof`.
Also, from what I see, `getChildren()` can never return `null`. So, we can write
if (Node.this instanceof Parent parent) {
parent.getChildren().forEach(child -> child.updateDisabled());
}
-
PR: https://git.openjdk.org/jfx/pull/841
Re: RFR: 8290765: Remove parent disabled/treeVisible listeners
On Thu, 21 Jul 2022 04:43:15 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. Tested with some layout and controls. LGTM. - Marked as reviewed by arapte (Reviewer). PR: https://git.openjdk.org/jfx/pull/841
RFR: 8290765: Remove parent disabled/treeVisible listeners
`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. - Commit messages: - Merge - added tests - Remove Node.disabled and Node.treeVisible listeners Changes: https://git.openjdk.org/jfx/pull/841/files Webrev: https://webrevs.openjdk.org/?repo=jfx=841=00 Issue: https://bugs.openjdk.org/browse/JDK-8290765 Stats: 69 lines in 3 files changed: 57 ins; 10 del; 2 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
