> > - I would recommend against the scanner figuring out the property name: > the property > names are codified by the CSS reference which serves as a > normative document in this case >
I think that the CSS reference should be generated from the source code, which is something the annotation processor can do. - isSettable() logic might be more complex than (prop == null && > !(prop.isBound)), see Node.translateXProperty. How would that work with > annotations? >From what John said, you can fall back to the CSSMetaData way if the defaults are not good for you. On Wed, Dec 6, 2023 at 6:21 PM Andy Goryachev <[email protected]> wrote: > I also like this idea very much. Still needs a reflective scanner, but > it's far more easier to understand and use. > > > > A couple of comments/questions: > > > > - I would recommend against the scanner figuring out the property name: > the property names are codified by the CSS reference which serves as a > normative document in this case > > > > - isSettable() logic might be more complex than (prop == null && > !(prop.isBound)), see Node.translateXProperty. How would that work with > annotations? > > > > What do you think? > > > > -andy > > > > > > > > *From: *Nir Lisker <[email protected]> > *Date: *Wednesday, December 6, 2023 at 02:37 > *To: *Andy Goryachev <[email protected]> > *Cc: *Michael Strauß <[email protected]>, openjfx-dev < > [email protected]> > *Subject: *[External] : Re: Reflective discovery of styleable properties > > I thought about the option of reflection, but I opted to propose > annotations instead. The following is my reasoning. > > > > Firstly, reflection is very magic-y. The author of the class has no > indication of what side effects happen due to the code they write, the > output (css handling in this case) comes out of nowhere from their > perspective. As with other reflection cases, it is a "pull" rather than > "push" approach - you don't write what should happen, you let someone else > decide that. For writers of skin/control classes, this means that they need > to know exactly what constitutes a hook for the reflection mechanism, or > face surprises. There is no compile time check that tells you whether you > have declared your styleable property properly or not (without an external > ad-hoc checker). > > We do this somewhat with properties - any method of the form > "...property()" gets special treatment, but this is for the docs. I don't > think we have code that depends on this other than in tests. > > > > Secondly, the proposed mechanism depends on the runtime type, not the > declared type. As a user, I see no indication in the API whether a property > is styleable or not. This is also (what I would consider) a problem with > the current state. When I thought about using reflection to solve this, I > at least thought to specify the declared type of the property as styleable, > like StyleableBooleanProperty instead of BooleanProperty (restricting the > returned type is backwards compatible). A downside of this is that it gives > access to the methods of StyleableProperty, which are not useful for the > user, I think, but maybe someone has a use for them. > > > > Thirdly, maybe I want to declare a styleable property not to be used > automatically. I can't think off the top of my head when I would want to do > that, but I'm also not a heavy css user. Are we sure that just initializing > a property with a styleable runtime type should *always* be caught by this > process? > > > > To compare, annotations have the following benefits: > > > > Firstly, they are declarative, which means no surprises for the class > author (WYSIWYG). This also allows more flexibility/control over which > properties get special treatment via an opt-in mechanism. > > > > Secondly, They can also be displayed in the JavaDocs (via @Documented) > with their assigned values. For example, the padding property of Region can > be annotated with @Styleable(property="-fx-padding"), informing the user > both that this value can be set by css, and how to do it. Interestingly, > the annotation doesn't need to be public API to be displayed, so we are not > bound by contracts. > > > > In terms of similarities: > > > > In both the reflection and the annotation proposals, the steps are: > > 1. Create styleable properties. > 2. That's it. > > It's just that step 1 also adds an annotation to the creation of the > property (which was/is a 2-step process anyway, declaring the property and > its css metadata). > > > > Annotations also require a processor to read the data from their values > and target (the field/method). This is a bit of work, but > Michael's CssMetaDataCache class is basically that - read the data from the > class (via reflection or annotations) and store it in a map. The logic > should be the same, just the method to obtain the data is different. Both, > as a result, have the benefits of handling control/skin combinations (what > I mentioned in the point "Usable both in controls and in skins (or other > classes)"). > > > > The benefit of co-locating the property and its css metadata in the class > itself also remains. > > > > > > To summarize, both approaches eliminate all the clutter of writing > styleable properties (John, will you like to create styleable properties > now? [1] :) ), both apply the flexibility of caching per class, both allow > better structuring of the class, but they read the properties differently > and have a different level of declarativness. > > > > [1] > https://mail.openjdk.org/pipermail/openjfx-dev/2023-December/044010.html > > > > > > On Tue, Dec 5, 2023 at 11:21 PM Andy Goryachev <[email protected]> > wrote: > > I like the idea. > > > > I wonder if it is possible to reduce the amount of boilerplate code? For > example, a CssMetaData can have a > > > > setGetter(Function<S, StyleableProperty<V>> getter) > > > > method which supplies the property reference? This way > CssMetaData.isSettable(Node) and CssMetaData.getStyleableProperty(Node) can > be implemented in the base class (there are more complicated cases, so > perhaps setIsSettable(Predicate<Node>) would also be required). > > > > Example: > > > > CssMetaData.<ControlExample,Font>of("-fx-font", Font.getDefault(), (n) -> > n.font) > > > > Just a thought. What do you think? > > > > -andy > > > > > > *From: *openjfx-dev <[email protected]> on behalf of Michael > Strauß <[email protected]> > *Date: *Sunday, December 3, 2023 at 22:02 > *To: *openjfx-dev <[email protected]> > *Subject: *Reflective discovery of styleable properties > > Following up the discussion around the CssMetaData API, I'd like to > chime in with yet another idea. To recap, here's Nir's summary of the > current API [0]: > > "Let's look at what implementation is required from a user who wants > to write their own styleable control: > 1. Create styleable properties. > 2. Create a list of these properties to be passed on. > 3. Create a public static method that returns the concatenation of > this list with the one of its parent. (This method happens to be > poorly documented, as mstr said.) > 4. Create a public non-static method that calls the static method in a > forced-override pattern because otherwise you will be calling the > wrong static method. (This method's docs seem to be just wrong because > you don't always want to delegate to Node's list.)" > > > I think this could reasonably be replaced with the following > implementation requirements: > 1. Create styleable properties. > 2. That's it. > > Let's look at what we're actually trying to do: create a list of > CSS-styleable property metadata of a class. But we can easily do that > without all of the boilerplate code. > > When ´Node.getCssMetaData()` is invoked, all public methods of the > class are reflectively enumerated, and metadata is retrieved from > `Property` and `StyleableProperty` getters. This is a price that's > only paid once for any particular class (i.e. not for every instance). > The resulting metadata list is cached and reused for all instances of > that particular class. > > As a further optimization, metadata lists are also cached and > deduplicated for Control/Skin combinations (currently every Control > instance has its own copy of the metadata list). > > Another benefit of this approach is that the CssMetaData can now be > co-located with the property implementation, and not be kept around in > other parts of the source code file. Here's how that looks like when a > new "myValue" property is added to MyClass: > > StyleableDoubleProperty myValue = > new SimpleStyleableDoubleProperty(this, "myValue") { > > static final CssMetaData<MyClass, Number> METADATA = > new CssMetaData<MyClass, Number>( > "-fx-my-value", > SizeConverter.getInstance(), > USE_COMPUTED_SIZE) { > @Override > public boolean isSettable(MyClass node) { > return !node.myValue.isBound(); > } > > @Override > public StyleableProperty getStyleableProperty( > MyClass node) { > return node.myValue; > } > }; > > @Override > public CssMetaData getCssMetaData() { > return METADATA; > } > }; > > public final DoubleProperty myValueProperty() { > return myValue; > } > > It is not required to override the `getCssMetaData()` method, nor is > it required to redeclare a new static `getClassCssMetaData()` method. > It is also not required to manually keep the list of styleable > properties in sync with the list of CSS metadata. > > I've prototyped this concept for the `Node`, `Region` and `Control` > classes [1]. > > [0] > https://mail.openjdk.org/pipermail/openjfx-dev/2023-December/044046.html > [1] https://github.com/openjdk/jfx/pull/1299 > <https://urldefense.com/v3/__https:/github.com/openjdk/jfx/pull/1299__;!!ACWV5N9M2RV99hQ!K7nDyvMP0PzEOLu-h9yGCoHIoSnny6LJ5acSISP81wBjJjP2z4VcDA6CIMU_Wvzxv2QJgPTsB6F9wtnzMK97$> > >
