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

[Phil Race]

On Fri, 2 May 2025 17:15:29 GMT, Hendrik Schick <duke 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/JavaSoundAudioClip.java line 101:
> 
>> 99:             clip.init(stream);
>> 100:         } catch (final Exception e) {
>> 101:             // AudioClip will be no-op if some exception will occurred
> 
> Suggestion:
> 
>             // AudioClip will be no-op if some exception will occur
> 
> or 'has occurred' (?)

I'm just going to delete the comment here as it is a bit out of place.
The more interesting question is whether IOException should be thrown or swallowed like all other exceptions.
The current code throws it (clearly) and so the app has to handle it.
But if we swallow other exceptions ... ?

> src/java.desktop/share/classes/com/sun/media/sound/JavaSoundAudioClip.java line 114:
> 
>> 112:             clip.init(uc.getInputStream());
>> 113:         } catch (final Exception ignored) {
>> 114:             // 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 initialization.

No plans to fix typos comments for code I'm not touching and expect to delete in the near future

> src/java.desktop/share/classes/com/sun/media/sound/JavaSoundAudioClip.java line 124:
> 
>> 122:             clip.init(url.openStream());
>> 123:         } catch (final Exception ignored) {
>> 124:             // 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.

No plans to fix typos comments for code I'm not touching and expect to delete in the near future

> src/java.desktop/share/classes/com/sun/media/sound/JavaSoundAudioClip.java line 294:
> 
>> 292:     public synchronized void update(LineEvent event) {
>> 293:        if (clip != null) {
>> 294:            if (clip == event.getSource()) {
> 
> merge the two ifs to one if?

I could .. but this way it is clearer that clip may actually be null ..

> test/jdk/javax/sound/SoundClip/SoundClipTest.java line 40:
> 
>> 38: 
>> 39:     public static void main(String[] args) throws Exception {
>> 40: 
> 
> Suggestion:

I like blank lines there

> test/jdk/javax/sound/SoundClip/SoundClipTest.java line 100:
> 
>> 98:             }
>> 99:         } catch (Exception e) {
>> 100:             System.err.println("Exception occured: "+e);
> 
> Suggestion:
> 
>             System.err.println("Exception occurred: " + e);

I copied that from another test.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2072221321
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2072221421
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2072221490
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2072222130
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2072222289
PR Review Comment: https://git.openjdk.org/jdk/pull/24991#discussion_r2072222456