possible (I could be
wrong).
What do you think?
-andy
From: openjfx-dev on behalf of Nir Lisker
Date: Friday, December 1, 2023 at 20:35
To: John Hendrikx
Cc: openjfx-dev@openjdk.org
Subject: Re: CssMetaData.combine()
John answered already to most of the points, but I want to give my own insights
On 02/12/2023 05:35, Nir Lisker wrote:
Lazy initialization in many places that IMHO is not needed
I noticed this for the first time in classes like Box, Sphere and
Cylinder. Their dimension properties are lazily initialized, but are
also initialized on construction, so I never understoo
John answered already to most of the points, but I want to give my own
insights as well.
> Even though the syntax is ugly, the current implementation of the static
> getClassCssMetaData() is *nearly perfect*, considering the lack of some
> kind of a 'lazy' keyword in java.
>
I don't buy Nir's arg
Hi Andy,
Let me start to say that I had no problem with this PR being merged as I
already agreed with one of the first versions.
Sometimes then on the same PR there can be some discussions on what else
can be done in this area, potentially maybe even alleviating the need
for the change (X/Y
with the
proposed utility method (and maybe we can address some other comments from
https://git.openjdk.org/jfx/pull/1293#discussion_r1411406802 ).
I do like what Michael suggested - is to provide a (better) explanation for
CssMetaData.combine(), including the requirements on the user code that
```java
> > private static final List>
> > STYLEABLES = CssMetaData.combine(
> > Node.getClassCssMetaData(),
> > FIT_HEIGHT,
> > FIT_WIDTH,
> > IMAGE,
> > PRESERVE_RATIO,
> > SMOOTH
> > )
gt;> {@link Node#getClassCssMetaData()}.
>> *
>> * Example:
>> *
>> * private static final List<CssMetaData<? extends Styleable, ?>>
>> STYLEABLES = CssMetaData.combine(
>> * <Parent>.getClassCssMetaData(),
>> * STYLE
gt;> {@link Node#getClassCssMetaData()}.
>> *
>> * Example:
>> *
>> * private static final List<CssMetaData<? extends Styleable, ?>>
>> STYLEABLES = CssMetaData.combine(
>> * <Parent>.getClassCssMetaData(),
>> * STYLE
gt;> {@link Node#getClassCssMetaData()}.
>> *
>> * Example:
>> *
>> * private static final List<CssMetaData<? extends Styleable, ?>>
>> STYLEABLES = CssMetaData.combine(
>> * <Parent>.getClassCssMetaData(),
>> * STYLE
Example:
> *
> * private static final List<CssMetaData<? extends Styleable, ?>>
> STYLEABLES = CssMetaData.combine(
> * <Parent>.getClassCssMetaData(),
> * STYLEABLE1,
> * STYLEABLE2
> * );
> *
>
On Wed, 29 Nov 2023 21:21:43 GMT, Andy Goryachev wrote:
> 3. Adding two pointers to every Node is not a good solution, in my opinion.
> Replacing lazy initialization with a busy one is not a good solution. Am I
> missing something here?
Given that it is a huge hassle to implement your own CS
On Wed, 29 Nov 2023 21:09:37 GMT, Nir Lisker wrote:
> But it's limited because it has been artificially limited. It's a special
> case of ` List combine(List, T... items)`, which is a general
> utility method. Just duplicating this method for specific `T`'s doesn't make
> it more fitting for t
gt;> {@link Node#getClassCssMetaData()}.
>> *
>> * Example:
>> *
>> * private static final List<CssMetaData<? extends Styleable, ?>>
>> STYLEABLES = CssMetaData.combine(
>> * <Parent>.getClassCssMetaData(),
>> * STYLE
On Wed, 29 Nov 2023 15:54:17 GMT, Andy Goryachev wrote:
> > I'm unconvinced that this is the way to solve the issue it describes in its
> > doc. The problem is that each control wants to add its own css metadata to
> > that of its parent (recursively), and that the list is immutable (or
> > un
gt;> {@link Node#getClassCssMetaData()}.
>> *
>> * Example:
>> *
>> * private static final List<CssMetaData<? extends Styleable, ?>>
>> STYLEABLES = CssMetaData.combine(
>> * <Parent>.getClassCssMetaData(),
>> * STYLE
On Wed, 29 Nov 2023 19:29:20 GMT, Andy Goryachev wrote:
> I do wonder if `combineStyleables` or `ofStyleables` might be a better name
> than just `combine`? I don't have a strong opinion.
That seems like it would be a misnomer. The proposed method combines the
metadata of styleable properties,
`
now. I think `CssMetaData.combine()` might be clear enough.
-
PR Comment: https://git.openjdk.org/jfx/pull/1296#issuecomment-1832570261
gt;> {@link Node#getClassCssMetaData()}.
>> *
>> * Example:
>> *
>> * private static final List<CssMetaData<? extends Styleable, ?>>
>> STYLEABLES = CssMetaData.combine(
>> * <Parent>.getClassCssMetaData(),
>> * STYLE
On Wed, 29 Nov 2023 15:42:47 GMT, Andy Goryachev wrote:
> > stored in a `CopyOnWriteArrayList`
>
> I am sorry, are you proposing to add two pointers to each Node?
I was responding to Nir, seeing if there is an option to make the entire
management of CSS properties simpler.
> > when Skin chang
On Wed, 29 Nov 2023 16:09:49 GMT, Michael Strauß wrote:
> This introduces yet another moving part, namely that the list of styleable
> properties can change when the control skin is changed.
This functionality is outside of the scope of this PR, completely unrelated.
The part that is being ch
On Wed, 29 Nov 2023 15:57:46 GMT, Andy Goryachev wrote:
> > when Skin changes happen
>
> can you please explain how Skins have managed to enter the discussion?
> Styleable properties are a part of the Control, they have nothing to do with
> Skins.
Skins seem to be everywhere. In particular, s
On Wed, 29 Nov 2023 07:38:45 GMT, John Hendrikx wrote:
> when Skin changes happen
can you please explain how Skins have managed to enter the discussion?
Styleable properties are a part of the Control, they have nothing to do with
Skins.
-
PR Comment: https://git.openjdk.org/jfx/
On Wed, 29 Nov 2023 02:44:31 GMT, Nir Lisker wrote:
> I'm unconvinced that this is the way to solve the issue it describes in its
> doc. The problem is that each control wants to add its own css metadata to
> that of its parent (recursively), and that the list is immutable (or
> unmodifiable).
On Wed, 29 Nov 2023 07:38:45 GMT, John Hendrikx wrote:
> stored in a `CopyOnWriteArrayList`
I am sorry, are you proposing to add two pointers to each Node?
-
PR Comment: https://git.openjdk.org/jfx/pull/1296#issuecomment-1832162481
On Wed, 29 Nov 2023 02:44:31 GMT, Nir Lisker wrote:
> What each class brings with itself is a list of its own metadata properties.
> That's all it should declare. This list should automatically be added to the
> one of its parent, recursively, forming the final list. This part shouldn't
> be d
On Tue, 28 Nov 2023 23:34:30 GMT, Andy Goryachev wrote:
>> Provides a public utility method for use by the skins (core and custom) to
>> simplify initialization of styleable properties.
>>
>>
>> + /**
>> + * Utility method which combines CssMetaData items in one unmodifiable list
>> with the
> Provides a public utility method for use by the skins (core and custom) to
> simplify initialization of styleable properties.
>
>
> + /**
> + * Utility method which combines CssMetaData items in one unmodifiable list
> with the size equal to the number
> + * of items it holds (i.e. with no un
On Tue, 28 Nov 2023 20:34:31 GMT, Andy Goryachev wrote:
>> Provides a public utility method for use by the skins (core and custom) to
>> simplify initialization of styleable properties.
>>
>>
>> + /**
>> + * Utility method which combines CssMetaData items in one unmodifiable list
>> with the
On Tue, 28 Nov 2023 22:41:31 GMT, Kevin Rushforth wrote:
>> modules/javafx.graphics/src/main/java/javafx/css/CssMetaData.java line 31:
>>
>>> 29: import java.util.List;
>>> 30: import javafx.scene.Node;
>>> 31: import com.sun.javafx.UnmodifiableArrayList;
>>
>> KCR: this is a dummy comment. Ign
On Tue, 28 Nov 2023 20:34:31 GMT, Andy Goryachev wrote:
>> Provides a public utility method for use by the skins (core and custom) to
>> simplify initialization of styleable properties.
>>
>>
>> + /**
>> + * Utility method which combines CssMetaData items in one unmodifiable list
>> with the
On Tue, 28 Nov 2023 20:14:13 GMT, Andy Goryachev wrote:
>> Nir was probably referring to the type of the `list` parameter.
>> As for the return value, not only must this be a `List`, it should also be
>> mandated to be `RandomAccess`. Any implementation that doesn't support
>> random access may
On Tue, 28 Nov 2023 18:45:01 GMT, Andy Goryachev wrote:
>> modules/javafx.graphics/src/main/java/javafx/css/CssMetaData.java line 333:
>>
>>> 331: /**
>>> 332: * Utility method which combines {@code CssMetaData} items in one
>>> unmodifiable list with size equal
>>> 333: * to the
On Tue, 28 Nov 2023 22:21:29 GMT, Kevin Rushforth wrote:
>> Andy Goryachev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> javadoc
>
> modules/javafx.graphics/src/main/java/javafx/css/CssMetaData.java line 31:
>
>> 29: import java.util.
On Tue, 28 Nov 2023 15:49:27 GMT, Andy Goryachev wrote:
>> modules/javafx.graphics/src/main/java/javafx/css/CssMetaData.java line 332:
>>
>>> 330:
>>> 331: /**
>>> 332: * Utility method which combines {@code CssMetaData} items in one
>>> unmodifiable list with size equal
>>
>> Did yo
On Tue, 28 Nov 2023 20:34:31 GMT, Andy Goryachev wrote:
>> Provides a public utility method for use by the skins (core and custom) to
>> simplify initialization of styleable properties.
>>
>>
>> + /**
>> + * Utility method which combines CssMetaData items in one unmodifiable list
>> with the
On Tue, 28 Nov 2023 21:53:19 GMT, Andy Goryachev wrote:
> > But I've already explained the rationale: a subclass **cannot** hide its
> > inherited JavaFX properties.
>
> It can, but only from the CSS subsystem. Whether it makes sense or not is a
> different topic, but it can - by not reporting
On Tue, 28 Nov 2023 19:02:27 GMT, Michael Strauß wrote:
> But I've already explained the rationale: a subclass **cannot** hide its
> inherited JavaFX properties.
It can, but only from the CSS subsystem. Whether it makes sense or not is a
different topic, but it can - by not reporting it via g
> Provides a public utility method for use by the skins (core and custom) to
> simplify initialization of styleable properties.
>
>
> + /**
> + * Utility method which combines CssMetaData items in one unmodifiable list
> with the size equal to the number
> + * of items it holds (i.e. with no un
On Tue, 28 Nov 2023 19:12:14 GMT, Nir Lisker wrote:
> which is required for new features.
There will be a CSR filed since it's an addition to a public API. It's a small
change, and usually there are changes to javadoc and method signatures that
make it more time consuming to keep two tickets
On Tue, 28 Nov 2023 19:20:18 GMT, Michael Strauß wrote:
>> It should have been a Set maybe, or indeed a Collection.
>>
>> But it is too late: Node codifies the List return values:
>>
>>
>> public static List>
>> getClassCssMetaData();
>>
>> public List> getCssMetaData();
>
> Nir was probably
On Tue, 28 Nov 2023 19:14:51 GMT, Andy Goryachev wrote:
>> modules/javafx.graphics/src/main/java/javafx/css/CssMetaData.java line 342:
>>
>>> 340: */
>>> 341: public static List> combine(
>>> 342: List> list,
>>
>> Any reason this is specifically a `List`? Can it not be a `Coll
On Tue, 28 Nov 2023 18:58:57 GMT, Nir Lisker wrote:
>> Andy Goryachev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> combine
>
> modules/javafx.graphics/src/main/java/javafx/css/CssMetaData.java line 342:
>
>> 340: */
>> 341:
On Tue, 28 Nov 2023 00:51:36 GMT, Andy Goryachev wrote:
>> Provides a public utility method for use by the skins (core and custom) to
>> simplify initialization of styleable properties.
>>
>>
>> + /**
>> + * Utility method which combines CssMetaData items in one unmodifiable list
>> with the
On Tue, 28 Nov 2023 18:46:54 GMT, Andy Goryachev wrote:
> > 1. I wouldn't frame this as introducing a new requirement, but merely
> > clarifying the specification.
>
> I'd rather leave this as is. If it is a requirement, then a) there should be
> a rationale and b) it has to be tested, at leas
On Tue, 28 Nov 2023 00:51:36 GMT, Andy Goryachev wrote:
>> Provides a public utility method for use by the skins (core and custom) to
>> simplify initialization of styleable properties.
>>
>>
>> + /**
>> + * Utility method which combines CssMetaData items in one unmodifiable list
>> with the
On Tue, 28 Nov 2023 18:07:34 GMT, Michael Strauß wrote:
> 2. Not that I know, but there's nothing to be gained from allowing controls
> to mix up the order of their properties. This is performance-critical code,
> and we might want to benefit from a stable and predictable order at some
> point
On Tue, 28 Nov 2023 00:51:36 GMT, Andy Goryachev wrote:
>> Provides a public utility method for use by the skins (core and custom) to
>> simplify initialization of styleable properties.
>>
>>
>> + /**
>> + * Utility method which combines CssMetaData items in one unmodifiable list
>> with the
On Tue, 28 Nov 2023 18:18:09 GMT, Michael Strauß wrote:
>> Andy Goryachev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> combine
>
> modules/javafx.graphics/src/main/java/javafx/css/CssMetaData.java line 333:
>
>> 331: /**
>> 332:
On Tue, 28 Nov 2023 00:51:36 GMT, Andy Goryachev wrote:
>> Provides a public utility method for use by the skins (core and custom) to
>> simplify initialization of styleable properties.
>>
>>
>> + /**
>> + * Utility method which combines CssMetaData items in one unmodifiable list
>> with the
On Tue, 28 Nov 2023 15:29:06 GMT, Andy Goryachev wrote:
> > 1. A derived class must include the metadata of its base class.
> > 2. The inherited metadata must come before the new metadata in the list.
>
> 1. what if the intent is to disallow certain parent properties from being
> modified by CS
On Tue, 28 Nov 2023 06:37:45 GMT, John Hendrikx wrote:
>> Andy Goryachev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> combine
>
> modules/javafx.graphics/src/main/java/javafx/css/CssMetaData.java line 332:
>
>> 330:
>> 331: /**
On Tue, 28 Nov 2023 07:00:36 GMT, John Hendrikx wrote:
>> Andy Goryachev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> combine
>
> modules/javafx.graphics/src/main/java/javafx/css/CssMetaData.java line 344:
>
>> 342: List> lis
On Tue, 28 Nov 2023 00:55:58 GMT, Michael Strauß wrote:
> 1. A derived class must include the metadata of its base class.
> 2. The inherited metadata must come before the new metadata in the list.
1. what if the intent is to disallow certain parent properties from being
modified by CSS? I know
On Tue, 28 Nov 2023 00:51:36 GMT, Andy Goryachev wrote:
>> Provides a public utility method for use by the skins (core and custom) to
>> simplify initialization of styleable properties.
>>
>>
>> + /**
>> + * Utility method which combines CssMetaData items in one unmodifiable list
>> with the
tadata of its base class.
2. The inherited metadata **must** come before the new metadata in the list.
The specification should also point developers to the new `CssMetaData.combine`
API.
-
PR Comment: https://git.openjdk.org/jfx/pull/1296#issuecomment-1828889945
> Provides a public utility method for use by the skins (core and custom) to
> simplify initialization of styleable properties.
>
>
> + /**
> + * Utility method which combines CssMetaData items in one unmodifiable list
> with the size equal to the number
> + * of items it holds (i.e. with no un
56 matches
Mail list logo
