Page MenuHomeWildfire Games
Differential D805

Use tabs for the options page
ClosedPublic
Actions

Authored by elexis on Aug 20 2017, 12:49 AM.

Details

Reviewers
None
Commits
rP20101: Use tabs for the options page and rewrite it altogether.
Summary

As proposed, discussed and agreed in https://wildfiregames.com/forum/index.php?/topic/22750-options-page-tabbing/
the options page should use tabs, so that it will be easier to add more options in the future.
It also has the advantage that one can identify the options more quickly.

There were different styles discussed (horizontal tabs, multi row and multi column tabs, multi column option controls),
but we have settled on this one.

Changes:
In the JS code, only a for (let i = 0; i < options[category].length; ++i) loop was removed, the contents are still the same.
The dependency code should be rewritten (done). The invertboolean type should be removed some day (done). +1 tab for options.json to be committed indenpendently.
XML duplication nuked.
JS duplication is nuked too in this patch (was planned in D510).
Tab button string changes (removal of "Settings") as also proposed in https://wildfiregames.com/forum/index.php?/topic/22745-all-relics-captured-message/

Test Plan

Just make sure that it works as advertized.

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.Aug 20 2017, 12:49 AM
Vulcan added a subscriber: Vulcan.Aug 20 2017, 1:39 AM

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!
Checking XML files...

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

Vulcan added a comment.Aug 20 2017, 1:41 AM
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/options/options.js
|  33| »   »   button.onPress·=·(category·=>·function()·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'category' is already declared in the upper scope.

binaries/data/mods/public/gui/options/options.js
| 164| »   »   onUpdate·=·function(key,·keyRenderer,·inverted)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'key' is already declared in the upper scope.

binaries/data/mods/public/gui/options/options.js
| 164| »   »   onUpdate·=·function(key,·keyRenderer,·inverted)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'keyRenderer' is already declared in the upper scope.

binaries/data/mods/public/gui/options/options.js
| 164| »   »   onUpdate·=·function(key,·keyRenderer,·inverted)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'inverted' is already declared in the upper scope.

binaries/data/mods/public/gui/options/options.js
| 210| »   »   onUpdate·=·function(key,·callbackFunction,·minvalue,·maxvalue)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'key' is already declared in the upper scope.

binaries/data/mods/public/gui/options/options.js
| 210| »   »   onUpdate·=·function(key,·callbackFunction,·minvalue,·maxvalue)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'callbackFunction' is already declared in the upper scope.

binaries/data/mods/public/gui/options/options.js
| 210| »   »   onUpdate·=·function(key,·callbackFunction,·minvalue,·maxvalue)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'minvalue' is already declared in the upper scope.

binaries/data/mods/public/gui/options/options.js
| 210| »   »   onUpdate·=·function(key,·callbackFunction,·minvalue,·maxvalue)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'maxvalue' is already declared in the upper scope.

binaries/data/mods/public/gui/options/options.js
| 271| »   »   onUpdate·=·function(key,·functionBody,·minval,·maxval)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'key' is already declared in the upper scope.

binaries/data/mods/public/gui/options/options.js
| 271| »   »   onUpdate·=·function(key,·functionBody,·minval,·maxval)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'functionBody' is already declared in the upper scope.

binaries/data/mods/public/gui/options/options.js
| 271| »   »   onUpdate·=·function(key,·functionBody,·minval,·maxval)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'minval' is already declared in the upper scope.

binaries/data/mods/public/gui/options/options.js
| 271| »   »   onUpdate·=·function(key,·functionBody,·minval,·maxval)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'maxval' is already declared in the upper scope.

binaries/data/mods/public/gui/options/options.js
| 316| »   »   onUpdate·=·function(key)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'key' is already declared in the upper scope.

binaries/data/mods/public/gui/options/options.js
|  33| »   »   button.onPress·=·(category·=>·function()·{
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/options/options.js
| 306| »   »   »   »   control.list·=·option.parameters.list.map(e·=>·translate(e.label));
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.
Executing section XML GUI...
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/options/options.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/options/options.xml
|   1|   1| <?xml version="1.0" encoding="utf-8"?>
|   2|    |-
|   3|   2| <!--
|   4|   3| ==========================================
|   5|   4| - Options Window -
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/options/options.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/options/options.xml
|   5|   5| - Options Window -
|   6|   6| ==========================================
|   7|   7| -->
|   8|    |-
|   9|   8| <objects>
|  10|   9| 	<script file="gui/common/functions_global_object.js"/>
|  11|  10| 	<script file="gui/options/options.js"/>
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/options/options.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/options/options.xml
|  19|  19| 		<!-- Category Tabs -->
|  20|  20| 		<object name="tabButtons" type="image" size="20 30 210 100%-54">
|  21|  21| 			<repeat count="15">
|  22|    |-				<object
|  23|    |-					name="tabButton[n]"
|  24|    |-					type="button"
|  25|    |-					sprite="ModernTabButtonVertical"
|  26|    |-					sound_pressed="audio/interface/ui/ui_button_click.ogg"
|  27|    |-					size="0 0 190 30"
|  28|    |-					hidden="true"
|  29|    |-				>
|    |  22|+				<object name="tabButton[n]" type="button" sprite="ModernTabButtonVertical" sound_pressed="audio/interface/ui/ui_button_click.ogg" size="0 0 190 30" hidden="true">
|  30|  23| 					<object type="text" name="tabButtonText[n]" style="ModernLabelText" ghost="true"/>
|  31|  24| 				</object>
|  32|  25| 			</repeat>
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/mod/gui/common/modern/styles.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/mod/gui/common/modern/styles.xml
|   1|    |-<?xml version = "1.0" encoding = "utf-8"?>
|   2|    |-
|    |   1|+<?xml version="1.0" encoding="utf-8"?>
|   3|   2| <styles>
|   4|   3| 	<!--
|   5|   4| 	==========================================
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/mod/gui/common/modern/styles.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/mod/gui/common/modern/styles.xml
|   7|   7| 	- be called by every object before any other style is loaded.
|   8|   8| 	==========================================
|   9|   9| 	-->
|  10|    |-	<style name="default"
|  11|    |-		absolute="false"
|  12|    |-		enabled="true"
|  13|    |-		ghost="false"
|  14|    |-		hidden="false"
|  15|    |-		size="0 0 100% 100%"
|  16|    |-		z="0"
|  17|    |-		font="sans-14"
|  18|    |-		buffer_zone="5"
|  19|    |-		multiline="false"
|  20|    |-	/>
|  21|    |-	<style name = "ModernWindow"
|  22|    |-		sprite = "ModernWindow"
|  23|    |-		buffer_zone = "12"
|  24|    |-		text_align = "left"
|  25|    |-		text_valign = "top"
|  26|    |-	/>
|  27|    |-	<style name = "ModernDialog"
|  28|    |-		sprite = "ModernDialog"
|  29|    |-		buffer_zone = "12"
|  30|    |-		text_align = "left"
|  31|    |-		text_valign = "top"
|  32|    |-	/>
|  33|    |-	<style name = "ModernList"
|  34|    |-		buffer_zone = "5"
|  35|    |-		font = "sans-bold-stroke-14"
|  36|    |-		heading_height="25"
|  37|    |-		scrollbar = "true"
|  38|    |-		scrollbar_style = "ModernScrollBar"
|  39|    |-		sprite = "ModernDarkBoxGoldNoTop"
|  40|    |-		sprite_selectarea = "ModernDarkBoxWhite"
|  41|    |-		sprite_heading = "ModernDarkBoxGoldNoBottom"
|  42|    |-		textcolor = "white"
|  43|    |-		textcolor_selected = "white"
|  44|    |-		text_align = "left"
|  45|    |-		text_valign = "center"
|  46|    |-		sound_selected = "audio/interface/ui/ui_button_click.ogg"
|  47|    |-	/>
|  48|    |-	<style name="ModernDropDown"
|  49|    |-		dropdown_buffer="1"
|  50|    |-		font="sans-bold-14"
|  51|    |-		textcolor="white"
|  52|    |-		text_align="left"
|  53|    |-		text_valign="center"
|  54|    |-
|  55|    |-		sprite="ModernDarkBoxGold"
|  56|    |-		sprite_disabled="ModernDarkBoxGoldDisabled"
|  57|    |-		button_width="16"
|  58|    |-		sprite2="ModernDropDownArrow"
|

http://jw:8080/job/phabricator_lint/418/ for more details.

Harbormaster completed remote builds in B2839: Diff 3194.Aug 20 2017, 1:42 AM
vladislavbelov added a subscriber: vladislavbelov.EditedAug 20 2017, 2:11 PM

I've tested it. It looks and works good for me. Now, if we have tabs, I think it's more useful and obvious for player, that the Revert (maybe Reset too) button should revert/reset only the current tab.

Also I have in mind how it could look. I made a patch:

BeforeAfter

changes.patch40 KBDownload

binaries/data/mods/public/gui/options/options.js
29 ↗(On Diff #3194)

What's the hardcoded values? Magic numbers around +s are mostly obvious, because it's like shifting, but what magic numbers around * do? I think need a comment or a const var.

99 ↗(On Diff #3194)

Why this indent? Couldn't we use the position shift?

elexis added a comment.Aug 20 2017, 2:57 PM

Unfortunately it's about impossible to find out what changes you did in the changes.patch unless I upload it to phabricator, so going to do that (might revert it).

binaries/data/mods/public/gui/options/options.js
29 ↗(On Diff #3194)

I personally would prefer to rewrite the options page code altogether (refs old patch uploaded as a file attachment to D510)
This part of the code was only changed by one tab and I don't want to change any logics in this patch yet to make it easier to review what actually changed and to make it easier to revert a fraudulent logic change.

99 ↗(On Diff #3194)

This only applies to dependencies and it's already in the existing code.
So if there's a fix, it should be done separately IMO

elexis updated this revision to Diff 3201.Aug 20 2017, 2:59 PM

Upload Vladislavs patch to see what changes he proposed.

Harbormaster completed remote builds in B2843: Diff 3201.Aug 20 2017, 2:59 PM
elexis planned changes to this revision.EditedAug 20 2017, 3:12 PM
  • Disagree with reducing the space at the top
  • Disagree with adding the black background to the tabs.
  • Agree with increasing the button width
  • Disagree with not giving all buttons the same width
  • Agree with moving the buttons further to the border
  • Reverting only the current tab might be awkward. If there were changes on another tab, the close button might complain about unsaved changes and the revert button doesn't fix it. But that can be solved with a tooltip.
Vulcan added a comment.Aug 20 2017, 5:05 PM

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!
Checking XML files...

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

Vulcan added a comment.Aug 20 2017, 5:06 PM
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/options/options.js
|  33| »   »   button.onPress·=·(category·=>·function()·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'category' is already declared in the upper scope.

binaries/data/mods/public/gui/options/options.js
| 164| »   »   onUpdate·=·function(key,·keyRenderer,·inverted)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'key' is already declared in the upper scope.

binaries/data/mods/public/gui/options/options.js
| 164| »   »   onUpdate·=·function(key,·keyRenderer,·inverted)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'keyRenderer' is already declared in the upper scope.

binaries/data/mods/public/gui/options/options.js
| 164| »   »   onUpdate·=·function(key,·keyRenderer,·inverted)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'inverted' is already declared in the upper scope.

binaries/data/mods/public/gui/options/options.js
| 210| »   »   onUpdate·=·function(key,·callbackFunction,·minvalue,·maxvalue)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'key' is already declared in the upper scope.

binaries/data/mods/public/gui/options/options.js
| 210| »   »   onUpdate·=·function(key,·callbackFunction,·minvalue,·maxvalue)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'callbackFunction' is already declared in the upper scope.

binaries/data/mods/public/gui/options/options.js
| 210| »   »   onUpdate·=·function(key,·callbackFunction,·minvalue,·maxvalue)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'minvalue' is already declared in the upper scope.

binaries/data/mods/public/gui/options/options.js
| 210| »   »   onUpdate·=·function(key,·callbackFunction,·minvalue,·maxvalue)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'maxvalue' is already declared in the upper scope.

binaries/data/mods/public/gui/options/options.js
| 271| »   »   onUpdate·=·function(key,·functionBody,·minval,·maxval)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'key' is already declared in the upper scope.

binaries/data/mods/public/gui/options/options.js
| 271| »   »   onUpdate·=·function(key,·functionBody,·minval,·maxval)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'functionBody' is already declared in the upper scope.

binaries/data/mods/public/gui/options/options.js
| 271| »   »   onUpdate·=·function(key,·functionBody,·minval,·maxval)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'minval' is already declared in the upper scope.

binaries/data/mods/public/gui/options/options.js
| 271| »   »   onUpdate·=·function(key,·functionBody,·minval,·maxval)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'maxval' is already declared in the upper scope.

binaries/data/mods/public/gui/options/options.js
| 316| »   »   onUpdate·=·function(key)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'key' is already declared in the upper scope.

binaries/data/mods/public/gui/options/options.js
|  33| »   »   button.onPress·=·(category·=>·function()·{
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/options/options.js
| 306| »   »   »   »   control.list·=·option.parameters.list.map(e·=>·translate(e.label));
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.
Executing section XML GUI...
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/options/options.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/options/options.xml
|   1|   1| <?xml version="1.0" encoding="utf-8"?>
|   2|    |-
|   3|   2| <!--
|   4|   3| ==========================================
|   5|   4| - Options Window -
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/options/options.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/options/options.xml
|   5|   5| - Options Window -
|   6|   6| ==========================================
|   7|   7| -->
|   8|    |-
|   9|   8| <objects>
|  10|   9| 	<script file="gui/common/functions_global_object.js"/>
|  11|  10| 	<script file="gui/options/options.js"/>
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/options/options.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/options/options.xml
|  19|  19| 		<!-- Category Tabs -->
|  20|  20| 		<object name="tabButtons" type="image" sprite="ModernDarkBoxGold" size="15 16 210 100%-52">
|  21|  21| 			<repeat count="15">
|  22|    |-				<object
|  23|    |-					name="tabButton[n]"
|  24|    |-					type="button"
|  25|    |-					sprite="ModernTabButtonVertical"
|  26|    |-					sound_pressed="audio/interface/ui/ui_button_click.ogg"
|  27|    |-					size="0 0 195 30"
|  28|    |-					hidden="true"
|  29|    |-				>
|    |  22|+				<object name="tabButton[n]" type="button" sprite="ModernTabButtonVertical" sound_pressed="audio/interface/ui/ui_button_click.ogg" size="0 0 195 30" hidden="true">
|  30|  23| 					<object type="text" name="tabButtonText[n]" style="ModernLabelText" ghost="true"/>
|  31|  24| 				</object>
|  32|  25| 			</repeat>
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/mod/gui/common/modern/styles.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/mod/gui/common/modern/styles.xml
|   1|    |-<?xml version = "1.0" encoding = "utf-8"?>
|   2|    |-
|    |   1|+<?xml version="1.0" encoding="utf-8"?>
|   3|   2| <styles>
|   4|   3| 	<!--
|   5|   4| 	==========================================
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/mod/gui/common/modern/styles.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/mod/gui/common/modern/styles.xml
|   7|   7| 	- be called by every object before any other style is loaded.
|   8|   8| 	==========================================
|   9|   9| 	-->
|  10|    |-	<style name="default"
|  11|    |-		absolute="false"
|  12|    |-		enabled="true"
|  13|    |-		ghost="false"
|  14|    |-		hidden="false"
|  15|    |-		size="0 0 100% 100%"
|  16|    |-		z="0"
|  17|    |-		font="sans-14"
|  18|    |-		buffer_zone="5"
|  19|    |-		multiline="false"
|  20|    |-	/>
|  21|    |-	<style name = "ModernWindow"
|  22|    |-		sprite = "ModernWindow"
|  23|    |-		buffer_zone = "12"
|  24|    |-		text_align = "left"
|  25|    |-		text_valign = "top"
|  26|    |-	/>
|  27|    |-	<style name = "ModernDialog"
|  28|    |-		sprite = "ModernDialog"
|  29|    |-		buffer_zone = "12"
|  30|    |-		text_align = "left"
|  31|    |-		text_valign = "top"
|  32|    |-	/>
|  33|    |-	<style name = "ModernList"
|  34|    |-		buffer_zone = "5"
|  35|    |-		font = "sans-bold-stroke-14"
|  36|    |-		heading_height="25"
|  37|    |-		scrollbar = "true"
|  38|    |-		scrollbar_style = "ModernScrollBar"
|  39|    |-		sprite = "ModernDarkBoxGoldNoTop"
|  40|    |-		sprite_selectarea = "ModernDarkBoxWhite"
|  41|    |-		sprite_heading = "ModernDarkBoxGoldNoBottom"
|  42|    |-		textcolor = "white"
|  43|    |-		textcolor_selected = "white"
|  44|    |-		text_align = "left"
|  45|    |-		text_valign = "center"
|  46|    |-		sound_selected = "audio/interface/ui/ui_button_click.ogg"
|  47|    |-	/>
|  48|    |-	<style name="ModernSortedList"
|  49|    |-		buffer_zone="5"
|  50|    |-		font="sans-bold-stroke-14"
|  51|    |-		heading_height="25"
|  52|    |-		scrollbar="true"
|  53|    |-		scrollbar_style="ModernScrollBar"
|  54|    |-		sprite="ModernDarkBoxGoldNoTop"
|  55|    |-		sprite_selectarea="ModernDarkBoxWhite"
|  56|    |-		sprite_heading="ModernDarkBoxGol

http://jw:8080/job/phabricator_lint/420/ for more details.

Harbormaster completed remote builds in B2843: Diff 3201.Aug 20 2017, 5:06 PM
vladislavbelov added a comment.Aug 20 2017, 5:10 PM
In D805#32063, @elexis wrote:

Disagree with reducing the space at the top

Probably ok, but I like to have equal distance for all directions (top, bottom, left, right).

Disagree with adding the black background to the tabs.

Why? It looks more consistent.

Disagree with not giving all buttons the same width

Ok, we could move all buttons under the tab.

Reverting only the current tab might be awkward. If there were changes on another tab, the close button might complain about unsaved changes and the revert button doesn't fix it. But that can be solved with a tooltip.

Nope, just complain about unsaved not on close, but on tab change, like in many games.

ffffffff added a subscriber: ffffffff.Aug 20 2017, 5:18 PM

considerable tab switch hotkey switching tabs and/or remember last opened tab as code D810

elexis mentioned this in D839: Replace Options page Renderer functions with generic Function.Aug 27 2017, 5:57 PM
elexis mentioned this in rP20092: Options page cleanup..Sep 1 2017, 4:23 PM
elexis mentioned this in D510: Add scopes to the options page and use let.Sep 2 2017, 12:36 AM
elexis updated this revision to Diff 3462.Sep 2 2017, 12:41 AM

Following rP20010, rP20066, rP20091 and rP20092,
remove the entire duplication beyond what was envisioned in D510,
so that there is no more corners and edges in the code and
doing things like reverting only one tab is easily possible.

However keep the reverting and resetting global for now.
(Because it might either be confusing to the user if reverting doesn't revert all changes and
because it will be annoying to the user if the window complains each time about unsaved changes when switching the tab.)

elexis updated this revision to Diff 3463.Sep 2 2017, 12:44 AM

Remove a leftover

elexis edited the summary of this revision. (Show Details)Sep 2 2017, 12:50 AM
Vulcan added a comment.Sep 2 2017, 1:27 AM

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!
Checking XML files...

http://jenkins-master:8080/job/phabricator/1950/ for more details.

Vulcan added a comment.Sep 2 2017, 1:29 AM
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/options/options.js
| 158| »   »   button.onPress·=·(category·=>·function()·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'category' is already declared in the upper scope.

binaries/data/mods/public/gui/options/options.js
| 220| »   »   »   let·value·=·optionType.guiToValue(control);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'value' is already declared in the upper scope.

binaries/data/mods/public/gui/options/options.js
| 158| »   »   button.onPress·=·(category·=>·function()·{
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/options/options.js
| 217| »   »   control[optionType.guiSetter]·=·function()·{
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.
Executing section XML GUI...
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/options/options.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/options/options.xml
|   1|   1| <?xml version="1.0" encoding="utf-8"?>
|   2|    |-
|   3|   2| <objects>
|   4|   3| 
|   5|   4| 	<script file="gui/common/functions_global_object.js"/>
Executing section Python...
Executing section Perl...

http://jenkins-master:8080/job/phabricator_lint/467/ for more details.

Harbormaster completed remote builds in B2978: Diff 3462.Sep 2 2017, 1:29 AM
Vulcan added a comment.Sep 2 2017, 2:16 AM

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!
Checking XML files...

http://jenkins-master:8080/job/phabricator/1951/ for more details.

Vulcan added a comment.Sep 2 2017, 2:17 AM
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/options/options.js
| 158| »   »   button.onPress·=·(category·=>·function()·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'category' is already declared in the upper scope.

binaries/data/mods/public/gui/options/options.js
| 220| »   »   »   let·value·=·optionType.guiToValue(control);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'value' is already declared in the upper scope.

binaries/data/mods/public/gui/options/options.js
| 158| »   »   button.onPress·=·(category·=>·function()·{
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/options/options.js
| 217| »   »   control[optionType.guiSetter]·=·function()·{
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.
Executing section XML GUI...
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/options/options.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/options/options.xml
|   1|   1| <?xml version="1.0" encoding="utf-8"?>
|   2|    |-
|   3|   2| <objects>
|   4|   3| 
|   5|   4| 	<script file="gui/common/functions_global_object.js"/>
Executing section Python...
Executing section Perl...

http://jenkins-master:8080/job/phabricator_lint/468/ for more details.

Harbormaster completed remote builds in B2979: Diff 3463.Sep 2 2017, 2:17 AM
mimo added a subscriber: mimo.Sep 2 2017, 3:41 PM

I find it more clear when the dependent options are slightly indented which is currently not working although code for that is there.

binaries/data/mods/public/gui/options/options.js
235 ↗(On Diff #3463)

indent

239 ↗(On Diff #3463)

indent

240 ↗(On Diff #3463)

indent + DependentLabelIndentation did not work when i tested that patch

elexis added a comment.Sep 3 2017, 11:18 PM

Thanks for the quick testing mimo! The missing indentation was caused by a typo dependency vs dependencies.

I've included the darker background for the tabs as proposed by Vladislav. Seems to be a bit nicer.
As agreed with Vladislav on irc a day or two ago, we can do the reseting and reverting of settings per page separately. It needs some GUI design first.
The buttons were not repositioned because I'm not sure if it looks better to do so. But that can be changed later easily.

invalid dropdown values:
The answer to "TODO: what happens if someone provides a non-existing option entry for a dropdown?" is that the options dialog selects item -1 (i.e. no selection).
The options dialog doesn't change or save the setting.
(The user of the config value has to deal with broken settings independently of the sanity of the options dialog.)

number sanitization:
The number sanitization didn't work as good as it might.
(1) It should display the true number loaded from the config, even if that exceeds the intended range.
(2) If the value was edited and sanitized, the capped number was saved to the config, but the display was not updated.
Fixed both.

tooltips:
While at it, might just as well display the min/max values for numbers in the tooltip, like we do for sliders.
While at it, nuking this oldSetter workaround and adding a tooltip function that is always called when the GUI value is changed.

NaN numbers:
The NaN handling for numerical input fields is not ideal (ideal would be reseting to the default if its NaN. Could possibly be added later to sanitizeValue with ugly workaround code).
The a22 options dialog is fine with displaying arbitrary strings in the number boxes, so its an improvement at least.
The user has to select NaN and type a number or press the revert/reset button to fix the value.
(Fun experiment: keep appending a 9 to the number to the chat backlog and first the number is converted to a seemingly arbitrary potency of 10, then it uses the exponential representation, then it becomes Infinity, then it becomes NaN)
One might consider only sanitizing when pressing return, but not too happy about that because it would not sanitize in case of never pressing enter.

Hotloading: While looking at hotloading code today, I've added a small helper function that restores the currently selected tab if options.js was modified.

Also removed few unneeded +x, an unused argument, inlined two variables, rephrased two tooltips and checked against jshint.

wowgetoffyourcellphone added a subscriber: wowgetoffyourcellphone.Sep 3 2017, 11:26 PM
Closed by commit rP20101: Use tabs for the options page and rewrite it altogether. (authored by elexis). · Explain WhySep 3 2017, 11:39 PM
This revision was automatically updated to reflect the committed changes.
elexis mentioned this in rP20102: Remove unneeded parameters nesting from the options page following rP17657….Sep 3 2017, 11:59 PM
elexis mentioned this in rP20103: Add +1 tab indentation following rP20101 / D805 for the lines not affected by….Sep 4 2017, 12:08 AM
elexis mentioned this in rP20106: Fix oversight in rP20101 (thought undefined was converted to emptystring….Sep 4 2017, 1:49 PM
elexis mentioned this in D332: Add an option in the Options menu to change the UI scale.Sep 4 2017, 7:03 PM
vladislavbelov mentioned this in D1017: Change option button positions.Nov 7 2017, 8:49 PM

Revision Contents
Changeset List

PathSize
ps/
trunk/
binaries/
data/
mods/
public/
gui/
options/
522 lines
40 lines
92 lines

Diff 3474

View Options

ps/trunk/binaries/data/mods/public/gui/options/options.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/options/options.json

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/options/options.xml

Loading...
Wildfire Games · Phabricator