RFR: 8320796: CssMetaData.combine() [v4]

[Kevin Rushforth]

On Tue, 28 Nov 2023 15:49:27 GMT, Andy Goryachev <angorya at openjdk.org> 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 you mean `immutable`?  As far as I know, the meaning of `unmodifiable` (in the Java world) means that the caller can't modify it, but the  provider still can.  For example:
>> 
>>       List<String> list = new ArrayList<>();
>>       List<String> unmodifiableList = Collections.unmodifiableList(list);
>>       list.add("A");  // both lists changed
>> 
>> Quote from collections docs:
>> 
>>> Note that changes to the backing collection might still be possible, and if they occur, they are visible through the unmodifiable view. Thus, an unmodifiable view collection is not necessarily immutable. However, if the backing collection of an unmodifiable view is effectively immutable, or if the only reference to the backing collection is through an unmodifiable view, the view can be considered effectively immutable.
>
> Since this is not an observable list, the two are semantically equivalent, in my opinion.
> 
> @kevinrushforth should we say "immutable" here?

Yes, I think using the term "immutable" makes sense for the reasons John mentioned.

>> modules/javafx.graphics/src/main/java/javafx/css/CssMetaData.java line 344:
>> 
>>> 342:         List<CssMetaData<? extends Styleable, ?>> list,
>>> 343:         CssMetaData<? extends Styleable, ?>... items)
>>> 344:     {
>> 
>> The method is not actually making use of `CssMetaData`, so a more generic utility method would have worked as well:
>> 
>>          public static <T> List<T> combine(List<T> list, T... items)
>> 
>> It could be placed in some kind of utility class (and in fact, many libs offer such a method already).
>
> Yeah, perhaps even in List?  
> JavaFX does not provide a common utility package, and I am trying to limit the scope by narrowing the accepted data type here.

I think having it in this class is good given the intent. It allows for a more narrowly-focused API.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1296#discussion_r1408490325
PR Review Comment: https://git.openjdk.org/jfx/pull/1296#discussion_r1408491827