On Tue, 13 May 2025 19:32:00 GMT, Sergey Bylokhov <s...@openjdk.org> wrote:
>> Phil Race has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8356049
>
> src/java.desktop/share/classes/javax/sound/SoundClip.java line 35:
>
>> 33: /**
>> 34: * The {@code SoundClip} class is a simple abstraction for playing a
>> sound clip.
>> 35: * It will play any format that is recognized by the {@code javax.sound}
>> API,
>
> I think we should mention in the doc that this is applicable for small audio
> files since we will load all data into the memory.
Example of memory requirement:
https://github.com/openjdk/jdk/blob/2c4e8d211a030c85488e656a9a851d10dd0f9c11/src/java.desktop/share/classes/javax/sound/sampled/Clip.java#L34
> src/java.desktop/share/classes/javax/sound/SoundClip.java line 45:
>
>> 43: * @since 25
>> 44: */
>> 45: public final class SoundClip {
>
> What about considering a different name, such as AudioClip with a static
> method like AudioClip.create(...)? or maybe Audio....Player?
Could having both javax.sound.SoundClip and javax.sound.sampled.Clip cause
confusion?
> src/java.desktop/share/classes/javax/sound/SoundClip.java line 63:
>
>> 61: public static SoundClip createSoundClip(File file) throws
>> IOException {
>> 62: if (file == null) {
>> 63: throw new IllegalArgumentException("file must not be null");
>
> Most of the APIs in javax.sound.* throw NullPointerException for null
> arguments and IllegalArgumentException for other invalid parameters.
it is even noticed for both subpackages, it is better to use NPE here as well:
* Please note: In the {@code javax.sound.sampled.spi} APIs, a {@code null}
* reference parameter to methods is incorrect unless explicitly documented on
* the method as having a meaningful interpretation. Usage to the contrary is
* incorrect coding and may result in a run time exception either immediately or
* at some later time. {@code NullPointerException} is an example of typical and
* acceptable run time exception for such cases.
> test/jdk/javax/sound/SoundClip/SoundClipTest.java line 41:
>
>> 39: public static void main(String[] args) throws Exception {
>> 40:
>> 41: if (!isSoundcardInstalled()) {
>
> Why we cannot check that the playing sound is a no-op in this case, as
> specified?
Why do not check this case as well?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2087976970
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2087986810
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2087974279
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2087985318
