Page MenuHomeWildfire Games

Add CSlider to GUI
ClosedPublic

Authored by vladislavbelov on Apr 13 2017, 3:09 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Sep 13, 3:32 PM
Unknown Object (File)
Thu, Sep 12, 1:44 PM
Unknown Object (File)
Mon, Sep 9, 2:32 AM
Unknown Object (File)
Wed, Sep 4, 10:36 PM
Unknown Object (File)
May 4 2017, 6:48 AM
Unknown Object (File)
Apr 30 2017, 6:40 PM
Unknown Object (File)
Apr 30 2017, 6:30 PM
Unknown Object (File)
Apr 30 2017, 1:26 PM
Subscribers
Restricted Owners Package
Restricted Owners Package
Tokens
"Like" token, awarded by elexis.

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.
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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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.

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)

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.

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

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.