RFR: 8320359: ImageView: add styleable fitWidth, fitHeight, preserveRatio, smooth properties [v3]
Andy Goryachev
angorya at openjdk.org
Fri Dec 1 15:35:21 UTC 2023
On Fri, 1 Dec 2023 04:32:16 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:
>> Good point, thanks!
>> This also applies to UnmodifiableArrayList.
>>
>> For completeness sake, I wanted to mention a few issues (not in scope for this PR) that came out of the code review:
>>
>> - could use `UnmodifiableArrayList` but it stores extra int size. perhaps a factory method can be added to it for when `size != elements.length`.
>> - could improve UnmodifiableArrayList with fast(er) `indexOf`, `hashCode`, `equals` per @hjohn 's comment earlier
>> - `Control.getCssMetaData()` can be improved to skip merging of two lists if skinBase is null
>> - the same immutable list should be used in `Control.getCssMetaData()` instead of `ArrayList()`
>
> I wouldn't do any of the above unless there is a very good reason (and I'm not seeing one). Just use standard `List.of` as the last step (or use the `Collections.unmodifableList` wrapper); you'll get the most optimized, automatically maintained, bugfree, immutable `List` implementation Java has to offer. It means another copy will be made; that's fine, this only happens once -- it's not in a hot path.
>
> If you feel like optimizing something, don't bother with `Control.getCssMetaData` either; instead, deduplicate the property lists so there is only one list per unique `Skin` + `Control` combo. That saves a **complete** list per control **instance**.
Good point, though I would still not use List.of() because of the unnecessary copy.
I agree on `Skin` + `Control` copy. Just not sure how... a static hash table perhaps?
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1293#discussion_r1412253329
More information about the openjfx-dev
mailing list