Page MenuHomeWildfire Games

JSInterface TextureExists function, fixes map specific biome previews
ClosedPublic

Authored by elexis on Jun 19 2018, 2:10 PM.

Details

Summary

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).

Test Plan

Make sure this works with both zipped and non-zipped mods.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Jun 19 2018, 2:10 PM
Vulcan added a subscriber: Vulcan.Jun 19 2018, 2:21 PM

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

vladislavbelov requested changes to this revision.Jun 19 2018, 4:03 PM
vladislavbelov added a subscriber: vladislavbelov.

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 ↗(On Diff #6769)

All getters that don't change a state of the object should be const.

This revision now requires changes to proceed.Jun 19 2018, 4:03 PM

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.

elexis added a comment.EditedJun 19 2018, 4:07 PM

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.

elexis requested review of this revision.Jun 19 2018, 4:10 PM

Anything other than the const?

vladislavbelov added a comment.EditedJun 19 2018, 4:39 PM
In D1583#63550, @elexis wrote:

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.

In D1583#63551, @elexis wrote:

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.

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.

It doesn't claim to either.

It claims by its name.

"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

elexis updated this revision to Diff 6778.Jun 21 2018, 9:26 PM

function const.
Unify replay menu and gamesetup biome preview logic.

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.

elexis added inline comments.Jun 23 2018, 3:52 PM
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.
So if files outside of art/textures/ are not supposed to be supported, then they shouldn't be supported in the TextureManager either (thus the path string would still not be in the JSInterface, keeping the latter minimal and dumb.

vladislavbelov added a comment.EditedJun 23 2018, 5:34 PM
In D1583#63713, @elexis wrote:

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.

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.

vladislavbelov added a comment.EditedJun 23 2018, 6:31 PM
In D1583#63717, @elexis wrote:

"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.

So this is a stateless revision proposal now?

vladislavbelov accepted this revision.Jun 24 2018, 6:30 PM

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.

This revision is now accepted and ready to land.Jun 24 2018, 6:30 PM
elexis updated the Trac tickets for this revision.Aug 5 2018, 4:33 PM
This revision was automatically updated to reflect the committed changes.
Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Aug 5 2018, 11:50 PM