On Thu, 20 Feb 2025 18:20:52 GMT, Harshitha Onkar <hon...@openjdk.org> wrote:
>> src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 1154:
>>
>>> 1152: * This method is useful for advanced applications which need to
>>> access
>>> 1153: * profile data directly. Only non-built-in, application provided
>>> profiles
>>> 1154: * should be updated using this method.
>>
>> What you mean is that only non-built-in profiles *can* be updated. The fix
>> makes it impossible to update built-in profiles.
>
> I'll be inverting this statement to state what is NOT allowed rather than
> what is allowed by .setData() as below. Does the following sound better?
>
>
> * Note: JDK built-in ICC Profiles cannot be updated using this method
> * as it will result in IAE. JDK built-in profiles are those obtained by
> * {@code ICC_Profile.getInstance(int colorSpaceID)} where colorSpaceID
> * is one of the following:
> * {@link ColorSpace#CS_sRGB}, {@link ColorSpace#CS_LINEAR_RGB},
> * {@link ColorSpace#CS_PYCC}, {@link ColorSpace#CS_GRAY} or
> * {@link ColorSpace#CS_CIEXYZ}
This is clearer.
>> src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 1164:
>>
>>> 1162: * array can not be interpreted as valid tag data,
>>> corresponding to
>>> 1163: * the {@code tagSignature}
>>> 1164: * @throws IllegalArgumentException if this is a profile for one
>>> of the
>>
>> `IllegalStateException` better describes the reason: the argument to the
>> method can be perfectly valid, but the internal state of the object doesn't
>> allow modifications.
>
> @aivanov-jdk
>
> _IllegalStateException - Signals that a method has been invoked at an
> **illegal or inappropriate time.**_
> Since IllegalStateException is thrown to indicate more of an unstable state
> of object, it may not be what we want here. The exception is to be thrown
> when the ICC_Profile object invoking .setData() is JDK Built-in profile and
> IIlegalArgumentException more closely match this case.
The javadoc for `IllegalArgumentException` says, <q
cite="https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/IllegalArgumentException.html";>Thrown
to indicate that a method has been passed <em>an illegal or inappropriate
argument</em>.</q> (Emphasis mine.)
Getting an `IllegalArgumentException` for a valid argument would be confusing.
`IllegalStateException`, as you said, “signals that a method has been invoked
at an <em>illegal or inappropriate</em> time”. It's exactly the state of the
object: a *built-in* color profile cannot be modified.
Although, there's no time where such a modification will be allowed, I still
think `IllegalStateException` better conveys the meaning: changes to built-in
profiles are *never* allowed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1964136111
PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1964135080
