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

Improve discoverability of dummy sound device #17470

Merged
merged 2 commits into from Dec 29, 2019

Conversation

@pchote
Copy link
Member

pchote commented Dec 15, 2019

This PR expands on #17245 to make it more obvious to players why their sound isn't working when the dummy sound device is active.

The first commit renames the sound device exposed in the settings.
The second commit disables the options when the sound is disabled or muted. It also fixes a silly bug where the sliders won't work at all if the sound is muted when the settings menu is first opened.

If the second commit is considered weird, I could instead display a label warning that the sound is disabled so changing them won't do anything, and find a different fix to whatever the mute code was trying to do.

@pchote pchote added this to the Next Release milestone Dec 15, 2019
@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Dec 23, 2019

Probably out of scope for this PR, but: The 'disabled' feedback of the volume sliders sucks. Rather, other than being immovable, there's no visible indicator that they're disabled at all, not even greyed out text.

Otherwise looks good, @pchote if you'd prefer to see this merged as-is for now, I'd be ok with that.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Dec 23, 2019

Indeed - this is what I was referring to in the final paragraph. Would you prefer to keep them active with a warning label somewhere (less obvious), or to hide the controls and show the warning instead (possibly less flexible)?

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Dec 23, 2019

If it doesn't require too many changes, personally I'd prefer the text and slider box to grey out (in addition to becoming immovable), like what we do with the cash ticks box.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Dec 23, 2019

That would work too, but i'm not touching chrome.yaml again until #17489 has been merged.

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Dec 23, 2019

That would work too, but i'm not touching chrome.yaml again until #17489 has been merged.

I guess that disqualifies the approach for prep, then...
Could we compromise on just greying out the slider text in this PR and then a follow-up after #17489 greying out the slider boxes as well (bleed only)?

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Dec 23, 2019

The main reason is that I have a deep tree with multiple branches all based on #17489, and I really don't want to deal with resolving the rebase conflict this would cause several times over.

If we wait for #17489 I can make the change in the new syntax for bleed, then file a separate PR with the legacy format for prep.

@pchote pchote dismissed stale reviews from abmyii and Mailaender via 12fb91e Dec 28, 2019
@pchote pchote force-pushed the pchote:dummy-sound-polish branch from d21352d to 12fb91e Dec 28, 2019
@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Dec 28, 2019

I'm now hiding the controls instead:

Screenshot 2019-12-28 at 10 37 36

and keep the slider bindings regardless of the mute setting. This avoids the conflict with the chrome rework so we can get this merged for prep.

@reaperrr reaperrr merged commit a7ae939 into OpenRA:bleed Dec 29, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pchote pchote deleted the pchote:dummy-sound-polish branch Jan 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.