rP21679 doesn't work with the released version, since it only looks for png files using VFS FileExists. But textures are converted to .cached.dds for zipped mods.
Everywhere else it is not checked if the texture exists but presumed.
So either we keep the mapname_biomename.png pattern and add TextureExists, or we add lots of hardcodings to every map (that are prone to typos and a bit annoying to test).
Details
Make sure this works with both zipped and non-zipped mods.
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 6256 Build 10384: Vulcan Build Jenkins Build 10383: arc lint + arc unit
Event Timeline
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/656/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/657/display/redirect
I don't see any code specific for textures. You only need to check a file existence. If you will need to check a sound file, would you add the same function for the SoundManager?
source/graphics/TextureManager.cpp | ||
---|---|---|
319 | All getters that don't change a state of the object should be const. |
If testing only for the existance of one file was sufficient, then the previous code would have been correct. This patch adds the testing of the alternative filename storing the same content.
If you will need to check a sound file, would you add the same function for the SoundManager
There are other types of cached files that are converted. XML to XMB, some collada files. So the answer is yes, then we need more JSInterface functions too querying the according managers that contain the logic to determine the conversion and the alternative filename.
Edit: Specifically for the SoundManager no, because there is no conversion currently, but there could be one eventually (for instance flac format in svn and ogg format in the release), then we would need a JS Interface function if we wanted to test for soundfile existance in JS.
I understood, it's not a good architecture. Because ExistsFile != TextureExists for the same argument, especially in case it's the file path. I'd prefer to call it TextureCacheExists to make it less misleading and less duplicated. Because TextureExists doesn't guarantee that the file exists and can be accessed through g_VFS by the direct name.
The only architecture alternative I can imagine is moving all converted filenames to the VFS file, but that seems worse to me, as the fileending logic should be contained in the file handling the fileending. So what better architecture could be there?
TextureExists doesn't guarantee that the file exists and can be accessed through g_VFS by the direct name.
It doesn't claim to either.
It doesn't claim to either.
It claims by its name.
The only architecture alternative I can imagine is moving all converted filenames to the VFS file, but that seems worse to me, as the fileending logic should be contained in the file handling the fileending. So what better architecture could be there?
I didn't think much about it. But I'd call it TextureFileOrCacheExists.
"TextureExists(path)", I don't see the word VFS there, nor is it in the VFS Interface.
Consider all the other code that uses "foo.png" and yet doesn't load "foo.png" but "foo.png.cached.dds" in the VFS.
The only architecture alternative I can imagine is moving all converted filenames to the VFS file, but that seems worse to me, as the fileending logic should be contained in the file handling the fileending. So what better architecture could be there?
I didn't think much about it. But I'd call it TextureFileOrCacheExists.
Changing the name doesn't change the architecture. Don't give me bad press :P
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Default... Executing section Source... Executing section JS... | | [NORMAL] ESLintBear (curly): | | Unnecessary { after 'if' condition. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/gamedescription.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/gamedescription.js | 111| 111| | 112| 112| let playerDescription; | 113| 113| if (isAI) | 114| |- { | | 114|+ | 115| 115| if (playerData.Civ) | 116| 116| { | 117| 117| if (isActive) | 130| 130| // Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu | 131| 131| playerDescription = translate("%(playerName)s (%(AIdescription)s, %(state)s)"); | 132| 132| } | 133| |- } | | 133|+ | 134| 134| else | 135| 135| { | 136| 136| if (playerData.Offline) | | [NORMAL] ESLintBear (curly): | | Unnecessary { after 'if' condition. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/gamedescription.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/gamedescription.js | 113| 113| if (isAI) | 114| 114| { | 115| 115| if (playerData.Civ) | 116| |- { | | 116|+ | 117| 117| if (isActive) | 118| 118| // Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu | 119| 119| playerDescription = translate("%(playerName)s (%(civ)s, %(AIdescription)s)"); | 120| 120| else | 121| 121| // Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu | 122| 122| playerDescription = translate("%(playerName)s (%(civ)s, %(AIdescription)s, %(state)s)"); | 123| |- } | | 123|+ | 124| 124| else | 125| 125| { | 126| 126| if (isActive) | | [NORMAL] ESLintBear (curly): | | Unnecessary { after 'else'. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/gamedescription.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/gamedescription.js | 122| 122| playerDescription = translate("%(playerName)s (%(civ)s, %(AIdescription)s, %(state)s)"); | 123| 123| } | 124| 124| else | 125| |- { | | 125|+ | 126| 126| if (isActive) | 127| 127| // Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu | 128| 128| playerDescription = translate("%(playerName)s (%(AIdescription)s)"); | 129| 129| else | 130| 130| // Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu | 131| 131| playerDescription = translate("%(playerName)s (%(AIdescription)s, %(state)s)"); | 132| |- } | | 132|+ | 133| 133| } | 134| 134| else | 135| 135| { | | [NORMAL] ESLintBear (curly): | | Unnecessary { after 'else'. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/gamedescription.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/gamedescription.js | 132| 132| } | 133| 133| } | 134| 134| else | 135| |- { | | 135|+ | 136| 136| if (playerData.Offline) | 137| 137| { | 138| 138| // Can only occur in the lobby for now, so no strings with civ needed | 160| 160| // Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu | 161| 161| playerDescription = translate("%(playerName)s (%(state)s)"); | 162| 162| } | 163| |- } | | 163|+ | 164| 164| | 165| 165| // Sort player descriptions by team | 166| 166| if (!playerDescriptions[teamIdx]) | | [NORMAL] ESLintBear (curly): | | Unnecessary { after 'if' condition. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/gamedescription.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/gamedescription.js | 134| 134| else | 135| 135| { | 136| 136| if (playerData.Offline) | 137| |- { | | 137|+ | 138| 138| // Can only occur in the lobby for now, so no strings with civ needed | 139| 139| if (isActive) | 140| 140| // Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu | 142| 142| else | 143| 143| // Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu | 144| 144| playerDescription = translate("%(playerName)s (OFFLINE, %(state)s)"); | 145| |- } | | 145|+ | 146| 146| else | 147| 147| { | 148| 148| if (playerData.Civ) | | [NORMAL] ESLintBear (curly): | | Unnecessary { after 'else'. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/gamedescription.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/gamedescription.js | 144| 144| playerDescription = translate("%(playerName)s (OFFLINE, %(state)s)"); | 145| 145| } | 146| 146| else | 147| |- { | | 147|+ | 148| 148| if (playerData.Civ) | 149| 149| if (isActive) | 150| 150| // Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu | 159| 159| else | 160| 160| // Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu | 161| 161| playerDescription = translate("%(playerName)s (%(state)s)"); | 162| |- } | | 162|+ | 163| 163| } | 164| 164| | 165| 165| // Sort player descriptions by team | | [NORMAL] ESLintBear (indent): | | Expected indentation of 4 tabs but found 5. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/gamedescription.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/gamedescription.js | 153| 153| // Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu | 154| 154| playerDescription = translate("%(playerName)s (%(civ)s, %(state)s)"); | 155| 155| else | 156| |- if (isActive) | | 156|+ if (isActive) | 157| 157| // Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu | 158| 158| playerDescription = translate("%(playerName)s"); | 159| 159| else | | [NORMAL] ESLintBear (indent): | | Expected indentation of 4 tabs but found 6. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/gamedescription.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/gamedescription.js | 154| 154| playerDescription = translate("%(playerName)s (%(civ)s, %(state)s)"); | 155| 155| else | 156| 156| if (isActive) | 157| |- // Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu | | 157|+ // Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu | 158| 158| playerDescription = translate("%(playerName)s"); | 159| 159| else | 160| 160| // Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu | | [NORMAL] ESLintBear (indent): | | Expected indentation of 5 tabs but found 6. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/gamedescription.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/gamedescription.js | 155| 155| else | 156| 156| if (isActive) | 157| 157| // Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu | 158| |- playerDescription = translate("%(playerName)s"); | | 158|+ playerDescription = translate("%(playerName)s"); | 159| 159| else | 160| 160| // Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu | 161| 161| playerDescription = translate("%(playerName)s (%(state)s)"); | | [NORMAL] ESLintBear (indent): | | Expected indentation of 4 tabs but found 5. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/gamedescription.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/gamedescription.js | 156| 156| if (isActive) | 157| 157| // Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu | 158| 158| playerDescription = translate("%(playerName)s"); | 159| |- else | | 159|+ else | 160| 160| // Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu | 161| 161| playerDescription = translate("%(playerName)s (%(state)s)"); | 162| 162| } | | [NORMAL] ESLintBear (indent): | | Expected indentation of 4 tabs but found 6. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/gamedescription.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/gamedescription.js | 157| 157| // Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu | 158| 158| playerDescription = translate("%(playerName)s"); | 159| 159| else | 160| |- // Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu | | 160|+ // Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu | 161| 161| playerDescription = translate("%(playerName)s (%(state)s)"); | 162| 162| } | 163| 163| } | | [NORMAL] ESLintBear (indent): | | Expected indentation of 5 tabs but found 6. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/gamedescription.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/gamedescription.js | 158| 158| playerDescription = translate("%(playerName)s"); | 159| 159| else | 160| 160| // Translation: Describe a player in a selected game, f.e. in the replay- or savegame menu | 161| |- playerDescription = translate("%(playerName)s (%(state)s)"); | | 161|+ playerDescription = translate("%(playerName)s (%(state)s)"); | 162| 162| } | 163| 163| } | 164| 164| | | [NORMAL] ESLintBear (indent): | | Expected indentation of 4 tabs but found 5. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/gamedescription.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/gamedescription.js | 348| 348| titles.push({ | 349| 349| "label": translate("Map Description"), | 350| 350| "value": g_GameAttributes.settings.Description ? | 351| |- translate(g_GameAttributes.settings.Description) : | | 351|+ translate(g_GameAttributes.settings.Description) : | 352| 352| translate("Sorry, no description available.") | 353| 353| }); | 354| 354| } | | [NORMAL] ESLintBear (indent): | | Expected indentation of 4 tabs but found 5. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/gamedescription.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/gamedescription.js | 349| 349| "label": translate("Map Description"), | 350| 350| "value": g_GameAttributes.settings.Description ? | 351| 351| translate(g_GameAttributes.settings.Description) : | 352| |- translate("Sorry, no description available.") | | 352|+ translate("Sorry, no description available.") | 353| 353| }); | 354| 354| } | 355| 355| binaries/data/mods/public/gui/common/gamedescription.js | 388| » » let·difficulty·=·g_Settings.TriggerDifficulties.find(difficulty·=>·difficulty.Difficulty·==·g_GameAttributes.settings.TriggerDifficulty); | | [NORMAL] ESLintBear (no-shadow): | | 'difficulty' is already declared in the upper scope. | | [NORMAL] ESLintBear (indent): | | Expected indentation of 1 tab but found 2. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | 62| 62| var g_RomanNumbers = [undefined, "I", "II", "III", "IV", "V", "VI", "VII", "VIII"]; | 63| 63| | 64| 64| var g_PlayerTeamList = prepareForDropdown([{ | 65| |- "label": translateWithContext("team", "None"), | | 65|+ "label": translateWithContext("team", "None"), | 66| 66| "id": -1 | 67| 67| }].concat( | 68| 68| Array(g_MaxTeams).fill(0).map((v, i) => ({ | | [NORMAL] ESLintBear (indent): | | Expected indentation of 1 tab but found 2. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | 63| 63| | 64| 64| var g_PlayerTeamList = prepareForDropdown([{ | 65| 65| "label": translateWithContext("team", "None"), | 66| |- "id": -1 | | 66|+ "id": -1 | 67| 67| }].concat( | 68| 68| Array(g_MaxTeams).fill(0).map((v, i) => ({ | 69| 69| "label": i + 1, | | [NORMAL] ESLintBear (indent): | | Expected indentation of 0 tabs but found 1. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | 64| 64| var g_PlayerTeamList = prepareForDropdown([{ | 65| 65| "label": translateWithContext("team", "None"), | 66| 66| "id": -1 | 67| |- }].concat( | | 67|+}].concat( | 68| 68| Array(g_MaxTeams).fill(0).map((v, i) => ({ | 69| 69| "label": i + 1, | 70| 70| "id": i | | [NORMAL] ESLintBear (indent): | | Expected indentation of 1 tab but found 2. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | 65| 65| "label": translateWithContext("team", "None"), | 66| 66| "id": -1 | 67| 67| }].concat( | 68| |- Array(g_MaxTeams).fill(0).map((v, i) => ({ | | 68|+ Array(g_MaxTeams).fill(0).map((v, i) => ({ | 69| 69| "label": i + 1, | 70| 70| "id": i | 71| 71| })) | | [NORMAL] ESLintBear (indent): | | Expected indentation of 2 tabs but found 3. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | 66| 66| "id": -1 | 67| 67| }].concat( | 68| 68| Array(g_MaxTeams).fill(0).map((v, i) => ({ | 69| |- "label": i + 1, | | 69|+ "label": i + 1, | 70| 70| "id": i | 71| 71| })) | 72| 72| ) | | [NORMAL] ESLintBear (indent): | | Expected indentation of 2 tabs but found 3. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | 67| 67| }].concat( | 68| 68| Array(g_MaxTeams).fill(0).map((v, i) => ({ | 69| 69| "label": i + 1, | 70| |- "id": i | | 70|+ "id": i | 71| 71| })) | 72| 72| ) | 73| 73| ); | | [NORMAL] ESLintBear (indent): | | Expected indentation of 1 tab but found 2. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | 68| 68| Array(g_MaxTeams).fill(0).map((v, i) => ({ | 69| 69| "label": i + 1, | 70| 70| "id": i | 71| |- })) | | 71|+ })) | 72| 72| ) | 73| 73| ); | 74| 74| | | [NORMAL] ESLintBear (indent): | | Expected indentation of 0 tabs but found 1. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | 69| 69| "label": i + 1, | 70| 70| "id": i | 71| 71| })) | 72| |- ) | | 72|+) | 73| 73| ); | 74| 74| | 75| 75| /** | | [NORMAL] ESLintBear (indent): | | Expected indentation of 1 tab but found 2. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | 78| 78| var g_RelicCountList = Object.keys(g_CivData).map((civ, i) => i + 1); | 79| 79| | 80| 80| var g_PlayerCivList = g_CivData && prepareForDropdown([{ | 81| |- "name": translateWithContext("civilization", "Random"), | | 81|+ "name": translateWithContext("civilization", "Random"), | 82| 82| "tooltip": translate("Picks one civilization at random when the game starts."), | 83| 83| "color": g_ColorRandom, | 84| 84| "code": "random" | | [NORMAL] ESLintBear (indent): | | Expected indentation of 1 tab but found 2. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | 79| 79| | 80| 80| var g_PlayerCivList = g_CivData && prepareForDropdown([{ | 81| 81| "name": translateWithContext("civilization", "Random"), | 82| |- "tooltip": translate("Picks one civilization at random when the game starts."), | | 82|+ "tooltip": translate("Picks one civilization at random when the game starts."), | 83| 83| "color": g_ColorRandom, | 84| 84| "code": "random" | 85| 85| }].concat( | | [NORMAL] ESLintBear (indent): | | Expected indentation of 1 tab but found 2. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | 80| 80| var g_PlayerCivList = g_CivData && prepareForDropdown([{ | 81| 81| "name": translateWithContext("civilization", "Random"), | 82| 82| "tooltip": translate("Picks one civilization at random when the game starts."), | 83| |- "color": g_ColorRandom, | | 83|+ "color": g_ColorRandom, | 84| 84| "code": "random" | 85| 85| }].concat( | 86| 86| Object.keys(g_CivData).filter( | | [NORMAL] ESLintBear (indent): | | Expected indentation of 1 tab but found 2. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | 81| 81| "name": translateWithContext("civilization", "Random"), | 82| 82| "tooltip": translate("Picks one civilization at random when the game starts."), | 83| 83| "color": g_ColorRandom, | 84| |- "code": "random" | | 84|+ "code": "random" | 85| 85| }].concat( | 86| 86| Object.keys(g_CivData).filter( | 87| 87| civ => g_CivData[civ].SelectableInGameSetup | | [NORMAL] ESLintBear (indent): | | Expected indentation of 0 tabs but found 1. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | 82| 82| "tooltip": translate("Picks one civilization at random when the game starts."), | 83| 83| "color": g_ColorRandom, | 84| 84| "code": "random" | 85| |- }].concat( | | 85|+}].concat( | 86| 86| Object.keys(g_CivData).filter( | 87| 87| civ => g_CivData[civ].SelectableInGameSetup | 88| 88| ).map(civ => ({ | | [NORMAL] ESLintBear (indent): | | Expected indentation of 1 tab but found 2. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | 83| 83| "color": g_ColorRandom, | 84| 84| "code": "random" | 85| 85| }].concat( | 86| |- Object.keys(g_CivData).filter( | | 86|+ Object.keys(g_CivData).filter( | 87| 87| civ => g_CivData[civ].SelectableInGameSetup | 88| 88| ).map(civ => ({ | 89| 89| "name": g_CivData[civ].Name, | | [NORMAL] ESLintBear (indent): | | Expected indentation of 2 tabs but found 3. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | 84| 84| "code": "random" | 85| 85| }].concat( | 86| 86| Object.keys(g_CivData).filter( | 87| |- civ => g_CivData[civ].SelectableInGameSetup | | 87|+ civ => g_CivData[civ].SelectableInGameSetup | 88| 88| ).map(civ => ({ | 89| 89| "name": g_CivData[civ].Name, | 90| 90| "tooltip": g_CivData[civ].History, | | [NORMAL] ESLintBear (indent): | | Expected indentation of 1 tab but found 2. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | 85| 85| }].concat( | 86| 86| Object.keys(g_CivData).filter( | 87| 87| civ => g_CivData[civ].SelectableInGameSetup | 88| |- ).map(civ => ({ | | 88|+ ).map(civ => ({ | 89| 89| "name": g_CivData[civ].Name, | 90| 90| "tooltip": g_CivData[civ].History, | 91| 91| "color": g_ColorRegular, | | [NORMAL] ESLintBear (indent): | | Expected indentation of 2 tabs but found 3. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | 86| 86| Object.keys(g_CivData).filter( | 87| 87| civ => g_CivData[civ].SelectableInGameSetup | 88| 88| ).map(civ => ({ | 89| |- "name": g_CivData[civ].Name, | | 89|+ "name": g_CivData[civ].Name, | 90| 90| "tooltip": g_CivData[civ].History, | 91| 91| "color": g_ColorRegular, | 92| 92| "code": civ | | [NORMAL] ESLintBear (indent): | | Expected indentation of 2 tabs but found 3. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | 87| 87| civ => g_CivData[civ].SelectableInGameSetup | 88| 88| ).map(civ => ({ | 89| 89| "name": g_CivData[civ].Name, | 90| |- "tooltip": g_CivData[civ].History, | | 90|+ "tooltip": g_CivData[civ].History, | 91| 91| "color": g_ColorRegular, | 92| 92| "code": civ | 93| 93| })).sort(sortNameIgnoreCase) | | [NORMAL] ESLintBear (indent): | | Expected indentation of 2 tabs but found 3. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | 88| 88| ).map(civ => ({ | 89| 89| "name": g_CivData[civ].Name, | 90| 90| "tooltip": g_CivData[civ].History, | 91| |- "color": g_ColorRegular, | | 91|+ "color": g_ColorRegular, | 92| 92| "code": civ | 93| 93| })).sort(sortNameIgnoreCase) | 94| 94| ) | | [NORMAL] ESLintBear (indent): | | Expected indentation of 2 tabs but found 3. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | 89| 89| "name": g_CivData[civ].Name, | 90| 90| "tooltip": g_CivData[civ].History, | 91| 91| "color": g_ColorRegular, | 92| |- "code": civ | | 92|+ "code": civ | 93| 93| })).sort(sortNameIgnoreCase) | 94| 94| ) | 95| 95| ); | | [NORMAL] ESLintBear (indent): | | Expected indentation of 1 tab but found 2. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | 90| 90| "tooltip": g_CivData[civ].History, | 91| 91| "color": g_ColorRegular, | 92| 92| "code": civ | 93| |- })).sort(sortNameIgnoreCase) | | 93|+ })).sort(sortNameIgnoreCase) | 94| 94| ) | 95| 95| ); | 96| 96| | | [NORMAL] ESLintBear (indent): | | Expected indentation of 0 tabs but found 1. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | 91| 91| "color": g_ColorRegular, | 92| 92| "code": civ | 93| 93| })).sort(sortNameIgnoreCase) | 94| |- ) | | 94|+) | 95| 95| ); | 96| 96| | 97| 97| /** | | [NORMAL] ESLintBear (indent): | | Expected indentation of 2 tabs but found 1. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js |1133|1133| translate("%(hotkey_civinfo)s / %(hotkey_structree)s: View History / Structure Tree\nLast opened will be reopened on click."), { |1134|1134| "hotkey_civinfo": colorizeHotkey("%(hotkey)s", "civinfo"), |1135|1135| "hotkey_structree": colorizeHotkey("%(hotkey)s", "structree") |1136| |- }); | |1136|+ }); |1137|1137| } |1138|1138| |1139|1139| function initDefaults() | | [NORMAL] ESLintBear (no-trailing-spaces): | | Trailing spaces not allowed. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js |1227|1227| offset = -Math.min(slideSpeed * dt, maxOffset); |1228|1228| } |1229|1229| |1230| |- updateSettingsPanelPosition(offset); | |1230|+ updateSettingsPanelPosition(offset); |1231|1231| } |1232|1232| |1233|1233| /** | | [NORMAL] ESLintBear (curly): | | Unnecessary { after 'if' condition. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/gamesetup/gamesetup.js |1755|1755| let biomeList; |1756|1756| |1757|1757| if (g_GameAttributes.mapType == "random" && g_GameAttributes.settings.SupportedBiomes) |1758| |- { | |1758|+ |1759|1759| if (typeof g_GameAttributes.settings.SupportedBiomes == "string") |1760|1760| biomeList = g_Settings.Biomes.filter(biome => biome.Id.startsWith(g_GameAttributes.settings.SupportedBiomes)); |1761|1761| else |1762|1762| biomeList = g_Settings.Biomes.filter( |1763|1763| biome => g_GameAttributes.settings.SupportedBiomes.indexOf(biome.Id) != -1); |1764| |- } | |1764|+ |1765|1765| |1766|1766| g_BiomeList = biomeList && prepareForDropdown( |1767|1767| [{ binaries/data/mods/public/gui/gamesetup/gamesetup.js |1999| » while·(g_IsNetworked) | | [NORMAL] ESLintBear (no-unmodified-loop-condition): | | 'g_IsNetworked' is not modified in this loop.
Link to build: https://jenkins.wildfiregames.com/job/differential/660/display/redirect
There are multiple ways to set oneself as a reviewer. One is intending to comprehend, examine and participate in the feature design, software design and implementation, testing stage, future maintenance. The other is not intending to do any of that but only marking one thing one noticed as a requested change and then not noticing that this property set oneself as a performer of the mentioned tasks.
binaries/data/mods/public/gui/common/gamedescription.js | ||
---|---|---|
47 ↗ | (On Diff #6778) | Subsets of the first category may want to argue for moving art/textures/ to the JSInteface function, thus limiting the potential damage that JS functions could do. But I decided against it because I don't see why the JS Interface function should be more restricted than the TextureManager. |
If something is bad, then it should be fixed. If it's a full change or a part of. C++ is much more strict than JS. So you need to follow rules. But not to commit without reviews/approves.
It's the normal practise to be reviewer of a part of a code.
It's the normal practise to be reviewer of a part of a code.
"Reviewers: Player 1" says that the patch has been or is supposed to be reviewed by "Player 1".
So if you set yourself as the only reviewer, then that sets you the reviewer of the patch.
If you don't intend to review the entire patch, say so in advance and don't set yourself to a reviewer unless completing the list of reviewers.
If there is noone that can perform a review of the rest of the patch, then the right thing to do is to post all the comments (such a the "const") without setting oneself as a reviewer.
I don't set myself as a reviewer, it happens automatically, because I mark it as Needs Changes. I agree with your point, I will removing myself from reviewers manually in similar cases after my objections will be completed.
I'd only prefer for a better name, but we didn't decide it. And I don't see problems in the code. So the patch is ok for me.
binaries/data/mods/public/gui/common/gamedescription.js | ||
---|---|---|
44 ↗ | (On Diff #6778) | It'd good to not hardcode the extension in future. |