8375588: Enhanced property metadata #2015
Conversation
|
👋 Welcome back mstrauss! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
Let me say that I'd really like a mechanism such as this to be part of FX, but I do have some questions: I think the I do however have a bit of problem with So to me, the attached part feels out of place for a low-level property API, unless there are compelling reasons and other use cases we'd envision outside of having CSS stylable child properties. If not, I don't see why we shouldn't make this a method on |
You can also specify it in the constructor of the
I think attached-ness is a fundamental aspect of properties, and making it visible in the type system is a logical choice. Compare this to WPF or Avalonia, where attached properties are also explicitly modelled. It's a piece of information that you can't reliably get from a property's metadata otherwise. |
I never used either framework, so "attached" is completely meaningless to me, and the description didn't enlighten me. What would this mean when I have just a bunch of properties (like a model)? It seems to me it is giving some extra information related to some kind of parent/child relationship that's outside the definition of the properties themselves. I'm only looking for a use case where this would be useful if I only have imported I did some checking, and this was suggested by AI:
|
|
An attached property is a property whose semantics are defined by one class, but whose value is associated with instances of another class. In other words, the declaring class "owns" the meaning, but the associated object owns the value. For example, This is not a parent/child relationship and it is not primarily about CSS, it's about meaning and context. For example, you could decorate arbitrary model objects with context-specific state: an entity could be "valid" as it relates to a specific validator or rule, it could be "selected" in a specific context, or it could carry metadata (constraints, tags, flags) that is only meaningful for a particular framework. JavaFX already has this concept today, but we're not using the If we expose attached properties with the It's important to note that the attached property model does not come with a scope model. If I set |
|
I removed the |
mstr2
force-pushed
the
feature/property-metadata
branch
from
8b725a6 to
a749597
Compare
mstr2
force-pushed
the
feature/property-metadata
branch
from
da503f0 to
731c68a
Compare
|
/reviewers 3 |
|
@mstr2 has indicated that a compatibility and specification (CSR) request is needed for this pull request. @mstr2 please create a CSR request for issue JDK-8375588 with the correct fix version. This pull request cannot be integrated until the CSR request is approved. |
Webrevs
|
I always found this design to be weird and unnecessary - these should be the properties/constraints of the container or the layout, not something stored in the child nodes. What happens when the said child is moved from one container to another?? It might be just me, but I fail to see the value of this proposal. |
This is not a good argument against this proposal, as JavaFX has always had attached properties and they're not going away. This just makes the existing concept more useful by using the |
|
Not going away - possibly. But it does not change the fact that these properties are stored in the wrong place. Adding some sort of band-aid code to help with some use cases seems a bit wrong to me. It does not mean that we don't have the actual problem. Perhaps you could describe a couple of use cases where the new API would shine, use cases where it is impossible to achieve the goal using the existing APIs. |
I would say that they are stored exactly where they belong, in the object which which they are associated. From an implementation standpoint, I agree with you that they are stored in the wrong place, namely in the
I already did:
As far as layout constraints go, they are not associated with the container. It doesn't make sense so say that the row index is a property of the grid; it's a property of an object within the grid. Similarly, it doesn't make sense to say that
Demonstrating impossibility is an unreasonable demand. In general, formalizing attached properties adds nothing conceptually, it merely straightens out rough edges of what's already there and gives a name to convention. |
|
Thank you for clarifications! I also suspect I did not get my point across about storing the layout properties in the node vs. the layout container. What happens when the code moves the node with these properties to another similar container? Whose responsibility it is to clear the unrelated properties and why is this needed in the first place? If the property (grid index, growth priority) makes no sense outside of the particular container, why is it stored in the node as opposed to let's say a hashtable within the layout container? This makes no sense to me, but okay, it's grandfathered like any other weird thing in FX. But why add more weird code on top of weird code? So I am not convinced: the use case you mentioned is just too abstract, and nothing tells me that it can't be implemented using existing APIs. What is the problem you are trying to solve, apart from some unnamed "tools"? These tools can simply query the |
This is basically the JavaFX way to attach layout information to nodes, used by .button {
-fx-hbox-margin: 10 10 10 10;
-fx-hbox-hgrow: ALWAYS;
}This is currently one of the biggest missing features when it comes to styling controls, having to go to programmatic configuration for these kinds of styling properties. |
|
Thank you @hjohn this example makes sense. So, if I understand it correctly, the proposal is to add two-tiered styleable properties? Are there limitations as to which attached property is legal for which node, or any attached property (including custom) can be attached to any node? I understand that CSS part is not there yet, but perhaps this PR should instead go via |
Nothing happens, just like for the past 15 years. The properties can live on there, until they're needed again when the node becomes part of a layout container.
This falls on the user, as they're the ones setting these properties, if they care (they don't stack up, there is a fixed number of them) and leaving them alone doesn't break anything. So, there is no need to clean them at all. It's exceedingly rare that a Node is re-used in such a way that this would matter (ie. moving a Node from a HBox to a GridPane, instead of what usually happens, which is just to recreate the Node).
Using a hash table actually makes this worse. Reparenting or temporarily removing a node would mean you also have to move or reconstruct its constraints, or lose them entirely. With attached properties, the metadata naturally travels with the node. In practice this case is rare anyway, so it shouldn’t carry much weight in the design discussion. Swing’s choices predate modern UI frameworks, and its designers were involved in creating JavaFX, so they clearly understood the trade-offs. Since then, almost all modern UI frameworks have converged on the attached-property style, including modern web layout via CSS. FX already follows this model, which is great as it was conceived over 15 years ago. What’s weird is not that layout metadata lives on the node, but that it’s exposed only through static setters instead of real, styleable, observable properties. Continuing this design would remove some of the weirdness, not add to it. It would also allow these properties to integrate cleanly with the CSS engine, opening up an entirely new way of controlling layout, where there exists a huge gap now. |
There can be limitations. For example, class HBox {
public static void setMargin(Node node, Insets value);
}This proposal formalizes a target-type restriction with the interface AttachedProperty {
Class<?> getTargetClass();
}If you're given a property instance, this allows you to then query the class of objects for which it can be set. |
It's not really two-tiered, or I am unsure what you mean by that. The properties are just coming from the container as a source, but they are still first class properties in all other respects. There aren't any limitations what a container could have as an attached property; the properties are container type specific, and other container types cannot accidentally get confused by them -- although we could perhaps share a few common property types among FX's containers; margin might be a good example (other frameworks also do this kind of sharing). So if you moved a node from GridPane to HBox, you wouldn't need to set the margin again, but may have to adjust container specific properties. |
hjohn
left a comment
hjohn
left a comment
There was a problem hiding this comment.
Hm, perhaps now that the idea here has had some time to sink in, perhaps the initial version with attachedness directly specified on the ReadOnlyProperty interface was better (also in light of the hoops we now need to jump through in the wrappers). Unless of course you think this is the better solution.
It seems to me that since we're already adding some weight to all the Simple property classes, and now additional mini-classes just to implement AttachedProperty, a solution where there is one or two default methods on ReadOnlyProperty is lighter, even if they may seem out of place (like I said, the idea needed some time to sink in, and there may be a good enough case to have this on simply all properties, UI related or not).
Best however to see what other reviewers think.
All in all, I think this is a good infrastructural change to build upon for making all static properties attached (I'm especially excited for HBox, VBox and GridPane), and I assume there will be follow-up PR's (where I could perhaps help) to modify these classes, as well and supporting these to be styleable by CSS.
The included HeaderBar changes may be better done separately. Also are you sure this will work for styleable properties as well?
modules/javafx.base/src/test/java/test/util/property/PropertyMetadataVerifier.java
Show resolved
Hide resolved
modules/javafx.base/src/test/java/test/util/property/PropertyMetadataVerifier.java
Outdated
Show resolved
Hide resolved
modules/javafx.base/src/main/java/javafx/beans/property/ReadOnlyBooleanWrapper.java
Outdated
Show resolved
Hide resolved
modules/javafx.base/src/main/java/javafx/beans/property/AttachedProperty.java
Show resolved
Hide resolved
| */ | ||
| String getName(); | ||
|
|
||
| /** |
There was a problem hiding this comment.
Should we add docs to getBean() as well?
If I understand correctly, the declaring class can be say HBox for an attached property, and the bean instance would then be a child of the hbox. The documentation for getBean then seems outdated, or could be clarified in light of this?
As it stands, one might think that getBean().getClass() == getDeclaringClass() (and it often is).
There was a problem hiding this comment.
getBean().getClass() == getDeclaringClass() is only true if the property is declared in the most derived class. For most JavaFX properties, this would not be the case. In any case, I've updated the documentation a bit.
| private class AttachedReadOnlyPropertyImpl extends ReadOnlyPropertyImpl implements AttachedProperty { | ||
|
|
||
| @Override | ||
| public Class<?> getTargetClass() { | ||
| return ((AttachedProperty)ReadOnlyBooleanWrapper.this).getTargetClass(); | ||
| } | ||
| } |
There was a problem hiding this comment.
This is clumsier than I had hoped.
There was a problem hiding this comment.
I think it's okay as it's a very localized implementation detail. You'll probably forget that it even exists very soon and move on.
| public SimpleBooleanProperty(Object bean, Class<?> declaringClass, String name, boolean initialValue) { | ||
| super(initialValue); | ||
| this.bean = bean; | ||
| this.declaringClass = declaringClass; | ||
| this.name = (name == null) ? DEFAULT_NAME : name; | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: I see this was already the case for existing, but just saying, I don't like the code duplication in the constructors.
There was a problem hiding this comment.
I've changed to telescoping constructor invocation where possible.
| } | ||
|
|
||
| var property = new SimpleObjectProperty<HeaderDragType>(child, "dragType"); | ||
| class PropertyImpl extends SimpleObjectProperty<HeaderDragType> implements AttachedProperty { |
There was a problem hiding this comment.
This seems to be primarily an infrastructure PR, we should consider to leave these specific changes out. I think there are many classes that could be adjusted, for which we could create a few grouped PR's.
I also noticed there is basically no users yet of the new getTargetClass method, I assume that was done deliberately as well.
There was a problem hiding this comment.
I included this as a small demonstration for an attached property implementation.
I don't think that there will be users of getTargetClass() in the JavaFX framework, at least at the moment (it didn't exist before, so why would there be users).
|
two-tiered: I meant one set of normally declared properties, the other set of properties that have a meaning only in the context of some layout container (and they must be cleared when the node is moved to another container). |
|
What do you think of without the Node having to implement |
I think the need for clearing is a bit overblown; currently when you do |
I agree; my point is that the property belongs in the container, not the child node. I know - grandfathered, nothing we can do at this point. |
The current state of affairs in FX matches much closer with most frameworks created after Swing, having the attached properties be part of the child, not the container. In other words, FX is the more modern and more flexible design here, and I'm sure the designers also drew upon lessons learned in this regard from Swing. |
|
Perhaps it could help to think of attached properties as simple configuration on the node, not as temporary state owned by a container. You may not know which container a node will end up in, but you can still set things like a margin, or tell it to grow if it’s placed in a VBox. You might do the same for other container types it could live in. When the node is actually added to a parent, that parent just reads the properties it understands and ignores the rest. From that point of view, it’s hard to justify why a container should remove properties it didn’t set. Auto-removal makes moving a node between parents lossy and surprising, because you lose configuration that was intentionally added. A simpler rule is: keep attached properties on the node, let containers use what applies to them, and only remove a property if the code that added it explicitly does so. |
I think it is the better solution, as it also provides a place for the
We're not adding weight to all simple property classes. With some sensible assumptions (memory alignment is not guaranteed, the VM is free to do anything it wants), the following classes will grow from 40 to 48 bytes: The following classes have enough unused space in their layout to accomodate the new field:
I don't see why it wouldn't. This PR doesn't add much functionality in any case. Making this work for styleable properties will require a fair bit of additional work, something along the lines of what you've proposed a few months ago. |
Attached properties are not exclusive to |
I understand that you don't like the idea of attached properties, and that you'd rather have narrow-scoped information colocated with layout containers. However, I'm glad that the JavaFX designers chose attached properties instead, as I think that this is a superior formalism. We're just finishing what they started. |
you could also have
How so? In fact, it completely separates the feature from whatever carrier is used. I really dislike this PR as it brings, in my opinion, unnecessary weight and code to the objects used everywhere for what seems to be a small side feature. I am ok with adding support to it, just not the way this PR does. I would say |
Yes, and that brings in But I think I'm not even clear on what you are proposing here. This PR does two things:
It seems like you don't object to the first addition, but only to the second addition. But what exactly does your proposed code do, then: Set<AttachedProperty> props = AttachedProperty.from(node);What are these |
|
@mstr2 : I don't understand the use case(s) then. @hjohn provided one: setting things like I do not understand why you need to add unrelated and unused fields and methods to the regular property classes that neither need nor use it. Again, if I understand it correctly. Does your JEP explain the problem in sufficient detail so even an old timer like me who is perpetually stuck in Swing can understand? |
Some enhancements are motivated by missing features, some are motivated by rough edges in a taxonomy. Consider the recent addition of pattern matching for primitives in Java: that's not a feature people were missing sorely, it's just what you get when you smooth out the rough edges. This PR falls into the second category: it smooths out rough edges in the JavaFX property system. Properties carry metadata to tell you about themselves: their name and the object to which they belong. The declaring class is a missing piece of information (and it's certainly not unrelated, as you say). Whatever criticism you have about why property metadata exists, your criticism should then also be levied against the existing "bean" and "name" metadata. The argument is the same. |
|
I've run into another scenario where having first-class styleable attached properties would make life much easier. I've placed a Actually getting hold of the tabs of a If we had styleable attached properties, I could just provide a stylesheet for my tab pane: .tab-pane {
-fx-headerbar-dragtype: transparent-subtree;
}
.tab-pane .tab {
-fx-headerbar-dragtype: none;
} |
3 participants
Implementation of enhanced property metadata.
New API
This PR includes the following API additions:
ReadOnlyProperty.getDeclaringClass()and its default implementation.javafx.beans.property.AttachedPropertyinterface.Simple<*>PropertyandReadOnly<*>Wrapperclasses, accepting the declaring class of the property.The declaring class is stored in a new field in the
Simple<*>Propertyclasses. If a legacy constructor is used that doesn't specify the declaring class, theReadOnlyProperty.getDeclaringClass()default implementation is called the first time theSimple<*>Property.getDeclaringClass()method is called, and its result is stored for future retrieval.Testing
For testing, this PR also includes the
test.util.property.PropertyMetadataVerifiertool. It systematically tests all public and protected properties of a class, and ensures conformance to the following rules:ReadOnlyProperty.getBean()returns the object instance of the enclosing class, or the target object instance if the property is an attached property.ReadOnlyProperty.getName()returns the name of the property, which must correspond to the name of the property getter (excluding the word "Property").ReadOnlyProperty.getDeclaringClass()returns the enclosing class of the property getter.Simple<*>PropertyorReadOnly<*>Wrappermust be specified in the constructor, not resolved at runtime.getBean(),getName(), andgetDeclaringClass()must not be overridden in subclasses ofSimple<*>PropertyorReadOnly<*>Wrapper.AttachedProperty.AttachedProperty.AttachedProperty.getTargetClass()returns the class of the single parameter of the static property getter.ReadOnly<*>Wrapper, it returns the result of callingReadOnly<*>Wrapper.getReadOnlyProperty().Many properties in existing JavaFX classes violate the
PropertyMetadataVerifierrules in some way or shape. This PR won't address these issues, this will be done in a future cleanup PR.Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/2015/head:pull/2015$ git checkout pull/2015Update a local copy of the PR:
$ git checkout pull/2015$ git pull https://git.openjdk.org/jfx.git pull/2015/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 2015View PR using the GUI difftool:
$ git pr show -t 2015Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/2015.diff
Using Webrev
Link to Webrev Comment