On Wed, 19 Apr 2023 17:15:02 GMT, Phil Race <[email protected]> wrote:
>> Martin Desruisseaux has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Update documentation for adressing comment on pull requests, with two
>> changes to be discussed:
>>
>> - The "The default implementation" sentence has not yet been removed, for
>> reason discussed on the pull request.
>> - The discussion about (0,0) tile indices mentions the relationship with
>> `getTileMinX()` and `getTileMinY()`.
>
> src/java.desktop/share/classes/java/awt/image/BufferedImage.java line 1596:
>
>> 1594: * point with (0,0) coordinates since {@code BufferedImage}
>> 1595: * {@linkplain #getRaster() single tile} is always checked out
>> 1596: * for writing.
>
> I think there's some missing punctuation there.
>
> I'd like to slip in some additional explanation that (0,0) is always going
> to be the offset because what else can it be if you only have one tile ?
> The text about "default implementation" in these updated docs might be
> misinterpreted as there's an alternative implementation .. I don't think
> there can be .. not in any compatible way.
> All these methods should have been made final. Sadly its too late for that
> but we need to be clear that this is what you can rely on for a BufferedImage
> and any subclass that changes it is breaking the contract.
>
> How about -
> Since a {@code BufferedImage} consists of a single tile, and that tile is
> always checked out for writing,
> this method, will always return an array of one point.
> Further, in any case of an image with a single tile, the offset will always
> be (0,0), that will always be the coordinates of the single returned {@code
> Point}.
I pushed a new commit which tries to address those comments. The _"default
implementation"_ texts have not yet been removed, pending discussion about
whether the following scenario is acceptable: A library creates a private
`BufferedImage` subclass with `TileObserver` support added. That library
returns instances of that subclass only through public methods having
`WritableRenderedImage` return type, never exposing the `BufferedImage` type
directly. It seems to me that no contract would be broken as long as the user
does not cast to `BufferedImage`.
Of course we can not prevent the user to cast to `BufferedImage`, but this
issue already exists elsewhere in the library. For example all
`Raster.createRaster(…)` static methods may return an instance of
`WritableRaster`, when they decided to return a subclass such as
`sun.awt.image.ByteInterleavedRaster` optimized for a specific sample model. So
we can not have a truly read-only `Raster` through the standard Java API. We
already have to tell users _"Don't try to get write access by casting"_.
Regarding tile indices, in the general `RenderedImage` case they do not
necessarily start at (0,0). They start at the values given by `getMinTileX()`
and `getMinTileY()`. In the particular case of `BufferedImage`, the Javadoc of
those two methods said _"This is always zero."_. So the text saying that the
offset will always be (0,0) is an indirect constraint. I tried to explain that
in the text.
I will create a third commit if the preference is still to remove the _"default
implementation"_ text or for other changes.
Regarding the CRS, I do not have the power to create one on
https://bugs.openjdk.org/.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13506#discussion_r1172303229
