Reflective enumeration of styleable properties #1299
Conversation
|
👋 Welcome back mstrauss! A progress list of the required criteria for merging this PR into |
| } | ||
|
|
||
| private static final List<CssMetaData<? extends Styleable, ?>> classCssMetaData = CssMetaData.of(new Node() {}); |
There was a problem hiding this comment.
perhaps it should be <S extends Styleable> CssMetaData.of(Class<S>) ?
|
|
||
| @Override | ||
| public StyleableProperty<BlendMode> getStyleableProperty(Node node) { | ||
| return (StyleableProperty<BlendMode>)node.blendModeProperty(); |
There was a problem hiding this comment.
these two methods can be implemented in the base class if we add a
setGetter(Function<S, StyleableProperty<V>> getter)
and also
setIsSettable(Predicate<S>)
?
This way we don't even need to declare a new class each time.
Just a thought.
There was a problem hiding this comment.
Yes, this can be further simplified.
| private CssMetaDataCache() {} | ||
|
|
||
| public synchronized static List<CssMetaData<? extends Styleable, ?>> getCssMetaData(Styleable node) { | ||
| List<CssMetaData<? extends Styleable, ?>> list = cache.get(node.getClass()); |
There was a problem hiding this comment.
wouldn't this fail in the case of a skin change?
There was a problem hiding this comment.
Control/Skin metadata is also cached in the Control class. This particular method will only discover the styleable properties declared on the specified node, but not on its skin.
| @@ -1364,7 +1364,30 @@ public final boolean isVisible() { | |||
| public final BooleanProperty visibleProperty() { | |||
| if (visible == null) { | |||
| visible = new StyleableBooleanProperty(true) { | |||
| static final CssMetaData<Node, Boolean> METADATA = new CssMetaData<>( | |||
There was a problem hiding this comment.
One problem: this idea requires creation of an instance, bc METADATA static objects get created when an instance is created.
I'd rather avoid this inversion...
There was a problem hiding this comment.
We're fully inflating a single instance of each particular class once to gather all metadata. After that, all further instances don't need to be fully inflated.
There was a problem hiding this comment.
what if this is an abstract custom Node/Control?
There was a problem hiding this comment.
We would never expect to encounter an instance of an abstract class. Generally, metadata reflection is done the first time we call getCssMetaData for any particular class, which we can only do if we have an instance of that class.
The only exceptions to this are the preexisting static getClassCssMetaData methods, which are initialized once with a throwaway instance of the class they are declared in, for example in the case of the Node class:
private static final List<CssMetaData<? extends Styleable, ?>> classCssMetaData =
CssMetaData.of(new Node() {});New classes should not copy this pattern of having a static getClassCssMetaData method, so the problem shouldn't exist for these classes.
| return node.nodeTransformation == null | ||
| || node.nodeTransformation.translateX == null | ||
| || node.nodeTransformation.canSetTranslateX(); | ||
| } |
There was a problem hiding this comment.
this is an example of a more complex isSettable() logic.
|
@mstr2 This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
|
❗ This change is not yet ready to be integrated. |
|
@mstr2 This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
Progress
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1299/head:pull/1299$ git checkout pull/1299Update a local copy of the PR:
$ git checkout pull/1299$ git pull https://git.openjdk.org/jfx.git pull/1299/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1299View PR using the GUI difftool:
$ git pr show -t 1299Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1299.diff