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: Disable audio if there is no audio device #7602

Merged
merged 1 commit into from
Sep 11, 2020
Merged

Conversation

DanVanAtta
Copy link
Member

Testing

Screens Shots

Additional Notes to Reviewer

Addresses: #7546

Release Note

FIX|Disable sound options instead of showing an error message if there is no sound on installed system.

@codecov
Copy link

codecov bot commented Sep 10, 2020

Codecov Report

Merging #7602 into master will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #7602      +/-   ##
============================================
- Coverage     24.42%   24.41%   -0.01%     
+ Complexity     7115     7114       -1     
============================================
  Files          1213     1213              
  Lines         79131    79143      +12     
  Branches      11337    11339       +2     
============================================
- Hits          19325    19324       -1     
- Misses        57625    57637      +12     
- Partials       2181     2182       +1     
Impacted Files Coverage Δ Complexity Δ
...re/src/main/java/org/triplea/sound/ClipPlayer.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
.../src/main/java/org/triplea/sound/SoundOptions.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
.../java/games/strategy/triplea/delegate/Matches.java 42.91% <0.00%> (-0.11%) 357.00% <0.00%> (-1.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d9b90f...fced270. Read the comment docs.

@DanVanAtta DanVanAtta merged commit d4ae7c6 into master Sep 11, 2020
@DanVanAtta DanVanAtta deleted the disable-audo branch September 11, 2020 03:21
Level.INFO,
"Unable to create audio device, is there audio on the system? " + e.getMessage(),
e);
return null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I consider this a satisfying solution but better than a weird exception.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not all systems are going to have audio at all, if that is the case disabling audio features and adding tooltips stating "you don't have audio, so we disabled audio" I think is about the best we can do. The 'info' level will be background and users will typically never see it, the tooltip being displayed thus is quite important.

Can you think of any thing more comprehensive that we could do, or do you have other thoughts on how we could or shoudl handle this? @RoiEXLab

We had a series of such error reports in a row, so it seems like the user was getting spammed with error messages every time the game tried to play a sound.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, I was mostly thinking about the API. I just think it's weird that an exception is thrown to control the flow

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little weird to see an exception handled and 'null' returned rather than 'optional' returned too , speaking of API 😁

I think I'm starting to come to a pattern where an optional return value means we have either an exception that is handled (no logging needed, it is all already done) and/or a nulllable return value; in those cases getting an optional back has seemed quite clean (so no logging when you get an empty optional back, just assume that it's all been handled already).

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.

2 participants