On Fri, 2 May 2025 23:00:32 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
Changes requested by aivanov (Reviewer).
src/java.desktop/share/classes/com/sun/media/sound/DataPusher.java line 97:
> 95: public boolean isPlaying() {
> 96: return threadState == STATE_PLAYING;
> 97: }
I think this method should be `synchronized`.
(The `threadState` field is modified in `run()` without synchronization, which
isn't guaranteed to be visible by other threads. But it's a different issue.)
src/java.desktop/share/classes/com/sun/media/sound/JavaSoundAudioClip.java line
104:
> 102: throw e;
> 103: }
> 104: }
Can any other (checked) exception be thrown?
I can't see that it's possible, then catching it isn't necessary.
src/java.desktop/share/classes/com/sun/media/sound/JavaSoundAudioClip.java line
113:
> 111: clip.init(uc.getInputStream());
> 112: } catch (final Exception ignored) {
> 113: // Playing the clip will be a no-op if an exception occured
> in inititialization.
Suggestion:
// Playing the clip will be a no-op if an exception occurred in
inititialization.
src/java.desktop/share/classes/javax/sound/SoundClip.java line 40:
> 38: * Typically this means running the application in a desktop environment.
> 39: *
> 40: * Multiple {@code SoundClip} items can be playing at the same time, and
Suggestion:
* Typically this means running the application in a desktop environment.
*
* <p>
* Multiple {@code SoundClip} items can be playing at the same time, and
I assume a new paragraph starts with _“Multiple…”_.
src/java.desktop/share/classes/javax/sound/SoundClip.java line 47:
> 45: public final class SoundClip {
> 46:
> 47: private JavaSoundAudioClip clip;
Suggestion:
private final JavaSoundAudioClip clip;
The value doesn't change after an instance is created, so it can and should be
final.
src/java.desktop/share/classes/javax/sound/SoundClip.java line 50:
> 48:
> 49: /**
> 50: * Create a {@code SoundClip} instance which will play a clip from
> the supplied file.
Suggestion:
* Creates a {@code SoundClip} instance which will play a clip from the
supplied file.
src/java.desktop/share/classes/javax/sound/SoundClip.java line 52:
> 50: * Create 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
Suggestion:
* <p>
* If the file does not contain recognizable and supported sound data, or
Without `<p>`, this block will be rendered right after the first sentence above
without a paragraph break. If it's intended, I propose removing a seemingly
empty line.
src/java.desktop/share/classes/javax/sound/SoundClip.java line 56:
> 54: * playing the clip will be a no-op.
> 55: *
> 56: * @param file from which to obtain the sound data
Suggestion:
* @param file the file from which to obtain the sound data
src/java.desktop/share/classes/javax/sound/SoundClip.java line 69:
> 67:
> 68: private SoundClip() {
> 69: }
If the no-arg constructor is never used, then it's not needed and it should be
removed.
src/java.desktop/share/classes/javax/sound/SoundClip.java line 78:
> 76: * Returns whether this is a playable sound clip.
> 77: * A value of {@code false} means that calling any of the other
> methods
> 78: * of this class is a no-op
Suggestion:
* of this class is a no-op.
src/java.desktop/share/classes/javax/sound/SoundClip.java line 90:
> 88: * @return whether sound is currently playing
> 89: */
> 90: public boolean isPlaying() {
Suggestion:
/**
* {@return whether sound is currently playing}
*/
public boolean isPlaying() {
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}
Suggestion:
* Capture, processing and playback of sampled audio data is under {@link
javax.sound.sampled}.
* <p>
* Sequencing, and synthesis of MIDI (Musical Instrument Digital Interface)
data is under {@link javax.sound.midi}.
test/jdk/javax/sound/SoundClip/SoundClipTest.java line 43:
> 41: if (!isSoundcardInstalled()) {
> 42: return;
> 43: }
It should throw `jtreg.SkippedException` instead of pretending the test passes.
test/jdk/javax/sound/SoundClip/SoundClipTest.java line 60:
> 58: if (clip.isPlaying()) {
> 59: playing = true;
> 60: break;
`break` is somewhat redundant since the loop has `!playing` as the condition
for executing the body.
test/jdk/javax/sound/SoundClip/SoundClipTest.java line 101:
> 99: } catch (Exception e) {
> 100: System.err.println("Exception occurred: "+e);
> 101: }
The stack trace of the exception could be crucial to understanding what went
wrong, so printing the stack trace could be a better approach than just the
error message.
I guess you want to pass the test (or as I suggest throw
`jtreg.SkippedException`) if anything goes wrong.
`AudioSystem.getSourceDataLine` throws `LineUnavailableException` which is
checked exception, and if the exception
-------------
PR Review: https://git.openjdk.org/jdk/pull/24991#pullrequestreview-2822446607
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078030777
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078040610
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078061539
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078065416
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078047776
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078048603
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078050809
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078051444
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078053524
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078054567
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078057503
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078067119
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078070770
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078072903
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078084485
