Page MenuHomeWildfire Games

Gamesetup support for sliders, use it for Ceasefire, RelicCount, RelicDuration, WonderDuration
ClosedPublic

Authored by elexis on Jan 15 2020, 2:11 PM.

Details

Summary

This patch adds support for Sliders, so that not only Checkboxes and Dropdowns can be used, and uses that Slider support for RelicDuration, WonderDuration and Ceasefire.
The benefit for the users is that they can select not only the predetermined values.
This may also be considered a disadvantage, since there are use cases to distinguish 1min, 2min, 3min, 4min of ceasefire, but not 30min, 31min, 32min, 33min.
An advantage from the code POV is that gui/common/ becomes cleaned (this could also be achieved by moving these JSONs to the gamesetup/),
and that code is removed by using the slider.

Another problem with the slider is the rounding, which is only supported using workarounds until D406 is committed.

Yet another problem with the slider is that there is no place where the value is shown, except for the gamedescriptions tab on the right hand side.
This problem was solved for the options page by showing the tooltip. But here we can't use that, because the tooltip is on the bottomleft corner - even further away than the gamedescription.
And not using the external tooltip means the fields are become noticably inconsistent (slider tooltip shows below the mouse, dropdown on the bottom left, irritating).
Perhaps this can be solved by showing tooltips always below the mouse.
nani proposed to add an input field right beside the slider, but then why do we need the slider (it would be less code and less UI if it only was an input field).
Another idea is to keep using the dropdown values [0, 1, 3, 5, 10, 15, 20, 25, 30, 35, 40, 45, 50, 55, 60, 75, 90, 105, 120], but map them onto a slider, though that would also sound a lot like wasted code complexity.

Edit: Updated version with label, and 36px setting height instead of 32px:

Test Plan

Check the feature concept and questions / concerns about that detailed in the summary. For the implementation, "make sure that it works".

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

elexis created this revision.Jan 15 2020, 2:11 PM
elexis planned changes to this revision.Jan 15 2020, 2:12 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/1047/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/143/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/GameSettings/Single/Dropdowns/Ceasefire.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/GameSettings/Single/Dropdowns/Ceasefire.js
|  59|  59| 	translateWithContext("ceasefire", "No ceasefire");
|  60|  60| 
|  61|  61| GameSettingControls.Ceasefire.prototype.CeasefireCaption =
|  62|    |-	minutes => translatePluralWithContext("ceasefire", "%(minutes)s minute", "%(minutes)s minutes", minutes)
|    |  62|+	minutes => translatePluralWithContext("ceasefire", "%(minutes)s minute", "%(minutes)s minutes", minutes);
|  63|  63| 
|  64|  64| GameSettingControls.Ceasefire.prototype.DefaultValue = 0;
|  65|  65| 

binaries/data/mods/public/gui/gamesetup/GameSettings/Single/Dropdowns/Ceasefire.js
|  62| »   minutes·=>·translatePluralWithContext("ceasefire",·"%(minutes)s·minute",·"%(minutes)s·minutes",·minutes)
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1565/display/redirect

(Note to self: forgot those)

GameSettings/GameSettingControlSlider.xml

<?xml version="1.0" encoding="utf-8"?>
<object name="sliderSettingFrame[n]" size="0 2 100% 32" hidden="true">

	<include file="gui/gamesetup/Pages/GameSetupPage/GameSettings/GameSettingControlLabels.xml"/>
	<object
		name="sliderSettingControl[n]"
		type="slider"
		size="175 0 100% 30"
		style="ModernSlider"
		tooltip_style="onscreenToolTip"
		hidden="true"
		z="1"
	/>
</object>

GameSettings/GameSettingControlSlider.js

/**
 * This class is implemented by gamesettings that are controlled by a slider.
 */
class GameSettingControlSlider extends GameSettingControl
{
	constructor(...args)
	{
		super(...args);

		this.isInGuiUpdate = false;

		this.slider.onValueChange = this.onValueChangeSuper.bind(this);

		if (this.MinValue !== undefined)
			this.slider.min_value = this.MinValue;

		if (this.MaxValue !== undefined)
			this.slider.max_value = this.MaxValue;
	}

	setControl(gameSettingControlManager)
	{
		let row = gameSettingControlManager.getNextRow("sliderSettingFrame");
		this.frame = Engine.GetGUIObjectByName("sliderSettingFrame[" + row + "]");
		this.slider = Engine.GetGUIObjectByName("sliderSettingControl[" + row + "]");

		let labels = this.frame.children[0].children;
		this.title = labels[0];
		this.label = labels[1];
	}

	setControlTooltip(tooltip)
	{
		this.slider.tooltip = tooltip;
	}

	setControlHidden(hidden)
	{
		this.slider.hidden = hidden;
	}

	setSelectedValue(value, caption)
	{
		this.isInGuiUpdate = true;
		this.slider.value = value;
		this.isInGuiUpdate = false;

		if (this.label)
			this.label.caption = caption;
	}

	onValueChangeSuper()
	{
		if (!this.isInGuiUpdate)
			this.onValueChange(this.slider.value);
	}
}

GameSettingControlSlider.prototype.UnknownValue =
	translateWithContext("settings value", "Unknown");
elexis updated this revision to Diff 11147.Jan 22 2020, 8:03 PM

Rebase, add value label, use it for RelicCount too

elexis edited the summary of this revision. (Show Details)Jan 22 2020, 8:05 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/common/gamedescription.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/common/gamedescription.js
| 102| 102| 						// Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu
| 103| 103| 						playerDescription = translate("%(playerName)s (%(civ)s, %(state)s)");
| 104| 104| 				else
| 105|    |-					if (isActive)
|    | 105|+				if (isActive)
| 106| 106| 						// Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu
| 107| 107| 						playerDescription = translate("%(playerName)s");
| 108| 108| 					else
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 6.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/common/gamedescription.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/common/gamedescription.js
| 103| 103| 						playerDescription = translate("%(playerName)s (%(civ)s, %(state)s)");
| 104| 104| 				else
| 105| 105| 					if (isActive)
| 106|    |-						// Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu
|    | 106|+				// Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu
| 107| 107| 						playerDescription = translate("%(playerName)s");
| 108| 108| 					else
| 109| 109| 						// Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 5 tabs but found 6.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/common/gamedescription.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/common/gamedescription.js
| 104| 104| 				else
| 105| 105| 					if (isActive)
| 106| 106| 						// Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu
| 107|    |-						playerDescription = translate("%(playerName)s");
|    | 107|+					playerDescription = translate("%(playerName)s");
| 108| 108| 					else
| 109| 109| 						// Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu
| 110| 110| 						playerDescription = translate("%(playerName)s (%(state)s)");
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/common/gamedescription.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/common/gamedescription.js
| 105| 105| 					if (isActive)
| 106| 106| 						// Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu
| 107| 107| 						playerDescription = translate("%(playerName)s");
| 108|    |-					else
|    | 108|+				else
| 109| 109| 						// Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu
| 110| 110| 						playerDescription = translate("%(playerName)s (%(state)s)");
| 111| 111| 			}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 6.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/common/gamedescription.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/common/gamedescription.js
| 106| 106| 						// Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu
| 107| 107| 						playerDescription = translate("%(playerName)s");
| 108| 108| 					else
| 109|    |-						// Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu
|    | 109|+				// Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu
| 110| 110| 						playerDescription = translate("%(playerName)s (%(state)s)");
| 111| 111| 			}
| 112| 112| 		}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 5 tabs but found 6.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/common/gamedescription.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/common/gamedescription.js
| 107| 107| 						playerDescription = translate("%(playerName)s");
| 108| 108| 					else
| 109| 109| 						// Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu
| 110|    |-						playerDescription = translate("%(playerName)s (%(state)s)");
|    | 110|+					playerDescription = translate("%(playerName)s (%(state)s)");
| 111| 111| 			}
| 112| 112| 		}
| 113| 113| 
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1630/display/redirect

elexis retitled this revision from Gamesetup: Use sliders for Ceasefire and VictoryDuration to Gamesetup support for sliders, use it for Ceasefire, RelicCount, RelicDuration, WonderDuration.Jan 22 2020, 9:56 PM
  • Discussed with Vladislav on http://irclogs.wildfiregames.com/2020-01/2020-01-19-QuakeNet-%230ad-dev.log on the problem that not all values are chosen with the same frequency by users
  • The UI theme was discussed with nani in a PM session today. nani proposed that the vertical center axis of the label on the left should be on the same vertical center as the average of the slider + label on the right side:
		Slider
Label:   -----------
		Number

which IMO looks worse.

nani then proposed another style, where the value label and the slider control are in the same location. The slider could be transparent or text rendered on top:


	<sprite name="ModernSliderButton">
		<image backcolor="255 255 255 60"
			size="50%-4 50%-10 50%+4 50%+11"
			border="true"
			bordercolor="70 70 70 50"
		/>
	</sprite>
	<sprite name="ModernSliderLine">
		<image backcolor="255 255 255 10"
			size="0 50%-12 100% 50%+12"
			border="true"
			bordercolor="255 255 255 50"
		/>
	</sprite>

IMO that style looks less appealing than the current slider style, so we agreed to have this version committed first and then if we come up with a better style, it can be applied easily without having to review the code again.

On the code itself, there is mostly the D406 issue remaining, setting the stepWidth=1. Currently there is Math.round added in gamedescription.js and onGameAttributesFinalize, since rounding during the mousemove would change the sliderposition during mousemove, which would be somewhat dysfunctional. With D406, these roundings should be removed.

elexis edited the summary of this revision. (Show Details)Jan 22 2020, 10:07 PM
Stan added a subscriber: Stan.Jan 22 2020, 10:10 PM

I like the example in the description though I think it could a bigger top padding.

I like nani's example (first image) as well.

binaries/data/mods/public/gui/gamesetup/Pages/GameSetupPage/GameSettings/Single/Sliders/WonderDuration.js
62 ↗(On Diff #11147)

Early return?

elexis added inline comments.Jan 22 2020, 10:21 PM
binaries/data/mods/public/gui/gamesetup/Pages/GameSetupPage/GameSettings/Single/Sliders/WonderDuration.js
62 ↗(On Diff #11147)

IMO the use case for an early return is where we can assume that this condition will be necessary for the remainder of the function, but here there could be something coming after that, and it also has the advantage of less indirection if conditions are phrased positively

elexis edited the summary of this revision. (Show Details)Jan 22 2020, 10:21 PM

I like the example in the description though I think it could a bigger top padding.

I could move the value label 1px further below, but if there is a dropdown coming below the dropdown, it will start to collapse with that. There is simply too few space, which is why the setting height was increased from 32px to 36px already:

See also comments from bb today on irc.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 22 2020, 10:38 PM
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Jan 22 2020, 10:38 PM