On Fri, 12 Mar 2021 15:32:10 GMT, Florian Kirmaier <[email protected]>
wrote:
>> I think others can review this. I do have a couple questions:
>> 1. In general, I don't like the idea of just making everything a weak
>> reference, since it often masks a design issue. Two exceptions are for
>> caches and for back references to a "parent" or controlling object that has
>> a strong reference to "this" object (most of our weak listeners fall into
>> this latter pattern). It sounds like latter case also applies here. Can you
>> confirm that?
>> 2. Have you verified that all the places that use the fields that are now
>> WeakReferences are prepared to deal with `get()` returning a null object?
>
> About whether we should use WeakReference here or not.
>
> It definitely falls into the exception for referring to the Parrent of a
> Node. (Or to the Parent in a more abstract sense, I think it can also be the
> parent of the parent, or even from another SceneGraph.)
>
> I don't know the CSS code very well, so I currently don't have the knowledge
> how to change it.
> But if we would change this variable, whenever the node is removed from the
> SceneGraph, my concern would be that it would have an unfavorable complexity.
> Currently (I hope) the complexity of removing a Node from the SceneGraph is
> O(1). If we would remove the stylehelper from the node, then the complexity
> would be O(<nodes-in-the-tree>).
>
> The current change assumes that we don't process the CSS, when a node is
> removed from the SG. This is why it isn't checked for null. I would argue, if
> this causes an Error, then it just reveals another issue, which would be
> preferable over a more complicated fix, and also changing the complexity of
> removing nodes from the SG.
Below is a fix that I tried, It fixes the issue. The system test passed with
this change.
I was suggesting a solution like this where we can know exactly when to `null`
the reference. The change is not extensively tested though. (For example, test
what happens if we remove a node and add it back, OR remove a node and add it
to a different scenegraph)
However, in this case using `WeakReference` approach seems harmless. Using
`WeakReference` for Listeners is not clean and may cause issues, but a
`WeakReference` to Node should not cause any harm.
I would recommend earlier way to explicitly release references/resources when
it is possible. That way we get to have the control instead of gc.
diff --git
a/modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java
b/modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java
index fd02c0c1ad..471d0071b5 100644
--- a/modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java
+++ b/modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java
@@ -36,6 +36,7 @@ import java.util.Set;
import javafx.beans.property.ObjectProperty;
import javafx.beans.property.SimpleObjectProperty;
+import javafx.beans.value.ChangeListener;
import javafx.beans.value.WritableValue;
import com.sun.javafx.css.CascadingStyle;
import javafx.css.CssMetaData;
@@ -82,6 +83,14 @@ final class CssStyleHelper {
this.triggerStates = new PseudoClassState();
}
+ ChangeListener<Scene> sceneChangeListener;
+
+ static void dispose(CssStyleHelper styleHelper, Node node) {
+ styleHelper.resetToInitialValues(node);
+ styleHelper.firstStyleableAncestor = null;
+ node.sceneProperty().removeListener(styleHelper.sceneChangeListener);
+ }
+
/**
* Creates a new StyleHelper.
*/
@@ -158,7 +167,7 @@ final class CssStyleHelper {
// If this node had a style helper, then reset properties to
their initial value
// since the node won't have a style helper after this call
if (node.styleHelper != null) {
- node.styleHelper.resetToInitialValues(node);
+ dispose(node.styleHelper, node);
}
//
@@ -181,8 +190,14 @@ final class CssStyleHelper {
// If this node had a style helper, then reset properties to their
initial value
// since the style map might now be different
if (node.styleHelper != null) {
- node.styleHelper.resetToInitialValues(node);
+ dispose(node.styleHelper, node);
}
+ helper.sceneChangeListener = (ov, oldScene, newScene) -> {
+ if (newScene == null) {
+ helper.firstStyleableAncestor = null;
+ }
+ };
+ node.sceneProperty().addListener(helper.sceneChangeListener);
return helper;
}
-------------
PR: https://git.openjdk.java.net/jfx/pull/424
