On Wed, 7 May 2025 16:26:33 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 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.)
ok
> 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.
The implementations that accept a URLConnection, a URL and an InputStream all
do the same.
It is probable that was just for IOExceptions but I'm not 100% sure since
init() swallows certain pother exception types itself and then just propagates
IOException to these and definitely those implementations swallow the
propagated IOException.
If we are sufficiently sure that it is OK I could do that (ie assume
IOException is the only possible exception here),
but the first question to answer is whether this new API should also swallow
the IOException like the existing ones.
The one reason for doing that might be if you wanted to be lazy about when you
actually read .. but I doubt we do want to do that. This API isn't for
"streaming" so it should consume the data immediately.
So I'll remove it as you suggest and leave everything else unchanged and we'll
see if anyone has a different opinion.
> 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.
As I wrote previously I am not going fixing typos in methods I didn't touch and
actually expect to remove when the Applet API is removed.
> 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…”_.
yes
> 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.
ok.
> 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.
ok
> 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.
it is intended to be one sentence so is fine as it is.
> 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
ok
> 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.
ok
> 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.
ok
> 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() {
I didn't know you could do that.
> 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}.
ok
> 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.
Well perhaps, but this is what every other similar jtreg sound test does.
> 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.
I suppose ..
> 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. Otherwise, I'd let the
> exception propagate to fail the test, which could be the right thing to do
> since the test include the _sound_ key.
I think I'd like to get to a place where if you pass @sound these tests should
skip this testing and actually FAIL if they can't find sound. Only allowing the
skipping if you didn't explicitly pass @sound.
I haven't found a way to know if that's possible. I'm going to ask ..
This method is copied straight from another test. I think lots of tests use it.
I guess since it already prints the exception adding a trace isn't a problem.
But it isn't completely reliable. Some systems (cloud ones with "emulated"
sound) pass but then the test hangs. I'm probably going to have to do some test
exclusion if I can't figure that out but I don't expect it to be in this PR
because it'll be something internal to our CI systems.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078341427
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078344831
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078347050
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078363357
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078370154
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078370889
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078372396
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078410687
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078373008
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078374629
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078398834
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078382942
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078447291
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078446904
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2078444767
