On Fri, 9 May 2025 15:30:12 GMT, Alexey Ivanov <aiva...@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
