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

Fix Sound Effects on Linux #238

Closed

Conversation

chanceVermilion
Copy link

First off I want to tell you that I just found this project and I'm impressed. I haven't played Osu! in years and in the mean time I've made Ubuntu my OS of choice so I was happy to find a native way to play the game again.

This fixes (on my Ubuntu 16.04 machine) #89 . Of course that fix introduced another issue on the Download Menu which I fixed by removing what looks like a redundant call to stop the track. I also did a directory restructure to make the project more maven-y, added a test, and changed the exec plugin to run during the integration-test phase. I've played around with this on one machine and now the sound effects and song download previews work.

itdelatrisu added a commit that referenced this pull request Jan 22, 2017
Thanks to @chanceVermilion for the fix.

Signed-off-by: Jeffrey Han <itdelatrisu@gmail.com>
@itdelatrisu
Copy link
Owner

Thanks a lot for figuring this out! (Especially since I never managed to reproduce any of the audio issues on Linux...)

I committed just the audio fix in 039dd66 (didn't want to bother you by asking to not move around imports, etc.).

I'm slightly against the other changes for these reasons:

  • Not really a fan of Maven's project structure with all the useless (for this project, at least) nested directories. I think it's plenty clear enough as is, too.
  • At this point it seems kind of hopeless/not worth the effort to add unit tests, and I don't think it makes sense to have a testing framework when there will inevitably be almost nothing added to it. (Your test also wouldn't have failed on my (Windows) machine anyway.)
  • I realize this is a pretty sketchy use of mvn compile, but exec isn't really an "integration test" either. Not sure if there's a better place to put it. :/

Closing this PR since the main issue was addressed -- feel free to keep commenting if you disagree with anything I said.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants