On Tue, 22 Oct 2024 05:51:04 GMT, John Hendrikx <[email protected]> wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> scanline stride always measured in bytes
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/iio/javax/XImageLoader.java
> line 209:
>
>> 207: : ImageStorage.ImageType.PALETTE;
>> 208:
>> 209: var scanlineStride = switch(image.getSampleModel()) {
>
> Just a general comment here, on the use of `var`; in my opinion, `var` helps
> the writer of the code, but almost never helps the reader of the code who now
> must read the RHS and manually determine what the type is. As we should be
> aiming for maintainable code and code that's as easy to read as possible
> without having to guess (there's usually enough guessing involved already), I
> always find it hard how the use of `var` can be justified anywhere.
>
> Also in my experience, `var` complicates refactors as it will morph with the
> target type, making all refactor errors occur at the use location instead of
> the declaration site when `var` isn't used (ie. a refactor could indicate
> dozens of errors, while fixing 2 declarations could solve all of them).
>
> So why make us guess that `scanlineStride` is an `int` here? And why force
> us to read the right hand sides of `colorModel`, `palette` and `imageType`...
`scanlineStride` is useful for all the `ImageFrame` constructors, or do you
think recomputing it (with the risk of it being different) is still the best
course of action given that AWT supplies it and might be using some other
alignment/padding?
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1809992939
