Page MenuHomeWildfire Games
Differential D862

Arbitrary order between checkboxes and dropdowns in gamesetup
ClosedPublic
Actions

Authored by bb on Sep 1 2017, 4:27 PM.

Details

Reviewers
elexis
Commits
rP20116: Allow arbitrary order between dropdowns and checkboxes in gamesetup
Trac Tickets
#4014
Summary

currently in the mapOption and moreOptions panel the dropdowns are always displayed above the checkboxes. This patch allows an arbitrary order of them.
Also the init of these can be in an arbitrary order (instead of doing all dropdowns first and later he checkboxes).
This is required for #4014 since we don't want f.e. the victoryDuration dropdown above the victory condition checkboxes.

Test Plan

Make sure the front doesn't fall of when setting a dropdown between the checkboxes and other way around.

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

bb created this revision.Sep 1 2017, 4:27 PM
Owners added a subscriber: Restricted Owners Package.Sep 1 2017, 4:27 PM
bb added inline comments.Sep 1 2017, 4:31 PM
binaries/data/mods/public/gui/gamesetup/gamesetup.js
347 ↗(On Diff #3452)

We might need to add a comment that "type" is either Dropdown or Checkbox but no strong opinion

binaries/data/mods/public/gui/gamesetup/gamesetup.xml
179 ↗(On Diff #3452)

instead of setting these hidden things and duplicating the guiObject, we could also change the type and style for these objects. This way one couldn't set a dropdown and a checkbox in the same place. However the current implementation leave shorter code.

elexis added a comment.Sep 1 2017, 4:36 PM

That addition is welcome!

  • The two objects in g_OptionOrderGUI could become arrays of string (i.e. avoid the type repetition) if the code using it looks up the type on it's own by testing the two globals.
  • Why is hidden added to the XML? The GUI objects must receive that property from JS, so the XML property is overwritten at any time, thus it should be misleading to specify it in the XML.
binaries/data/mods/public/gui/gamesetup/gamesetup.xml
187 ↗(On Diff #3452)

good

284 ↗(On Diff #3452)

It was 15 and 15 because 15 was the most controls that fit to 1024 if I recall correctly.
But I don't mind if the number exceeds, because mods.

335 ↗(On Diff #3452)

good

elexis awarded a token.Sep 1 2017, 4:36 PM
bb added a comment.Sep 1 2017, 5:06 PM
In D862#33642, @elexis wrote:
  • The two objects in g_OptionOrderGUI could become arrays of string (i.e. avoid the type repetition) if the code using it looks up the type on it's own by testing the two globals.

That way we need to look up the type of an option in g_Dropdowns/g_Checkboxes, so implementing more types of options becomes a little more complex. but meh

  • Why is hidden added to the XML? The GUI objects must receive that property from JS, so the XML property is overwritten at any time, thus it should be misleading to specify it in the XML.

The problem here is that we assign a dropdown and a checkbox in the frame, but only one needs to be visible, so what happens now is both are hidden and gamesetup makes one visible in updateDropdown/Checkbox. When we delete the hidden tag from the xml, both will be visible resulting in an ugly state. We could maybe hide the wrong one in updateDropdown/Checkbox though.

elexis added a comment.Sep 1 2017, 5:37 PM
In D862#33647, @bb wrote:

That way we need to look up the type of an option in g_Dropdowns/g_Checkboxes, so implementing more types of options becomes a little more complex. but meh

Two if statements that would avoid the duplication in the manifest object.

for (let option of g_OptionOrderGUI)
{
       if (g_Dropdowns[option])
          g_Dropdowns[option].funnyStuff();

       if (g_Checkboxes[option])
          g_Checkboxes[option].funnyStuff();}

(Still wondering if we shouldn't warn if people forget an entry in that array. But I guess that could also be considered mod support to remove options like that.)

Why is hidden added to the XML?

only one needs to be visible

The hidden property of all controls must be set in the updateGUI method, since the options depend on the map selection.

bb added a comment.Sep 1 2017, 5:56 PM

Why is hidden added to the XML?

only one needs to be visible

The hidden property of all controls must be set in the updateGUI method, since the options depend on the map selection.

It is set from updateGUI method, but I only set it off by default, anyway changed it (albeit it leave ugly code)

bb updated this revision to Diff 3453.Sep 1 2017, 5:58 PM

see above

Vulcan added a subscriber: Vulcan.Sep 1 2017, 5:59 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://jenkins-master:8080/job/phabricator/1948/ for more details.

Vulcan added a comment.Sep 1 2017, 6:01 PM
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1596| »   while·(g_IsNetworked)
|    | [NORMAL] ESLintBear (no-unmodified-loop-condition):
|    | 'g_IsNetworked' is not modified in this loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1578| »   if·(g_LoadingState·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1630| »   »   if·(playerData.some((pData,·j)·=>·i·!=·j·&&·sameColor(playerData[i].Color,·pData.Color)))
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1631| »   »   »   playerData[i].Color·=·g_PlayerColorPickerList.find(color·=>·playerData.every(pData·=>·!sameColor(color,·pData.Color)));
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1808| »   »   »   chosenCiv·=·pickRandom(Object.keys(g_CivData).filter(civ·=>·g_CivData[civ].Culture·==·culture));
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1822| »   »   let·usedName·=·g_GameAttributes.settings.PlayerData.filter(pData·=>·pData.Name·&&·pData.Name.indexOf(chosenName)·!==·-1).length;
|    | [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/gamesetup/gamesetup.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|   1|   1| <?xml version="1.0" encoding="utf-8"?>
|   2|    |-
|   3|   2| <objects>
|   4|   3| 
|   5|   4| 	<script file="gui/common/color.js"/>
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|  59|  59| 						<translatableAttribute id="caption">Civilization</translatableAttribute>
|  60|  60| 					</object>
|  61|  61| 
|  62|    |-					<object name="civInfoButton"
|  63|    |-						type="button"
|  64|    |-						style="IconButton"
|  65|    |-						sprite="iconInfoGold"
|  66|    |-						sprite_over="iconInfoWhite"
|  67|    |-						size="85%-37 0 85%-21 16"
|  68|    |-					>
|    |  62|+					<object name="civInfoButton" type="button" style="IconButton" sprite="iconInfoGold" sprite_over="iconInfoWhite" size="85%-37 0 85%-21 16">
|  69|  63| 						<translatableAttribute id="tooltip">View civilization info</translatableAttribute>
|  70|  64| 						<action on="Press"><![CDATA[
|  71|  65| 							Engine.PushGuiPage("page_civinfo.xml");
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|  72|  72| 						]]></action>
|  73|  73| 					</object>
|  74|  74| 
|  75|    |-					<object name="civResetButton"
|  76|    |-						type="button"
|  77|    |-						style="IconButton"
|  78|    |-						sprite="iconResetGold"
|  79|    |-						sprite_over="iconResetWhite"
|  80|    |-						size="85%-16 0 85% 16"
|  81|    |-					>
|    |  75|+					<object name="civResetButton" type="button" style="IconButton" sprite="iconResetGold" sprite_over="iconResetWhite" size="85%-16 0 85% 16">
|  82|  76| 						<translatableAttribute id="tooltip">Reset any civilizations that have been selected to the default (random)</translatableAttribute>
|  83|  77| 						<action on="Press">resetCivilizations();</action>
|  84|  78| 					</object>
|    | [INFO] XMLBear:

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

Harbormaster completed remote builds in B2972: Diff 3452.Sep 1 2017, 6:01 PM
elexis added a comment.Sep 1 2017, 6:08 PM

Looks much better to me without that duplication. There should be only one source of truth (https://en.wikipedia.org/wiki/Single_source_of_truth).

binaries/data/mods/public/gui/gamesetup/gamesetup.js
1694 ↗(On Diff #3453)

dropdown.hidden = guiType != dropdown || ... ?

binaries/data/mods/public/gui/gamesetup/gamesetup.xml
152 ↗(On Diff #3453)

mapOptionFrame?

291 ↗(On Diff #3453)

moreOptionTitle, moreOptionText?

bb added inline comments.Sep 1 2017, 6:15 PM
binaries/data/mods/public/gui/gamesetup/gamesetup.js
1694 ↗(On Diff #3453)

nope since that should be done from updateGUICheckbox
(notice there are 3 options for every object: dropdown, checkbox and null)

elexis added inline comments.Sep 1 2017, 6:17 PM
binaries/data/mods/public/gui/gamesetup/gamesetup.js
1694 ↗(On Diff #3453)

Why aren't all settings of a dropdown set in the method that is supposed to set all properties for the dropdown and nothing else?
null? If there are more controls coming up like a text input field or a slider, then there would be methods setting those properties and hiding themselves.

bb added inline comments.Sep 1 2017, 6:22 PM
binaries/data/mods/public/gui/gamesetup/gamesetup.js
1694 ↗(On Diff #3453)

in the xml every optionframe has both the checkbox and the dropdown object in it, ofc only one should be visible, so we have got the set the dropdown and the checkbox should always be disabled on the same position. maybe we should go with my first inline

instead of setting these hidden things and duplicating the guiObject, we could also change the type and style for these objects. This way one couldn't set a dropdown and a checkbox in the same place. However the current implementation leave shorter code.

Vulcan added a comment.Sep 1 2017, 6:49 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://jenkins-master:8080/job/phabricator/1949/ for more details.

Vulcan added a comment.Sep 1 2017, 6:50 PM
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1598| »   while·(g_IsNetworked)
|    | [NORMAL] ESLintBear (no-unmodified-loop-condition):
|    | 'g_IsNetworked' is not modified in this loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1005| »   »   if·(g_OptionOrderInit.every(option·=>·option·!=·dropdown))
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1009| »   »   if·(g_OptionOrderInit.every(option·=>·option·!=·checkbox))
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1053| »   »   let·idx·=·g_OptionOrderGUI[panel].indexOf(name)
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1580| »   if·(g_LoadingState·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1632| »   »   if·(playerData.some((pData,·j)·=>·i·!=·j·&&·sameColor(playerData[i].Color,·pData.Color)))
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1633| »   »   »   playerData[i].Color·=·g_PlayerColorPickerList.find(color·=>·playerData.every(pData·=>·!sameColor(color,·pData.Color)));
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1816| »   »   »   chosenCiv·=·pickRandom(Object.keys(g_CivData).filter(civ·=>·g_CivData[civ].Culture·==·culture));
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1830| »   »   let·usedName·=·g_GameAttributes.settings.PlayerData.filter(pData·=>·pData.Name·&&·pData.Name.indexOf(chosenName)·!==·-1).length;
|    | [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/gamesetup/gamesetup.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|   1|   1| <?xml version="1.0" encoding="utf-8"?>
|   2|    |-
|   3|   2| <objects>
|   4|   3| 
|   5|   4| 	<script file="gui/common/color.js"/>
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|  59|  59| 						<translatableAttribute id="caption">Civilization</translatableAttribute>
|  60|  60| 					</object>
|  61|  61| 
|  62|    |-					<object name="civInfoButton"
|  63|    |-						type="button"
|  64|    |-						style="IconButton"
|  65|    |-						sprite="iconInfoGold"
|  66|    |-						sprite_over="iconInfoWhite"
|  67|    |-						size="85%-37 0 85%-21 16"
|  68|    |-					>
|    |  62|+					<object name="civInfoButton" type="button" style="IconButton" sprite="iconInfoGold" sprite_over="iconInfoWhite" size="85%-37 0 85%-21 16">
|  69|  63| 						<translatableAttribute id="tooltip">View civilization info</translatableAttribute>
|  70|  64| 						<action on="Press"><![CDATA[
|  71|  65| 							Engine.PushGuiPage("page_civinfo.xml");
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|  72|  72| 						]]></action>
|  73|  73| 					</object>
|  74|  74| 
|  75|    |-					<object name="civResetButton"
|  76|    |-						type="button"
|  77|    |-						st

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

Harbormaster completed remote builds in B2974: Diff 3453.Sep 1 2017, 6:50 PM
bb added inline comments.Sep 2 2017, 9:38 PM
binaries/data/mods/public/gui/gamesetup/gamesetup.js
1895–1897 ↗(On Diff #3453)

Another option is to for the problem described above is to hide the dropdowns/checkboxes from here aswell.

bb added a comment.Sep 3 2017, 9:43 PM

I have been testing out the options above, my results here:

binaries/data/mods/public/gui/gamesetup/gamesetup.js
1694 ↗(On Diff #3453)

Actually this doesn't work at all since we can't/shouldn't change the type tag of the guiObject.

1895–1897 ↗(On Diff #3453)

This would actually leave extremely ugly and errorprone code, since besides the repeat in the moreOption object there are some more object, which doesn't have the checkboxes/dropdowns, so we got to filter those out manually when looping over them, or we need to make a new object (for both map and more) containing the repeat and hiding in that too.

imo both alternatives are worse than the proposed solution (open for arguments if you think differently or when having another solution). Another solution would be to hide all option from updateGUIDropdown/Checkbox and a few lines below possible unhide it. We could ofc make a function for this (would nuke some duplication especially when implementing more optionTypes)

bb updated this revision to Diff 3478.Sep 4 2017, 1:23 PM

Implement the function as suggested in last inline

Vulcan added a comment.Sep 4 2017, 2:12 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://jenkins-master:8080/job/phabricator/1962/ for more details.

Vulcan added a comment.Sep 4 2017, 2:15 PM
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1599| »   while·(g_IsNetworked)
|    | [NORMAL] ESLintBear (no-unmodified-loop-condition):
|    | 'g_IsNetworked' is not modified in this loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1006| »   »   if·(g_OptionOrderInit.every(option·=>·option·!=·dropdown))
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1010| »   »   if·(g_OptionOrderInit.every(option·=>·option·!=·checkbox))
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1054| »   »   let·idx·=·g_OptionOrderGUI[panel].indexOf(name)
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1581| »   if·(g_LoadingState·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1633| »   »   if·(playerData.some((pData,·j)·=>·i·!=·j·&&·sameColor(playerData[i].Color,·pData.Color)))
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1634| »   »   »   playerData[i].Color·=·g_PlayerColorPickerList.find(color·=>·playerData.every(pData·=>·!sameColor(color,·pData.Color)));
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1693| »   if·(guiType·!=·"")
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with ''.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1738| »   if·(guiType·!=·"")
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with ''.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1823| »   »   »   chosenCiv·=·pickRandom(Object.keys(g_CivData).filter(civ·=>·g_CivData[civ].Culture·==·culture));
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1837| »   »   let·usedName·=·g_GameAttributes.settings.PlayerData.filter(pData·=>·pData.Name·&&·pData.Name.indexOf(chosenName)·!==·-1).length;
|    | [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/gamesetup/gamesetup.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|   1|   1| <?xml version="1.0" encoding="utf-8"?>
|   2|    |-
|   3|   2| <objects>
|   4|   3| 
|   5|   4| 	<script file="gui/common/color.js"/>
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|  59|  59| 						<translatableAttribute id="caption">Civilization</translatableAttribute>
|  60|  60| 					</object>
|  61|  61| 
|  62|    |-					<object name="civInfoButton"
|  63|    |-						type="button"
|  64|    |-						style="IconButton"
|  65|    |-						sprite="iconInfoGold"
|  66|    |-						sprite_over="iconInfoWhite"
|  67|    |-						size="85%-37 0 85%-21 16"
|  68|    |-					>
|    |  62|+					<object name="civInfoButton" type="button" style="IconButton" sprite="iconInfoGold" sprite_over="iconInfoWhite" size="85%-37 0 85%-21 16">
|  69|  63| 						<translatableAttribute id="tooltip">View civilization info</translatableAttribute>
|  70|  64| 						<action on="Press"><![CDATA[
|  71|  65| 							Engine.PushGuiPage("page_civinfo.xml");
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|  72|  72| 						]]></action>
|  73|  73| 					</object>
|  74|  74| 
|  75|    |-					<object name="civResetButton"
|  76|    |-						type="button"
|  77|    |-						style="IconButton"
|  78|    |-						sprite="iconResetGold"
|  79|    |-						sprite_over="iconResetWhite"
|  80|    |-						size="85%-16 0 85% 16"
|  81|    |-					>
|    |  75|+					<object name="civResetButton" type="button" style="IconButton" sprite="iconResetGold" sprite_over="iconResetWhite" size="85%-16 0 85% 16">
|  82|  76| 						<translatableAttribute id="tooltip">Reset any civilizations that have been selected to the default (random)</translatableAttribute>
|  83|  77| 						<action on="Press">resetCivilizations();</action>
|  84|  78| 					</object>
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|  87|  87| 						<translatableAttribute id="caption">Team</translatableAttribute>
|  88|  88| 					</object>
|  89|  89| 
|  90|    |-					<object name="teamResetButton"
|  91|    |-						type="button"
|  92|    |-						style="IconButton"
|  93|    |-						sprite="iconResetGold"
|  94|    |-						sprite_over="iconResetWhite"
|  95|    |-						size="100%-21 0 100%-5 16"
|  96|    |-					>
|    |  90|+					<object name="teamResetButton" type="button" style="IconButton" sprite="iconResetGold" sprite_over="iconResetWhite" size="100%-21 0 100%-5 16">
|  97|  91| 						<translatableAttribute id="tooltip">Reset all teams to the default.</translatableAttribute>
|  98|  92| 						<action on="Press">resetTeams();</action>
|  99|  93| 					</object>
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
| 111| 111| 								<translatableAttribute id="tooltip">Select player.</translatableAttribute>
| 112| 112| 							</object>
| 113| 113| 							<object name="playerAssignmentText[n]" type="text" style="ModernLabelText" size="22%+5 0 50%+35 30"/>
| 114|    |-							<object name="playerConfig[n]" type="button" style="StoneButton" size="50%+40 4 50%+64 28"
| 115|    |-								tooltip_style="onscreenToolTip"
| 116|    |-								font="sans-bold-stroke-12"
| 117|    |-								sprite="ModernGear"
| 118|    |-								sprite_over="ModernGearHover"
| 119|    |-								sprite_pressed="ModernGearPressed"
| 120|    |-							>
|    | 114|+							<object name="playerConfig[n]" type="button" style="StoneButton" size="50%+40 4 50%+64 28" tooltip_style="onscreenToolTip" font="sans-bold-stroke-12" sprite="ModernGear" sprite_over="ModernGearHover" sprite_pressed="ModernGearPressed">
| 121| 115| 								<translatableAttribute id="tooltip">Configure AI settings.</translatableAttribute>
| 122| 116| 							</object>
| 123| 117| 							<object name="playerCiv[n]" type="dropdown" style="ModernDropDown" size="50%+69 2 85% 30" tooltip_style="onscreenToolTip" dropdown_size="350">
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
| 150| 150| 
| 151| 151| 				<repeat count="30" var="n">
| 152| 152| 					<object name="mapOptionFrame[n]" size="0 0 100% 30">
| 153|    |-						<object
| 154|    |-							name="mapOptionTitle[n]"
| 155|    |-							type="text"
| 156|    |-							size="0 0 140 28"
| 157|    |-							style="ModernRightLabelText"
| 158|    |-						/>
| 159|    |-						<object
| 160|    |-							name="mapOp

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

Harbormaster completed remote builds in B2997: Diff 3478.Sep 4 2017, 2:15 PM
elexis added inline comments.Sep 4 2017, 2:15 PM
binaries/data/mods/public/gui/gamesetup/gamesetup.js
371 ↗(On Diff #3478)

(Dunno bout your taste, but this looks tremendously better to understand and modify than before)

1014 ↗(On Diff #3478)

(Took too much time to understand what initGUIObjects was doing after not reading it for some months.
If g_OptionOrderInit were complete, there were only one loop.
Even better would probably be to add a initOrderPriority to g_Dropdowns and g_Checkboxes that defaults to 10000 and is only set for these three currently relevant items.
Same could be done for the GUI order. Then every setting would truly be contained in only one hunk (unless there is a dependency on another option).
My plan is to allow maps with custom gamesetup options (like the difficulty on survival) and have that one hunk in a separate file in the maps directory, refs #4676.
)

1048 ↗(On Diff #3478)

That comment is outdated as of https://github.com/elexis1/0ad/commit/32a6c482eef4d237941f0a2ce0ae5e2172ca424d and should be deleted (refs why I dont delete merged github branches)

1060 ↗(On Diff #3478)

This return value is outdated as well, isn't it? gamesetup.xml seems to be agnostic from gamesetup.js, so we can return undefined and pump out an error messag here, delete the guiType == "" checks otherwise and merge the first and second array item returned above?

(People often suggest to add non-simulation settings to that dialog, but I don't see why we would want to support gamesetup.xml not being agnostic of simulation settings)

1005 ↗(On Diff #3453)

(We should warn here instead, because mods are free to change the arrays from the outside, yet should keep both in sync, nay? (Would also make that code slightly more readable))

bb added inline comments.Sep 4 2017, 2:17 PM
binaries/data/mods/public/gui/gamesetup/gamesetup.js
1054 ↗(On Diff #3478)

vulcan: thx noticed

bb added inline comments.Sep 4 2017, 5:52 PM
binaries/data/mods/public/gui/gamesetup/gamesetup.js
371 ↗(On Diff #3478)

My taste is a pizza peperoni with extra cheese :P

1014 ↗(On Diff #3478)

The gui order change should be done seperately and with some more elebarate tinking, since then the "map" or "more" tag should be merged too. (Notice that the "more" page is only becoming bigger so we probably need scrolling or tabs there sometime)

1060 ↗(On Diff #3478)

No this is still used for the playerDropdowns

bb updated this revision to Diff 3479.Sep 4 2017, 5:53 PM

Use a initOrderPriority instead of an g_GUIOrderInit

Vulcan added a comment.Sep 4 2017, 6:41 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://jenkins-master:8080/job/phabricator/1963/ for more details.

Vulcan added a comment.Sep 4 2017, 6:42 PM
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1613| »   while·(g_IsNetworked)
|    | [NORMAL] ESLintBear (no-unmodified-loop-condition):
|    | 'g_IsNetworked' is not modified in this loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1595| »   if·(g_LoadingState·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1647| »   »   if·(playerData.some((pData,·j)·=>·i·!=·j·&&·sameColor(playerData[i].Color,·pData.Color)))
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1648| »   »   »   playerData[i].Color·=·g_PlayerColorPickerList.find(color·=>·playerData.every(pData·=>·!sameColor(color,·pData.Color)));
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1707| »   if·(guiType·!=·"")
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with ''.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1752| »   if·(guiType·!=·"")
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with ''.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1837| »   »   »   chosenCiv·=·pickRandom(Object.keys(g_CivData).filter(civ·=>·g_CivData[civ].Culture·==·culture));
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1851| »   »   let·usedName·=·g_GameAttributes.settings.PlayerData.filter(pData·=>·pData.Name·&&·pData.Name.indexOf(chosenName)·!==·-1).length;
|    | [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/gamesetup/gamesetup.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|   1|   1| <?xml version="1.0" encoding="utf-8"?>
|   2|    |-
|   3|   2| <objects>
|   4|   3| 
|   5|   4| 	<script file="gui/common/color.js"/>
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|  59|  59| 						<translatableAttribute id="caption">Civilization</translatableAttribute>
|  60|  60| 					</object>
|  61|  61| 
|  62|    |-					<object name="civInfoButton"
|  63|    |-						type="button"
|  64|    |-						style="IconButton"
|  65|    |-						sprite="iconInfoGold"
|  66|    |-						sprite_over="iconInfoWhite"
|  67|    |-						size="85%-37 0 85%-21 16"
|  68|    |-					>
|    |  62|+					<object name="civInfoButton" type="button" style="IconButton" sprite="iconInfoGold" sprite_over="iconInfoWhite" size="85%-37 0 85%-21 16">
|  69|  63| 						<translatableAttribute id="tooltip">View civilization info</translatableAttribute>
|  70|  64| 						<action on="Press"><![CDATA[
|  71|  65| 							Engine.PushGuiPage("page_civinfo.xml");
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|  72|  72| 						]]></action>
|  73|  73| 					</object>
|  74|  74| 
|  75|    |-					<object name="civResetButton"
|  76|    |-						type="button"
|  77|    |-						style="IconButton"
|  78|    |-						sprite="iconResetGold"
|  79|    |-						sprite_over="iconResetWhite"
|  80|    |-						size="85%-16 0 85% 16"
|  81|    |-					>
|    |  75|+					<object name="civResetButton" type="button" style="IconButton" sprite="iconResetGold" sprite_over="i

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

Harbormaster completed remote builds in B2999: Diff 3479.Sep 4 2017, 6:42 PM
elexis added a comment.Sep 5 2017, 2:32 PM
  • initOrderPriority -> initOrder
  • Comment could state that lower numbers come first.
  • Not a fan of cloning the entire object. It should be possible to only sort the object keys by priority and then iterate in that order.

(Still dubious about g_OptionOrderGUI not being mod friendly, offtopic)

Other than that looks good.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
1025 ↗(On Diff #3479)

else warn

1070 ↗(On Diff #3479)

Perhaps that return value would be more readable if each item is on a separate line

1752 ↗(On Diff #3479)

!guiType

1770 ↗(On Diff #3479)

singular, no?
GUI Object is the word used elsewhere in this file

1048 ↗(On Diff #3478)

for example or e.g. (since were trying to use more common english in the code)
Or just "Player assignments use custom names".

(In theory, should explain what it returns. I probably won't like comment since I couldnt come up with a neat one myself. This stuff is getting complex, but still better than adding duplicated code as some time ago. Probably easier for the reader to read the uses than to try to put it into words.)

bb updated this revision to Diff 3500.Sep 5 2017, 3:58 PM
bb marked 3 inline comments as done.

fixing the above

binaries/data/mods/public/gui/gamesetup/gamesetup.js
1752 ↗(On Diff #3479)

-> guiType

elexis added inline comments.Sep 5 2017, 4:21 PM
binaries/data/mods/public/gui/gamesetup/gamesetup.js
399 ↗(On Diff #3500)

US english everywhere
Items = Options
Second sentence contains the information of the first sentence

-> Options with lower values will be initialized first?

1829 ↗(On Diff #3500)

I'd add idxName here too for consistency, remove the default of idxName and pass emptystring explicitly.

Then I'd even make this a for (let guiType of ["checkbox", "dropdown"]) loop.

The function only exists to make the dropdown function agnostic of checkboxes?

Having in the checkbox function would work too without the need for the function. Probably the lesser evil

	if (guiType == "Checkbox")
		Engine.GetGUIObjectByName(guiName + "Dropdown" + guiIdx).hidden = true;
1048 ↗(On Diff #3478)

See comment above. Furthermore custum -> custom.

Just replace the second sentence with "Player settings use custom names"?

Vulcan added a comment.Sep 5 2017, 4:45 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://jenkins-master:8080/job/phabricator/1972/ for more details.

Vulcan added a comment.Sep 5 2017, 4:47 PM
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1668| »   while·(g_IsNetworked)
|    | [NORMAL] ESLintBear (no-unmodified-loop-condition):
|    | 'g_IsNetworked' is not modified in this loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1493| »   »   »   »   if·(g_Settings.Biomes.every(bio·=>·bio.Id·!=·biome))
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1494| »   »   »   »   »   warn("Map·'"·+·g_GameAttributes.map·+·"'·contains·unknown·biome·'"·+·biome·+·"'")
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1650| »   if·(g_LoadingState·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1702| »   »   if·(playerData.some((pData,·j)·=>·i·!=·j·&&·sameColor(playerData[i].Color,·pData.Color)))
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1703| »   »   »   playerData[i].Color·=·g_PlayerColorPickerList.find(color·=>·playerData.every(pData·=>·!sameColor(color,·pData.Color)));
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1896| »   »   »   chosenCiv·=·pickRandom(Object.keys(g_CivData).filter(civ·=>·g_CivData[civ].Culture·==·culture));
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1910| »   »   let·usedName·=·g_GameAttributes.settings.PlayerData.filter(pData·=>·pData.Name·&&·pData.Name.indexOf(chosenName)·!==·-1).length;
|    | [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/gamesetup/gamesetup.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|   1|   1| <?xml version="1.0" encoding="utf-8"?>
|   2|    |-
|   3|   2| <objects>
|   4|   3| 
|   5|   4| 	<script file="gui/common/color.js"/>
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|  59|  59| 						<translatableAttribute id="caption">Civilization</translatableAttribute>
|  60|  60| 					</object>
|  61|  61| 
|  62|    |-					<object name="civInfoButton"
|  63|    |-						type="button"
|  64|    |-						style="IconButton"
|  65|    |-						sprite="iconInfoGold"
|  66|    |-						sprite_over="iconInfoWhite"
|  67|    |-						size="85%-37 0 85%-21 16"
|  68|    |-					>
|    |  62|+					<object name="civInfoButton" type="button" style="IconButton" sprite="iconInfoGold" sprite_over="iconInfoWhite" size="85%-37 0 85%-21 16">
|  69|  63| 						<translatableAttribute id="tooltip">View civilization info</translatableAttribute>
|  70|  64| 						<action on="Press"><![CDATA[
|  71|  65| 							Engine.PushGuiPage("page_civinfo.xml");
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|  72|  72| 						]]></action>
|  73|  73| 					</object>
|  74|  74| 
|  75|    |-					<object name="civResetButton"
|  76|    |-						type="button"
|  77|    |-						style="IconButton"
|  78|    |-						sprite="iconResetGold"
|  79|    |-						sprite_over="iconResetWhite"
|  80|    |-						size="85%-16 0 85% 16"
|  81|    |-					>
|    |  75|+					<object name="civResetButton" type="button" style="IconButton" sprite="iconResetGold" sprite_over="iconResetWhite" size="85%-16 0 85% 16">
|  82|  76| 						<translatableAttribute id="tooltip">Reset any civilizations that have been selected to the default (random)</translatableAttribute>
|  83|  77| 						<action on="Press">resetCivilizations();</action>
|  84|  78| 					</object>
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|  87|  87| 						<translatableAttribute id="caption">Team</translatableAttribute>
|  88|  88| 					</object>
|  89|  89| 
|  90|    |-					<object name="teamResetButton"
|  91|    |-						type="button"
|  92|    |-						style="IconButton"
|  93|    |-						sprite="iconResetGold"
|  94|    |-						sprite_over="iconResetWhite"
|  95|    |-						size="100%-21 0 100%-5 16"
|  96|    |-					>
|    |  90|+					<object name="teamResetButton" type="button" style="IconButton" sprite="iconResetGold" sprite_over="iconResetWhite" size="100%-21 0 100%-5 16">
|  97|  91| 						<translatableAttribute id="tooltip">Reset all teams to the default.</translatableAttribute>
|  98|  92| 						<action on="Press">resetTeams();</action>
|  99|  93| 					</object>
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
| 111| 111| 								<translatableAttribute id="tooltip">Select player.</translatableAttribute>
| 112| 112| 							</object>
| 113| 113| 							<object name="playerAssignmentText[n]" type="text" style="ModernLabelText" size="22%+5 0 50%+35 30"/>
| 114|    |-							<object name="playerConfig[n]" type="button" style="StoneButton" size="50%+40 4 50%+64 28"
| 115|    |-								tooltip_style="onscreenToolTip"
| 116|    |-								font="sans-bold-stroke-12"
| 117|    |-								sprite="ModernGear"
| 118|    |-								sprite_over="ModernGearHover"
| 119|    |-								sprite_pressed="ModernGearPressed"
| 120|    |-							>
|    | 114|+							<object name="playerConfig[n]" type="button" style="StoneButton" size="50%+40 4 50%+64 28" tooltip_style="onscreenToolTip" font="sans-bold-stroke-12" sprite="ModernGear" sprite_over="ModernGearHover" sprite_pressed="ModernGearPressed">
| 121| 115| 								<translatableAttribute id="tooltip">Configure AI settings.</translatableAttribute>
| 122| 116| 							</object>
| 123| 117| 							<object name="playerCiv[n]" type="dropdown" style="ModernDropDown" size="50%+69 2 85% 30" tooltip_style="onscreenToolTip" dropdown_size="350">
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
| 150| 150| 
| 151| 151| 				<repeat count="30" var="n">
| 152| 152| 					<object name="mapOptionFrame[n]" size="0 0 100% 30">
| 153|    |-						<object
| 154|    |-							name="mapOptionTitle[n]"
| 155|    |-							type="text"
| 156|    |-							size="0 0 140 28"
| 157|    |-							style="ModernRightLabelText"
| 158|    |-						/>
| 159|    |-						<object
| 160|    |-							name="mapOptionText[n]"
| 161|    |-							type="text"
| 162|    |-							size="150 0 100% 28"
| 163|    |-							style="ModernLeftLabelText"
| 164|    |-						/>
| 165|    |-						<object
| 166|    |-							name="mapOptionDropdown[n]"
| 167|    |-							type="dropdown"
| 168|    |-							size="150 0 100% 28"
| 169|    |-							style="ModernDropDown"
| 170|    |-							tooltip_style="onscreenToolTip"
| 171|    |-						/>
| 172|    |-						<object
| 173|    |-							name="mapOptionCheckbox[n]"
| 174|    |-							type="checkbox"
| 175|    |-							size="150 0 180 28"
| 176|    |-							style="ModernTickBox"
| 177|    |-							tooltip_style="onscreenToolTip"
| 178|    |-						/>
|    | 153|+						<object name="mapOptionTitle[n]" type="text" size="0 0 140 28" style="ModernRightLabelText"/>
|    | 154|+						<object name="mapOptionText[n]" type="text" size="150 0 100% 28" style="ModernLeftLabelText"/>
|    | 155|+						<object name="mapOptionDropdown[n]" type="dropdown" size="150 0 100% 28" style="ModernDropDown" tooltip_style="onscreenToolTip"/>
|    | 156|+						<object name="mapOptionCheckbox[n]" type="checkbox" size="150 0 180 28" style="ModernTickBox" tooltip_style="onscreenToolTip"/>
| 179| 157| 					</object>
| 180| 158| 				</repeat>
| 181| 159| 			</object>
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
| 211| 211| 			</object>
| 212| 212| 
| 213| 213| 			<!-- Tooltip -->
| 214|    |-			<object name="onscreenToolTip"
| 215|    |-				type="text"
| 216|    |-				font="sans-14"
| 217|    |-				textcolor="white"
| 218|    |-				sprite="BackgroundTranslucent"
| 219|    |-				size="20 100%-56 100%-312 100%-24"
| 220|    |-			/>
|    | 214|+			<object name="onscreenToolTip" type="text" font="sans-14" textcolor="white" sprite="BackgroundTranslucent" size="20 100%-56 100%-312 100%-24"/>
| 221| 215| 
| 222| 216| 			<!-- Cheat Warning Text -->
| 223| 217| 			<object size="0 100%-52 100%-320 100%-24" name="cheatWarningText">
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
| 227| 227| 			</object>
| 228| 228| 
| 229| 229| 			<!-- Start/Ready Button -->
| 230|    |-			<object
| 231|    |-				name="startGame"
| 232|    |-				type="button"
| 233|    |-				style="StoneButton"
| 234|    |-				size="100%-164 100%-52 100%-24 100%-24"
| 235|    |-				tooltip_style="onscreenToolTip"
| 236|    |-			>
|    | 230|+			<object name="startGame" type="button" style="StoneButton" size="100%-164 100%-52 100%-24 100%-24" tooltip_style="onscreenToolTip">
| 237| 231| 				<action on="Press">
| 238| 232| 					if (g_IsController)
| 239| 233| 						launchGame();
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
| 243| 243| 			</object>
| 244| 244| 
| 245| 245| 			<!-- Cancel Button -->
| 246|    |-			<object
| 247|    |-				name="cancelGame"
| 248|    |-				type="button"
| 249|    |-				style="StoneButton"
| 250|    |-				size="100%-308 100%-52 100%-168 100%-24"
| 251|    |-				tooltip_style="onscreenToolTip"
| 252|    |-			>
|    | 246|+			<object name="cancelGame" type="button" style="StoneButton" size="100%-308 100%-52 100%-168 100%-24" tooltip_style="onscreenToolTip">
| 253| 247| 				<translatableAttribute id="caption">Back</translatableAttribute>
| 254| 248| 				<action on="Press">cancelSetup();</action>
| 255| 249| 			</object>
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
| 257| 257| 			<!-- Options -->
| 258| 258| 			<object name="gameOptionsBox" size="100%-425 529 100%-25 525">
| 259| 259| 				<!-- More Options Button -->
| 260|    |-				<object
| 261|    |-					name="showMoreOptions"
| 262|    |-					type="button"
| 263|    |-					style="StoneButton"
| 264|    |-					size="100%-120 0 100% 28"
| 265|    |-					tooltip_style="onscreenToolTip"
| 266|    |-				>
|    | 260|+				<object name="showMoreOptions" type="button" style="StoneButton" size="100%-120 0 100% 28" tooltip_style="onscreenToolTip">
| 267| 261| 					<translatableAttribute id="caption">More Options</translatableAttribute>
| 268| 262| 					<translatableAttribute id="tooltip">See more game options</translatableAttribute>
| 269| 263| 					<action on="Press">showMoreOptions(true);</action>
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
| 281| 281| 
| 282| 282| 				<repeat count="30" var="n">
| 283| 283| 					<object name="moreOptionFrame[n]" size="14 30 94% 60">
| 284|    |-						<object
| 285|    |-							name="moreOptionTitle[n]"
| 286|    |-							type="text"
| 287|    |-							size="0 0 40% 28"
| 288|    |-							style="ModernRightLabelText"
| 289|    |-						/>
| 290|    |-						<object
| 291|    |-							name="moreOptionText[n]"
| 292|    |-							type="text"
| 293|    |-							size="40% 0 100% 28"
| 294|    |-							style="ModernLeftLabelText"
| 295|    |-						/>
| 296|    |-						<object
| 297|    |-							name="moreOptionDropdown[n]"
| 298|    |-							type="dropdown"
| 299|    |-							size="40%+10 0 100% 28"
| 300|    |-							style="ModernDropDown"
| 301|    |-							tooltip_style="onscreenToolTip"
| 302|    |-						/>
| 303|    |-						<object
| 304|    |-							name="moreOptionCheckbox[n]"
| 305|    |-							type="checkbox"
| 306|    |-							size="40%+10 5 40%+28 28"
| 307|    |-							style="ModernTickBox"
| 308|    |-							tooltip_style="onscreenToolTip"
| 309|    |-						/>
|    | 284|+						<object name="moreOptionTitle[n]" type="text" size="0 0 40% 28" style="ModernRightLabelText"/>
|    | 285|+						<object name="moreOptionText[n]" type="text" size="40% 0 100% 28" style="ModernLeftLabelText"/>
|    | 286|+						<object name="moreOptionDropdown[n]" type="dropdown" size="40%+10 0 100% 28" style="ModernDropDown" tooltip_style="onscreenToolTip"/>
|    | 287|+						<object name="moreOptionCheckbox[n]" type="checkbox" size="40%+10 5 40%+28 28" style="ModernTickBox" tooltip_style="onscreenToolTip"/>
| 310| 288| 					</object>
| 311| 289| 				</repeat>
| 312| 290| 
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.xml
| 311| 311| 				</repeat>
| 312| 312| 
| 313| 313| 				<!-- Hide More Options Button -->
| 314|    |-				<object
| 315|    |-					name="hideMoreOptions"
| 316|    |-					type="button"
| 317|    |-					style="StoneButton"
| 318|    |-					size="50%-70 428 50%+70 456"
| 319|    |-					tooltip_style="onscreenToolTip"
| 320|    |-					hotkey="cancel"
| 321|    |-				>
|    | 314|+				<object name="hideMoreOptions" type="button" style="StoneButton" size="50%-70 428 50%+70 456" tooltip_style="onscreenToolTip" hotkey="cancel">
| 322| 315| 					<translatableAttribute id="caption">OK</translatableAttribute>
| 323| 316| 					<translatableAttribute id="tooltip">Close more game options window</translatableAttribute>
| 324| 317| 					<action on="Press">showMoreOptions(false);</action>
Executing section Python...
Executing section Perl...

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

Harbormaster completed remote builds in B3017: Diff 3500.Sep 5 2017, 4:47 PM
bb updated this revision to Diff 3505.Sep 5 2017, 5:02 PM

nuke the function and return to an earlier patch and fix some comments

Closed by commit rP20116: Allow arbitrary order between dropdowns and checkboxes in gamesetup (authored by bb). · Explain WhySep 5 2017, 5:58 PM
This revision was automatically updated to reflect the committed changes.
elexis added a comment.Sep 5 2017, 6:03 PM

Thanks (I had also tested the code and agreed on irc)

Vulcan added a comment.Sep 5 2017, 6:23 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://jenkins-master:8080/job/phabricator/1974/ for more details.

Vulcan added a comment.Sep 5 2017, 6:25 PM
Executing section Default...
Executing section Source...
Executing section JS...
Executing section XML GUI...
Executing section Python...
Executing section Perl...

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

Harbormaster completed remote builds in B3020: Diff 3505.Sep 5 2017, 6:25 PM
elexis mentioned this in D429: Gamesetup unification variable renaming.Sep 7 2017, 1:03 AM

Revision Contents
Changeset List

PathSize
ps/
trunk/
binaries/
data/
mods/
public/
gui/
gamesetup/
155 lines
50 lines

Diff 3506

View Options

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

Loading...
View Options

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

Loading...
Wildfire Games · Phabricator