Hide mapfilter option if there are no maps matching the filter
Details
- Reviewers
elexis rob-v - Commits
- rP19773: Gamesetup - Hide mapfilter option if there are no maps matching the filter
- Trac Tickets
- #4423
select e.g. Scenario, New maps
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- https://svn.wildfiregames.com/public/ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 2210 Build 3632: Vulcan Build (Windows) Jenkins Build 3631: Vulcan Build Jenkins Build 3630: arc lint + arc unit
Event Timeline
binaries/data/mods/public/gui/credits/texts/programming.json | ||
---|---|---|
167 ↗ | (On Diff #1377) | The nicks should be sorted alphabetically |
binaries/data/mods/public/gui/credits/texts/programming.json | ||
---|---|---|
167 ↗ | (On Diff #1377) | Ok. I removed it now. This diff is minor change. I can add it lately. |
Thanks for working on the release blocker ticket!
There are some concerns with this patch though:
- The approach of the patch is not what I had in mind, see the title of that ticket. The mapfilter choice itself should become hidden if there are no maps matching that filter initMapNameList can be extended to test for that after selecting the maptype. Perhaps we can get D322 in, perhaps not. Might want to wait a bit. The mapfilter bug has to be fixed before the release and can be done after feature freeze, the option unification probably can't be added after feature freze. So might want to wait for feature freeze before starting to rewrite this patch.
- The programming.json entry should always be part of the first patch.
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/843/ for more details.
Ping, D322 is complete.
When selecting a maptype, it should call initDropdown("mapFilter").
g_MapFilterList could be renamed to g_MapFilters and then we can have a new variable g_MapFilterList which is generated in a new function reloadMapFilters above reloadMapList which just sets g_MapFilterList = g_MapFilters.filter(mapFilter => { reloadMapList(mapFilter); return g_MapSelectionList.length ) (or close to that)
This is not a performance issue since we load each map XML / JSON when selecting a maptype already and since these things are cached both in the VFS (engine) and in g_MapData of gamesetup.js.
The mapFilter entry of g_Dropdowns should have
@rob-v are you still intending to work on this? Feature freeze is this sunday, so we will have to approach this in the next 1-3 weeks +/-
Build has FAILED
Link to build: http://jw:8080/job/phabricator/1504/
See console output for more information: http://jw:8080/job/phabricator/1504/console
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 | 313| 313| /** | 314| 314| * Index of the GUI timer. | 315| 315| */ | 316| |-var g_GameStanzaTimer = undefined; | | 316|+var g_GameStanzaTimer; | 317| 317| | 318| 318| /** | 319| 319| * Only send a lobby update if something actually changed. | | [NORMAL] ESLintBear (no-multi-spaces): | | Multiple spaces found before 'name'. |----| | /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 | 899| 899| name = | 900| 900| '[color="' + | 901| 901| g_ReadyData[assignedGUID ? g_PlayerAssignments[assignedGUID].status : 2].color + | 902| |- '"]' + name + '[/color]'; | | 902|+ '"]' + name + '[/color]'; | 903| 903| | 904| 904| return name; | 905| 905| }, | | [NORMAL] ESLintBear (no-multi-spaces): | | Multiple spaces found before 'attribs'. |----| | /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 | 937| 937| | 938| 938| g_IsNetworked = attribs.type != "offline"; | 939| 939| g_IsController = attribs.type != "client"; | 940| |- g_IsTutorial = attribs.tutorial && attribs.tutorial == true; | | 940|+ g_IsTutorial = attribs.tutorial && attribs.tutorial == true; | 941| 941| g_ServerName = attribs.serverName; | 942| 942| g_ServerPort = attribs.serverPort; | 943| 943| g_StunEndpoint = attribs.stunEndpoint; | | [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 |1138|1138| |1139|1139| function verticallyDistributeGUIObjects(parent, objectHeight, ignore) |1140|1140| { |1141| |- let yPos = undefined; | |1141|+ let yPos; |1142|1142| |1143|1143| let parentObject = Engine.GetGUIObjectByName(parent); |1144|1144| for (let child of parentObject.children) binaries/data/mods/public/gui/gamesetup/gamesetup.js | 689| » » » 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 |1573| » 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 |2023| » » 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 |2023| » » 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 | 316| var·g_GameStanzaTimer·=·undefined; | | [NORMAL] JSHintBear: | | It's not necessary to initialize 'g_GameStanzaTimer' to 'undefined'. binaries/data/mods/public/gui/gamesetup/gamesetup.js | 940| » g_IsTutorial·=·attribs.tutorial·&&··attribs.tutorial·==·true; | | [NORMAL] JSHintBear: | | Use '===' to compare with 'true'. binaries/data/mods/public/gui/gamesetup/gamesetup.js |1141| » let·yPos·=·undefined; | | [NORMAL] JSHintBear: | | It's not necessary to initialize 'yPos' to 'undefined'. binaries/data/mods/public/gui/gamesetup/gamesetup.js |1344| » if·(map·==·"random"·||·map·==·"") | | [NORMAL] JSHintBear: | | Use '===' to compare with ''. binaries/data/mods/public/gui/gamesetup/gamesetup.js |1386| » » let·filterID·=·g_MapFilterList.id.findIndex(id·=>·id·==·g_GameAttributes.mapFilter); | | [NORMAL] JSHintBear: | | Don't make functions within a loop. binaries/data/mods/public/gui/gamesetup/gamesetup.js |1555| » if·(g_LoadingState·==·0) | | [NORMAL] JSHintBear: | | Use '===' to compare with '0'. binaries/data/mods/public/gui/gamesetup/gamesetup.js |1607| » » 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 |1608| » » » 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 |1785| » » » 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 |1799| » » 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 |1933| » Engine.GetGUIObjectByName("mapInfoDescription").caption·=·g_GameAttributes.map·==·""·?·""·:·getGameDescription(); | | [NORMAL] JSHintBear: | | Use '===' to compare with ''. binaries/data/mods/public/gui/gamesetup/gamesetup.js |2235| » if·(g_GameStanzaTimer·!=·undefined) | | [NORMAL] JSHintBear: | | Use '!==' to compare with 'undefined'. binaries/data/mods/public/gui/gamesetup/gamesetup.js |2275| » 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/151/ for more details.
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/1505/ for more details.
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 | 315| 315| /** | 316| 316| * Index of the GUI timer. | 317| 317| */ | 318| |-var g_GameStanzaTimer = undefined; | | 318|+var g_GameStanzaTimer; | 319| 319| | 320| 320| /** | 321| 321| * Only send a lobby update if something actually changed. | | [NORMAL] ESLintBear (no-multi-spaces): | | Multiple spaces found before 'name'. |----| | /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 | 901| 901| name = | 902| 902| '[color="' + | 903| 903| g_ReadyData[assignedGUID ? g_PlayerAssignments[assignedGUID].status : 2].color + | 904| |- '"]' + name + '[/color]'; | | 904|+ '"]' + name + '[/color]'; | 905| 905| | 906| 906| return name; | 907| 907| }, | | [NORMAL] ESLintBear (no-multi-spaces): | | Multiple spaces found before 'attribs'. |----| | /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 | 939| 939| | 940| 940| g_IsNetworked = attribs.type != "offline"; | 941| 941| g_IsController = attribs.type != "client"; | 942| |- g_IsTutorial = attribs.tutorial && attribs.tutorial == true; | | 942|+ g_IsTutorial = attribs.tutorial && attribs.tutorial == true; | 943| 943| g_ServerName = attribs.serverName; | 944| 944| g_ServerPort = attribs.serverPort; | 945| 945| g_StunEndpoint = attribs.stunEndpoint; | | [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 |1140|1140| |1141|1141| function verticallyDistributeGUIObjects(parent, objectHeight, ignore) |1142|1142| { |1143| |- let yPos = undefined; | |1143|+ let yPos; |1144|1144| |1145|1145| let parentObject = Engine.GetGUIObjectByName(parent); |1146|1146| for (let child of parentObject.children) binaries/data/mods/public/gui/gamesetup/gamesetup.js | 691| » » » 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 |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 |2046| » » 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 |2046| » » 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 | 318| var·g_GameStanzaTimer·=·undefined; | | [NORMAL] JSHintBear: | | It's not necessary to initialize 'g_GameStanzaTimer' to 'undefined'. binaries/data/mods/public/gui/gamesetup/gamesetup.js | 942| » g_IsTutorial·=·attribs.tutorial·&&··attribs.tutorial·==·true; | | [NORMAL] JSHintBear: | | Use '===' to compare with 'true'. binaries/data/mods/public/gui/gamesetup/gamesetup.js |1143| » let·yPos·=·undefined; | | [NORMAL] JSHintBear: | | It's not necessary to initialize 'yPos' to 'undefined'. 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. binaries/data/mods/public/gui/gamesetup/gamesetup.js |2258| » if·(g_GameStanzaTimer·!=·undefined) | | [NORMAL] JSHintBear: | | Use '!==' to compare with 'undefined'. binaries/data/mods/public/gui/gamesetup/gamesetup.js |2298| » 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/152/ for more details.
Works marvelous, just requesting some JSdoc and variable renames.
binaries/data/mods/public/gui/gamesetup/gamesetup.js | ||
---|---|---|
200 | wraitii proposed to name all variables that hold dropdown values as g_FooList, so g_MapFilters and g_MapFilterList? A JSdoc comment could explain that this only contains mapfilters for which we have at least one map matching this. | |
1369–1372 | JSdoc for filter | |
1374 | This function is correct, in particular becaues it excludes the "Random" map choice. | |
1406–1426 | could merge that with the line above, or remove the mapList = as sort sorts in place too | |
1413 | entry -> mapFilter? This is doing too much work, since need to know only about one map. But fixing this is not worth the code IMO as the maps are already cached by the OS (#4072) and being re-read just few calls later. | |
1424 | There's always one defined, comment not really needed | |
1885–1886 | Someone didn't update svn before uploading the patch. The reloadMapList(); call that was inserted (restored) here doesn't need to be replaced with the reloadMapFilterList(), because the clients can't change the mapfilter setting and only see what the host set. As mentioned somewhere, the rebuilding of dropdown values should be made generic too later. |
There is a slight bug in my patch: select random and new maps. Then set map type to scenario. The Map filter dropdown will be empty.
binaries/data/mods/public/gui/gamesetup/gamesetup.js | ||
---|---|---|
1885–1886 |
I think it would be safer and saner to replace it, because in future clients may change the settings(D431). |
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/1507/ for more details.
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-multi-spaces): | | Multiple spaces found before 'name'. |----| | /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 | 904| 904| name = | 905| 905| '[color="' + | 906| 906| g_ReadyData[assignedGUID ? g_PlayerAssignments[assignedGUID].status : 2].color + | 907| |- '"]' + name + '[/color]'; | | 907|+ '"]' + name + '[/color]'; | 908| 908| | 909| 909| return name; | 910| 910| }, | | [NORMAL] ESLintBear (no-multi-spaces): | | Multiple spaces found before 'attribs'. |----| | /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 | 942| 942| | 943| 943| g_IsNetworked = attribs.type != "offline"; | 944| 944| g_IsController = attribs.type != "client"; | 945| |- g_IsTutorial = attribs.tutorial && attribs.tutorial == true; | | 945|+ g_IsTutorial = attribs.tutorial && attribs.tutorial == true; | 946| 946| g_ServerName = attribs.serverName; | 947| 947| g_ServerPort = attribs.serverPort; | 948| 948| g_StunEndpoint = attribs.stunEndpoint; | | [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 |1143|1143| |1144|1144| function verticallyDistributeGUIObjects(parent, objectHeight, ignore) |1145|1145| { |1146| |- let yPos = undefined; | |1146|+ let yPos; |1147|1147| |1148|1148| let parentObject = Engine.GetGUIObjectByName(parent); |1149|1149| for (let child of parentObject.children) binaries/data/mods/public/gui/gamesetup/gamesetup.js | 694| » » » 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 |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 |2049| » » 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 |2049| » » 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 | 945| » g_IsTutorial·=·attribs.tutorial·&&··attribs.tutorial·==·true; | | [NORMAL] JSHintBear: | | Use '===' to compare with 'true'. binaries/data/mods/public/gui/gamesetup/gamesetup.js |1146| » let·yPos·=·undefined; | | [NORMAL] JSHintBear: | | It's not necessary to initialize 'yPos' to 'undefined'. 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 |1811| » » » 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 |1825| » » 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 |2261| » if·(g_GameStanzaTimer·!=·undefined) | | [NORMAL] JSHintBear: | | Use '!==' to compare with 'undefined'. binaries/data/mods/public/gui/gamesetup/gamesetup.js |2301| » 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/153/ for more details.
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/1508/ for more details.
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-multi-spaces): | | Multiple spaces found before 'name'. |----| | /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 | 904| 904| name = | 905| 905| '[color="' + | 906| 906| g_ReadyData[assignedGUID ? g_PlayerAssignments[assignedGUID].status : 2].color + | 907| |- '"]' + name + '[/color]'; | | 907|+ '"]' + name + '[/color]'; | 908| 908| | 909| 909| return name; | 910| 910| }, | | [NORMAL] ESLintBear (no-multi-spaces): | | Multiple spaces found before 'attribs'. |----| | /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 | 942| 942| | 943| 943| g_IsNetworked = attribs.type != "offline"; | 944| 944| g_IsController = attribs.type != "client"; | 945| |- g_IsTutorial = attribs.tutorial && attribs.tutorial == true; | | 945|+ g_IsTutorial = attribs.tutorial && attribs.tutorial == true; | 946| 946| g_ServerName = attribs.serverName; | 947| 947| g_ServerPort = attribs.serverPort; | 948| 948| g_StunEndpoint = attribs.stunEndpoint; | | [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 |1143|1143| |1144|1144| function verticallyDistributeGUIObjects(parent, objectHeight, ignore) |1145|1145| { |1146| |- let yPos = undefined; | |1146|+ let yPos; |1147|1147| |1148|1148| let parentObject = Engine.GetGUIObjectByName(parent); |1149|1149| for (let child of parentObject.children) binaries/data/mods/public/gui/gamesetup/gamesetup.js | 694| » » » 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 |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 |2049| » » 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 |2049| » » 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 | 945| » g_IsTutorial·=·attribs.tutorial·&&··attribs.tutorial·==·true; | | [NORMAL] JSHintBear: | | Use '===' to compare with 'true'. binaries/data/mods/public/gui/gamesetup/gamesetup.js |1146| » let·yPos·=·undefined; | | [NORMAL] JSHintBear: | | It's not necessary to initialize 'yPos' to 'undefined'. 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 |1811| » » » 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 |1825| » » 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 |2261| » if·(g_GameStanzaTimer·!=·undefined) | | [NORMAL] JSHintBear: | | Use '!==' to compare with 'undefined'. binaries/data/mods/public/gui/gamesetup/gamesetup.js |2301| » 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/154/ for more details.
Didn't check the bug yet you found
binaries/data/mods/public/gui/gamesetup/gamesetup.js | ||
---|---|---|
443 | This implies that the default map filter contains maps. Could keep track of the working map filters, use the default if that passes and use any other working mapfilter otherwise, but I'm not sure if we care about that. | |
1372 | @return could also mention the type (or both possibilities if we go with the second argument, x|y iirc) | |
1413 | and loadMapData also caches the map data, so the OS cache is irrelevant even. | |
1413 | Actually we only have to add a new argument to getFilteredMaps, use a ternaries in the existing return statements of getFilteredMaps and add an early return to the loop to avoid the useless work. It would also inform the reader that we don't need the results in some cases of calling this function. getFilteredMaps should be named so that it works with boolean returns too, filterMaps? | |
1413 | Actually mapFilter should be filterFunc for global consistency (since it isn't an element of g_MapFilters`), right? | |
1885–1886 | That sounds true. |
binaries/data/mods/public/gui/gamesetup/gamesetup.js | ||
---|---|---|
443 |
Nope, if no default is given, prepareForDropdown sets the default to 0. (https://github.com/0ad/0ad/blob/ec75f37daf825880e19c9095bf06282a98c0ce1d/binaries/data/mods/public/gui/common/settings.js#L291) | |
1413 | I don't think that is worth it. In the gamesetup code simple code is more important than performance. | |
1413 | No, as this is a whole filter and not just the filter func. |
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/1537/ for more details.
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 |1143|1143| |1144|1144| function verticallyDistributeGUIObjects(parent, objectHeight, ignore) |1145|1145| { |1146| |- let yPos = undefined; | |1146|+ let yPos; |1147|1147| |1148|1148| let parentObject = Engine.GetGUIObjectByName(parent); |1149|1149| for (let child of parentObject.children) binaries/data/mods/public/gui/gamesetup/gamesetup.js | 694| » » » 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 |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 |2049| » » 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 |2049| » » 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 | 945| » g_IsTutorial·=·attribs.tutorial·&&·attribs.tutorial·==·true; | | [NORMAL] JSHintBear: | | Use '===' to compare with 'true'. binaries/data/mods/public/gui/gamesetup/gamesetup.js |1146| » let·yPos·=·undefined; | | [NORMAL] JSHintBear: | | It's not necessary to initialize 'yPos' to 'undefined'. 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 |1811| » » » 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 |1825| » » 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 |2261| » if·(g_GameStanzaTimer·!=·undefined) | | [NORMAL] JSHintBear: | | Use '!==' to compare with 'undefined'. binaries/data/mods/public/gui/gamesetup/gamesetup.js |2301| » 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/177/ for more details.
Just wanted to state that I didn't review that part yet.
Tested now and it's been exactly the same I expected in the inline comment and fixed.
binaries/data/mods/public/gui/gamesetup/gamesetup.js | ||
---|---|---|
443 | Indeed, if the default "default" mapfilter doesn't contain any valid maps, it's going to be reset to the first mapfilter. | |
444 | Can be shortened to g_MapFilterList.id.indexOf(g_GameAttributes.mapFilter || "") != -1 | |
1413 | It loops over g_MapFilters.filter which is an array of filter functions right? Not fond of that boolean argument? At least remove the unneeded > 0 |
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/1545/ for more details.
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 |1143|1143| |1144|1144| function verticallyDistributeGUIObjects(parent, objectHeight, ignore) |1145|1145| { |1146| |- let yPos = undefined; | |1146|+ let yPos; |1147|1147| |1148|1148| let parentObject = Engine.GetGUIObjectByName(parent); |1149|1149| for (let child of parentObject.children) binaries/data/mods/public/gui/gamesetup/gamesetup.js | 694| » » » 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 |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 |2049| » » 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 |2049| » » 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 | 945| » g_IsTutorial·=·attribs.tutorial·&&·attribs.tutorial·==·true; | | [NORMAL] JSHintBear: | | Use '===' to compare with 'true'. binaries/data/mods/public/gui/gamesetup/gamesetup.js |1146| » let·yPos·=·undefined; | | [NORMAL] JSHintBear: | | It's not necessary to initialize 'yPos' to 'undefined'. 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 |1811| » » » 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 |1825| » » 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 |2261| » if·(g_GameStanzaTimer·!=·undefined) | | [NORMAL] JSHintBear: | | Use '!==' to compare with 'undefined'. binaries/data/mods/public/gui/gamesetup/gamesetup.js |2301| » 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/187/ for more details.