On Wed, 7 May 2025 20:47:16 GMT, Phil Race <p...@openjdk.org> wrote:
>> A simple API to replace java.applet.AudioClip
>>
>> CSR is now created : https://bugs.openjdk.org/browse/JDK-8356200
>
> Phil Race has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8356049
Looks good to me except for a few nits.
I guess you should also update the copyright years in modified files.
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.
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.
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.
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`.
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.
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.
-------------
Changes requested by aivanov (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/24991#pullrequestreview-2828804099
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2081937814
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2081976115
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2081955900
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2081966734
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2081992607
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2081993946
