[External] : Re: Reflective discovery of styleable properties
Nir Lisker
nlisker at gmail.com
Thu Dec 7 09:53:56 UTC 2023
>
> - 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 <andy.goryachev at oracle.com>
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 <nlisker at gmail.com>
> *Date: *Wednesday, December 6, 2023 at 02:37
> *To: *Andy Goryachev <andy.goryachev at oracle.com>
> *Cc: *Michael Strauß <michaelstrau2 at gmail.com>, openjfx-dev <
> openjfx-dev at openjdk.org>
> *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 <andy.goryachev at oracle.com>
> 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 <openjfx-dev-retn at openjdk.org> on behalf of Michael
> Strauß <michaelstrau2 at gmail.com>
> *Date: *Sunday, December 3, 2023 at 22:02
> *To: *openjfx-dev <openjfx-dev at openjdk.org>
> *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$>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20231207/7af55c5e/attachment-0001.htm>
More information about the openjfx-dev
mailing list