On Mon, 17 Feb 2025 01:15:37 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
> 8350917: Allow parent nodes to provide CSS styleable properties for child
> nodes
modules/javafx.graphics/src/main/java/javafx/css/Styleable.java line 101:
> 99: * only for its direct children!
> 100: *
> 101: * @return an immutable list of CSS meta data, never {@code null}
Since we don't specify that `getCssMetaData()` should never return null, we
should consider also not specifying it here. Alternatively, maybe we should add
the non-null requirement to the existing `getCssMetaData()`.
modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java line 465:
> 463: }
> 464:
> 465: private record StylingContext(Node node, CalculatedValue font,
> StyleMap styleMap, Set<PseudoClass> pseudoClasses) {}
It might not be a good trade-off to create lots of transient objects on a hot
path just to save a few arguments in the calling convention. This would be a
nice improvement once we have value types in Java.
modules/javafx.graphics/src/main/java/javafx/scene/layout/Pane.java line 128:
> 126: @SuppressWarnings("unchecked")
> 127: StyleableObjectProperty<T> castProperty =
> (StyleableObjectProperty<T>) child.getProperties()
> 128: .computeIfAbsent(cssMetaData, k ->
> createChildConstraintProperty(child, cssMetaData));
Since we're on a hot path here, I'd like to see this work without unnecessary
memory allocations:
1. Check `hasProperties()` before calling `getProperties()`.
2. Don't use `computeIfAbsent()` since it requires a capturing lambda.
modules/javafx.graphics/src/main/java/javafx/scene/layout/Pane.java line 140:
> 138: }
> 139:
> 140: String propertyName = name + " (parent property)";
I think a more sensible naming scheme would be something like "VBox.margin",
which should be supplied as an argument and not generated on the fly.
modules/javafx.graphics/src/main/java/javafx/scene/layout/Pane.java line 234:
> 232: }
> 233:
> 234: if (value != null) {
How would the value ever be anything other than a `StyleableObjectProperty`,
since `setChildConstraint()` always creates a `StyleableObjectProperty`?
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1714#discussion_r1997679699
PR Review Comment: https://git.openjdk.org/jfx/pull/1714#discussion_r1997663655
PR Review Comment: https://git.openjdk.org/jfx/pull/1714#discussion_r1997683419
PR Review Comment: https://git.openjdk.org/jfx/pull/1714#discussion_r1997682667
PR Review Comment: https://git.openjdk.org/jfx/pull/1714#discussion_r1997684595
