Reflective discovery of styleable properties
Nir Lisker
nlisker at gmail.com
Thu Dec 7 09:46:46 UTC 2023
>
> I also like this route, perhaps slightly more, depending on how far this
> can be taken
>
Yes, how far is the initial question I had when initially proposing the
alternative Andy asked for.
A few points:
@Styleable(name = "-fx-label-padding", defaultValue = "(0, 0, 0, 0)")
...
The required converter can be derived from the generic type (Insets ->
> InsetsConverter), or specified (converter = InsetsConverter.class).
Specifying the defaultValue in the annotation duplicates the one in the
docs. This is minor, but we should keep it to one place. Specifying the
converter is an implementation detail, so I don't think this will work well
with a documented annotation. Maybe we shouldn't show the annotation then
and specify the property name in the docs instead.
The `isSettable` method is always the same boilerplate (and if not, you can
> use the old way).
>
I noticed this too. Maybe this can be fixed at the class rather than at the
processor.
To convert a regular property into a styleable does require somewhat more
> magic
>
We need to be careful with how much load we put on the annotation
processor or on reflection. I'm not sure if we can replace the whole
CSSMetadaData cleanly. I see that it has quite a lot of configuration.
On Wed, Dec 6, 2023 at 6:41 PM John Hendrikx <john.hendrikx at gmail.com>
wrote:
> You mean using an annotation processor?
>
> These can actually only create new types; modifying an existing one like
> Lombok is doing is illegal (the original source code with Lombok
> annotations is not plain Java anymore, hence an IDE plugin is required for
> IDE's to understand it).
>
> Still, a lot is possible. I really like RecordBuilder (
> https://github.com/Randgalt/record-builder) -- it provide builders for
> records (in a seperate type named <recordName>`Builder`) and if your record
> implements <recordName>`Builder.With`, you even get wither methods courtesy
> of the default methods on the generated interface
> <recordName>`Builder.With`.
>
> --John
> On 06/12/2023 14:42, Thiago Milczarek Sayão wrote:
>
> Hi,
>
> Maybe it could generate the boilerplate like Lombok does?
> So it has the benefits of eliminating error-prone boilerplate code and the
> benefits of not having the cost of reflection at runtime.
>
> -- Thiago.
>
>
> Em qua., 6 de dez. de 2023 às 10:28, John Hendrikx <
> john.hendrikx at gmail.com> escreveu:
>
>> I also like this route, perhaps slightly more, depending on how far this
>> can be taken:
>>
>> Disclaimer: I dislike the CSSMetaData and special property subclass a lot
>> -- it's a major concession to make properties work with the CSS system that
>> brings a lot of boilerplate that is mostly unnecessary.
>>
>> If you go the route of an annotation, we could offer to do everything via
>> the annotation so a regular property can be converted to styleable by
>> annotating it. The property would then have to specify the name
>> ("-fx-label-padding") and the default value (which can be a string that is
>> parseable via the existing CSS parser).
>>
>> In other words:
>>
>> @Styleable(name = "-fx-label-padding", defaultValue = "(0, 0, 0, 0)")
>> public final ReadOnlyObjectProperty<Insets> labelPaddingProperty() { ...
>> }
>> The rest the scanner can figure out (it could even figure out a default
>> name). The `isSettable` method is always the same boilerplate (and if not,
>> you can use the old way). The required converter can be derived from the
>> generic type (Insets -> InsetsConverter), or specified (converter =
>> InsetsConverter.class).
>>
>> To convert a regular property into a styleable does require somewhat more
>> magic (the CSS system would have to wrap it, or otherwise track them) but
>> that's all hidden. It gets rid of the StyleableProperty uglyness and the
>> need for users to create those, and puts the "extra" CSSMetaData
>> information a styleable property needs firmly where it belongs (in an
>> annotation).
>>
>> --John
>>
>> On 06/12/2023 11:37, Nir Lisker wrote:
>>
>> 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
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20231207/cf45aae4/attachment-0001.htm>
More information about the openjfx-dev
mailing list