Page MenuHomeWildfire Games

Fix player color dropdown in gamesetup
ClosedPublic

Authored by Imarok on Jul 1 2017, 6:54 PM.

Details

Summary

The player color dropdown is reset on every change. Because of that scrolling though the colors doesn't work anymore.

Test Plan

Try scrolling through the color dropdown.

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

Imarok created this revision.Jul 1 2017, 6:54 PM
Vulcan added a subscriber: Vulcan.Jul 1 2017, 8:31 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/1678/ for more details.

Vulcan added a comment.Jul 1 2017, 8:32 PM
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'g_GameStanzaTimer' to undefined.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
| 318| 318| /**
| 319| 319|  * Index of the GUI timer.
| 320| 320|  */
| 321|    |-var g_GameStanzaTimer = undefined;
|    | 321|+var g_GameStanzaTimer;
| 322| 322| 
| 323| 323| /**
| 324| 324|  * Only send a lobby update if something actually changed.
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'yPos' to undefined.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1144|1144| 
|1145|1145| function verticallyDistributeGUIObjects(parent, objectHeight, ignore)
|1146|1146| {
|1147|    |-	let yPos = undefined;
|    |1147|+	let yPos;
|1148|1148| 
|1149|1149| 	let parentObject = Engine.GetGUIObjectByName(parent);
|1150|1150| 	for (let child of parentObject.children)

binaries/data/mods/public/gui/gamesetup/gamesetup.js
| 695| »   »   »   let·pData·=·playerData.find(pData·=>·sameColor(g_PlayerColorPickerList[selectedIdx],·pData.Color));
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'pData' is already declared in the upper scope.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1600| »   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
|2050| »   »   for·(let·guid·in·g_PlayerAssignments)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'guid' is already declared in the upper scope.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|2050| »   »   for·(let·guid·in·g_PlayerAssignments)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'guid' is already declared in the upper scope.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
| 321| var·g_GameStanzaTimer·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'g_GameStanzaTimer' to 'undefined'.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
| 946| »   g_IsTutorial·=·attribs.tutorial·&&·attribs.tutorial·==·true;
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with 'true'.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1147| »   let·yPos·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'yPos' to 'undefined'.

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

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1634| »   »   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
|1635| »   »   »   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
|1812| »   »   »   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
|1826| »   »   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.

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

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|2302| »   if·(g_GameStanzaTimer·!=·undefined)
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with 'undefined'.
Executing section XML GUI...
Executing section Python...
Executing section Perl...

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

elexis edited edge metadata.Jul 1 2017, 9:58 PM

Currently one can change the color to blue if the color isn't already blue by scrolling downwards.
If we click on the dropdown, we can see that the selection rectangle is always absent on that particular dropdown, because

	let selected = indexHidden ? -1 : dropdown.list_data.indexOf(String(obj.get(idx)));

always returns -1 (so we can only scroll to 0).

Notice every setting in g_Dropdowns and g_PlayerDropdowns returns the actual value in get and not the index, so this would be the first one where we return the list index.
However playerColorPicker.ids is also the only one that returns the indices instead of the values.
This is because the C++ dropdown can only hold a string or number (see CGUIList.h being used by CList.cpp inherited by CDropdown.cpp and getting Script value conversion check failed: v.isString() || v.isNumber() (got type object) if we tried to pass objects).
So if that wasn't the case, I would have proposed to save the actual colors in ids to make it consistent.

Could add a comment to playerColorPicker clarify that, but maybe not needed.
What we should add though is a comment for get stating that it must return something that is in ids.

Other than that the patched code is valid, because the color is still saved as before in g_GameAttributes and
it is complete (which is can be quickly seen by testing every dropdown for the ability to keep the selection rectangle at the actually selected value).

elexis accepted this revision.Jul 1 2017, 11:04 PM

Well the comments should be clear enough already actually:

* ids     - Array of identifier strings that indicate the selected value.
* get     - The identifier of the currently selected value.
This revision is now accepted and ready to land.Jul 1 2017, 11:04 PM
This revision was automatically updated to reflect the committed changes.