Page MenuHomeWildfire Games

Add CSlider to GUI
ClosedPublic

Authored by vladislavbelov on Apr 13 2017, 3:09 PM.

Details

Reviewers
wraitii
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Owners Package(Owns No Changed Paths)
Commits
rP19479: Add sliders to the GUI. Use them for the sound/music volume controls.
Trac Tickets
#4039
Summary

Original patch was by dpiquet, I rewrote it and added some style/code things.

Use a slider to control the sound volume.

Currently the upper bound of sound gain is 10.0: ​http://trac.wildfiregames.com/wiki/Manual_Settings. But 1.0 is normal sound gain (not increased, not decreased), so upper bound would be good in [1.0, 2.0], because user with normal settings will have a problem to make gain in [0.5, 1.0], also the default gain value is 0.5. So I've set it to 2.0.

Also @echotangoecho has a good point about titling:

While you're at it, IMO, the "Gain" part of the sound settings names should be renamed "volume".

So I suggest to discuss and add it in the next commit, if all would agree.

Also I plan to add tooltip if it'd needed and support discrete values, like pass an array of string/float/int to it.

Test Plan
  1. Run the game
  2. Go to the option menu
  3. Move volume sliders and test that volume is really changing
  4. Save current options
  5. Restart the game and test that all sliders are in right places

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vladislavbelov edited the summary of this revision. (Show Details)Apr 13 2017, 3:16 PM
Vulcan added a subscriber: Vulcan.Apr 13 2017, 3:54 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/755/ for more details.

Works as advertised, code seem OK. See inline comments.

source/gui/CSlider.cpp
28 ↗(On Diff #1228)

Indent seems wonky here but maybe it's phabricator.

64 ↗(On Diff #1228)

It would be nice to add a "step" parameter like the CSS slider, so that you can define the granularity of the slider.

elexis added a subscriber: elexis.Apr 15 2017, 5:12 PM

Just some linting

binaries/data/mods/public/gui/options/options.js
141 ↗(On Diff #1228)

for...in

binaries/data/mods/public/gui/options/options.json
220 ↗(On Diff #1228)

0.0 -> 0, 2.0 -> 2. No type distinction for JS numbers

source/gui/CSlider.cpp
26 ↗(On Diff #1228)

Don't we miss some initialization of members?

28 ↗(On Diff #1228)

The former

111 ↗(On Diff #1228)

should be moved below the early return, could be moved just before its usage

source/gui/CSlider.h
23 ↗(On Diff #1228)

pointless comment, same below (besides UpdateValue maybe)

wraitii requested changes to this revision.Apr 30 2017, 10:11 AM
This revision now requires changes to proceed.Apr 30 2017, 10:11 AM
vladislavbelov edited edge metadata.
vladislavbelov marked 5 inline comments as done.

Style fixes.

source/gui/CSlider.cpp
26 ↗(On Diff #1228)

No, CPos has default c-tor, also we should fill this variable only on mouse down, so until mouse not down, the value may be invalid.

64 ↗(On Diff #1228)

Agree, I'll add it in next patches.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/937/ for more details.

wraitii accepted this revision.Apr 30 2017, 6:21 PM
This revision is now accepted and ready to land.Apr 30 2017, 6:21 PM
wraitii requested changes to this revision.Apr 30 2017, 6:28 PM

rm actually never mind, see inline comments :p

source/gui/CSlider.cpp
30 ↗(On Diff #1552)

Missed that: you have too many tabs here.

This revision now requires changes to proceed.Apr 30 2017, 6:28 PM
wraitii accepted this revision.Apr 30 2017, 6:29 PM

Right actually I'll fix that before committing, forgot you couldn't commit there…

This revision is now accepted and ready to land.Apr 30 2017, 6:29 PM
This revision was automatically updated to reflect the committed changes.