Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent play! from throwing on systems with no audio output #523

Merged

Conversation

RadicalZephyr
Copy link
Contributor

@RadicalZephyr RadicalZephyr commented Nov 12, 2016

I chose to catch JavaLayerException because that constrains the types of
errors that will be caught to those originating inside of Java Layer.

It's possible that we could detect the presence/absence of an audio
device using the Java Sound API AudioSystem, but it's not clear to me
that would be more robust. This will probably also catch problems like
trying to play a format that is not supported by Java Layer.

Closes #520.

@micha
Copy link
Contributor

micha commented Nov 12, 2016

@RadicalZephyr Awesome, thanks for the patch!

One question, perhaps it would be better to just catch any exception there, rather than only the specific javazoom one? Seems like there really isn't any exception there that we would want to propagate up the stack any further.

@RadicalZephyr
Copy link
Contributor Author

Yeah, that's a good question. One reason I caught the JavaZoom one specifically was that there's an explicit throw if the file isn't found. But on thinking about it today, that's probably only worthy of a warning message as well.

I'll change it to catch Exception

It's possible that we could detect the presence/absence of an audio
device using the Java Sound API AudioSystem, but it's not clear to me
that would be more robust.  This will also change problems like trying to
play an supported format, or passing a non-existent file name into a
printed warning instead of a hard-failure.
@RadicalZephyr
Copy link
Contributor Author

@micha FYI, I updated this to catch Exception now.

@alandipert alandipert merged commit 5884886 into boot-clj:master Dec 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants