On Tue, 28 Nov 2023 15:49:27 GMT, Andy Goryachev <ango...@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
