RFR: 8356049: Need a simple way to play back a sound clip [v3]

[Phil Race]

On Fri, 9 May 2025 15:30:12 GMT, Alexey Ivanov <aivanov at 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/com/sun/media/sound/DataPusher.java line 97:
> 
>> 95:     public boolean isPlaying() {
>> 96:         return threadState == STATE_PLAYING;
>> 97:     }
> 
> I still think the `isPlaying` should be declared `synchronized`.
> 
> You resolved my previous comment but nothing's changed in the code.

I guess I can but I made javax.SoundClip.isPlaying() synchronized and other methods there are also synchronized so ...

> src/java.desktop/share/classes/com/sun/media/sound/JavaSoundAudioClip.java line 119:
> 
>> 117:             clip.init(url.openStream());
>> 118:         } catch (final Exception ignored) {
>> 119:             // Playing the clip will be a no-op if an exception occurred in inititialization.
> 
> Suggestion:
> 
>             // Playing the clip will be a no-op if an exception occurred in initialization.

I really have no intention of updating comments I will delete soon. Its just too odd.

> src/java.desktop/share/classes/javax/sound/SoundClip.java line 52:
> 
>> 50:      * Creates a {@code SoundClip} instance which will play a clip from the supplied file.
>> 51:      *
>> 52:      * If the file does not contain recognizable and supported sound data, or
> 
> If the two sentences are intended to be in one paragraph, I strongly suggest removing the blank line between them. Otherwise, the blank line in the javadoc source hints that the second sentence should be in its own paragraph.

I misunderstood which line you meant.

> src/java.desktop/share/classes/javax/sound/package-info.java line 32:
> 
>> 30:  * Capture, processing and playback of sampled audio data is under {@link javax.sound.sampled}.
>> 31:  * <p>
>> 32:  * Sequencing, and synthesis of MIDI (Musical Instrument Digital Interface) data is under {@link javax.sound.midi}.
> 
> The description fits into an unordered list.
> 
> Suggestion:
> 
>  * The API is divided into sub-packages:
>  * <ul>
>  *   <li>Capture, processing and playback of sampled audio data is under {@link javax.sound.sampled}.
>  *   <li>Sequencing, and synthesis of MIDI (Musical Instrument Digital Interface) data is under {@link javax.sound.midi}.
>  * </ul>
> 
> 
> The resulting text could be clearer and easier to skim quickly:
>> The API is divided into sub-packages:
>> * Capture, processing and playback of sampled audio data is under `javax. sound. sampled`.
>> * Sequencing, and synthesis of MIDI (Musical Instrument Digital Interface) data is under `javax. sound. midi`.

ok

> test/jdk/javax/sound/SoundClip/SoundClipTest.java line 74:
> 
>> 72:                 playing = false;
>> 73:                 break;
>> 74:             }
> 
> Suggestion:
> 
>             if (clip.isPlaying()) {
>                 playing = false;
>             }
> 
> Since you removed the `break` statement from the loop above, this one needs to go too for consistency.

ok

> test/jdk/javax/sound/SoundClip/SoundClipTest.java line 91:
> 
>> 89:      * on the system.
>> 90:      */
>> 91:     public static boolean isSoundcardInstalled() {
> 
> Suggestion:
> 
>     /**
>      * Returns true if at least one soundcard is correctly installed
>      * on the system.
>      */
>     public static boolean isSoundcardInstalled() {
> 
> The start of the comment is misaligned.

ok

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2082432876
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2082434062
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2082438861
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2082440731
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2082441853
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2082442824