Page MenuHomeWildfire Games
Differential D2407

Delete void "debug-print", unused "chat" simulation command and non-existent "aichat" network message
ClosedPublic
Actions

Authored by elexis on Nov 5 2019, 3:46 PM.

Details

Reviewers
None
Commits
rP23137: Delete dead "chat" simulation command, "chat" GUIInterface notification type…
Summary

rP7653 introduced the "debug-print" simulation command which consumes a string provided by the GUI, sends it to the simulation and then shows that string in the terminal output.
But the JS console already has that function, a complete eval since rP469, so it can be deleted without any impact on debuggability. Aside from that this should really not be a simulation command (let alone a command available to every client).

rP14991 enabled translations for the tutorial AI sent messages (though GUIInterface) by inserting a new type "aichat".
However it (1) added the "aichat" case not only for message parsing by the simulation GUIInterface notifications but also for the NetClient message parsing, but the NetClient never sends this message, so that case is dead and misleading (certainly I was fooled into assuming it had a reason to exist ever since)
and (2) the untranslated "chat" simulation command has no more use case, since user provided chat strings should not be sent through the simulation because chat should also be sent during extended match (simulation) pauses, and since the code-provided strings should always be translated (even if they shouldn't it would be nicer to add a property translated = false)

rP17796 added translateMessage to an aichat GUIInterface notification that isn't used by that type (only by 'timenotifications', i.e. !type || type == "message").

Test Plan

Verify that the removal is correct. Read the other commands to see that this removal is complete, i.e. no further unused / dead cases exist. Mind that rP19155/D65 already had to remove a bad debugging command.
Notice that it is still awkward to use player commands to send GUIInterface notifications, but that this is currently used by Petra to chat and the bot cannot access the GUIInterface, at least easily currently (so could be done independently if desired).
Notice that no other "aichat" GUIinterface notifications use wrong translation parameters.

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.Nov 5 2019, 3:46 PM
Vulcan added a comment.Nov 5 2019, 3:48 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/539/display/redirect

Vulcan added a comment.Nov 5 2019, 3:52 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/messages.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/messages.js
| 357| 357| 	let notificationText =
| 358| 358| 		notification.instructions.reduce((instructions, item) =>
| 359| 359| 			instructions + (typeof item == "string" ? translate(item) : colorizeHotkey(translate(item.text), item.hotkey)),
| 360|    |-			"");
|    | 360|+		"");
| 361| 361| 
| 362| 362| 	Engine.GetGUIObjectByName("tutorialText").caption = g_TutorialMessages.concat(setStringTags(notificationText, g_TutorialNewMessageTags)).join("\n");
| 363| 363| 	g_TutorialMessages.push(notificationText);
|    | [NORMAL] ESLintBear (operator-linebreak):
|    | '&&' should be placed at the end of the line.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
| 527| 527| 			if (cmpGarrisonHolder)
| 528| 528| 			{
| 529| 529| 				// Only the owner of the garrisonHolder may unload entities from any owners
| 530|    |-				if (!IsOwnedByPlayer(player, garrisonHolder) && !data.controlAllUnits
| 531|    |-				    && player != +cmd.owner)
|    | 530|+				if (!IsOwnedByPlayer(player, garrisonHolder) && !data.controlAllUnits &&
|    | 531|+				    player != +cmd.owner)
| 532| 532| 						continue;
| 533| 533| 
| 534| 534| 				if (!cmpGarrisonHolder.UnloadTemplate(cmd.template, cmd.owner, cmd.all))
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 5 tabs but found 6.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
| 529| 529| 				// Only the owner of the garrisonHolder may unload entities from any owners
| 530| 530| 				if (!IsOwnedByPlayer(player, garrisonHolder) && !data.controlAllUnits
| 531| 531| 				    && player != +cmd.owner)
| 532|    |-						continue;
|    | 532|+					continue;
| 533| 533| 
| 534| 534| 				if (!cmpGarrisonHolder.UnloadTemplate(cmd.template, cmd.owner, cmd.all))
| 535| 535| 					notifyUnloadFailure(player, garrisonHolder);
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'metadata'.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|1132|1132| 
|1133|1133| 	// send Metadata info if any
|1134|1134| 	if (cmd.metadata)
|1135|    |-		Engine.PostMessage(ent, MT_AIMetadata, { "id": ent, "metadata" : cmd.metadata, "owner" : player } );
|    |1135|+		Engine.PostMessage(ent, MT_AIMetadata, { "id": ent, "metadata": cmd.metadata, "owner" : player } );
|1136|1136| 
|1137|1137| 	// Tell the units to start building this new entity
|1138|1138| 	if (cmd.autorepair)
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'owner'.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|1132|1132| 
|1133|1133| 	// send Metadata info if any
|1134|1134| 	if (cmd.metadata)
|1135|    |-		Engine.PostMessage(ent, MT_AIMetadata, { "id": ent, "metadata" : cmd.metadata, "owner" : player } );
|    |1135|+		Engine.PostMessage(ent, MT_AIMetadata, { "id": ent, "metadata" : cmd.metadata, "owner": player } );
|1136|1136| 
|1137|1137| 	// Tell the units to start building this new entity
|1138|1138| 	if (cmd.autorepair)
|    | [NORMAL] ESLintBear (space-in-parens):
|    | There should be no spaces inside this paren.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|1132|1132| 
|1133|1133| 	// send Metadata info if any
|1134|1134| 	if (cmd.metadata)
|1135|    |-		Engine.PostMessage(ent, MT_AIMetadata, { "id": ent, "metadata" : cmd.metadata, "owner" : player } );
|    |1135|+		Engine.PostMessage(ent, MT_AIMetadata, { "id": ent, "metadata" : cmd.metadata, "owner" : player });
|1136|1136| 
|1137|1137| 	// Tell the units to start building this new entity
|1138|1138| 	if (cmd.autorepair)
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|1240|1240| 		}
|1241|1241| 
|1242|1242| 		lastTowerControlGroup = cmpSnappedStartObstruction.GetControlGroup();
|1243|    |-		//warn("setting lastTowerControlGroup to control group of start snapped entity " + cmd.startSnappedEntity + ": " + lastTowerControlGroup);
|    |1243|+		// warn("setting lastTowerControlGroup to control group of start snapped entity " + cmd.startSnappedEntity + ": " + lastTowerControlGroup);
|1244|1244| 	}
|1245|1245| 
|1246|1246| 	var i = 0;
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|1304|1304| 
|1305|1305| 				if (i > 0)
|1306|1306| 				{
|1307|    |-					//warn("   updating previous wall piece's secondary control group to " + newTowerControlGroup);
|    |1307|+					// warn("   updating previous wall piece's secondary control group to " + newTowerControlGroup);
|1308|1308| 					var cmpPreviousObstruction = Engine.QueryInterface(pieces[i-1].ent, IID_Obstruction);
|1309|1309| 					// TODO: ensure that cmpPreviousObstruction exists
|1310|1310| 					// TODO: ensure that the previous obstruction does not yet have a secondary control group set
|    | [NORMAL] ESLintBear (operator-linebreak):
|    | '&&' should be placed at the end of the line.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|1467|1467| 		// Check that all its members are selected
|1468|1468| 		var fid = formationIds[0];
|1469|1469| 		var cmpFormation = Engine.QueryInterface(+fid, IID_Formation);
|1470|    |-		if (cmpFormation && cmpFormation.GetMemberCount() == formation.members[fid].length
|1471|    |-			&& cmpFormation.GetMemberCount() == formation.entities.length)
|    |1470|+		if (cmpFormation && cmpFormation.GetMemberCount() == formation.members[fid].length &&
|    |1471|+			cmpFormation.GetMemberCount() == formation.entities.length)
|1472|1472| 		{
|1473|1473| 			cmpFormation.DeleteTwinFormations();
|1474|1474| 			// The whole formation was selected, so reuse its controller for this command
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|1575|1575| 		for (var i = matrix.length - 1; i >= 0 && !closeClusters; --i)
|1576|1576| 			for (var j = i - 1; j >= 0 && !closeClusters; --j)
|1577|1577| 				if (matrix[i][j] < distSq)
|1578|    |-					closeClusters = [i,j];
|    |1578|+					closeClusters = [i, j];
|1579|1579| 
|1580|1580| 		// if no more close clusters found, just return all found clusters so far
|1581|1581| 		if (!closeClusters)
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|1597|1597| 		}
|1598|1598| 		// remove the rows and columns in the matrix for the merged clusters,
|1599|1599| 		// and the clusters themselves from the cluster list
|1600|    |-		clusters.splice(closeClusters[0],1);
|    |1600|+		clusters.splice(closeClusters[0], 1);
|1601|1601| 		clusters.splice(closeClusters[1],1);
|1602|1602| 		matrix.splice(closeClusters[0],1);
|1603|1603| 		matrix.splice(closeClusters[1],1);
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|1598|1598| 		// remove the rows and columns in the matrix for the merged clusters,
|1599|1599| 		// and the clusters themselves from the cluster list
|1600|1600| 		clusters.splice(closeClusters[0],1);
|1601|    |-		clusters.splice(closeClusters[1],1);
|    |1601|+		clusters.splice(closeClusters[1], 1);
|1602|1602| 		matrix.splice(closeClusters[0],1);
|1603|1603| 		matrix.splice(closeClusters[1],1);
|1604|1604| 		for (let i = 0; i < matrix.length; ++i)
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|1599|1599| 		// and the clusters themselves from the cluster list
|1600|1600| 		clusters.splice(closeClusters[0],1);
|1601|1601| 		clusters.splice(closeClusters[1],1);
|1602|    |-		matrix.splice(closeClusters[0],1);
|    |1602|+		matrix.splice(closeClusters[0], 1);
|1603|1603| 		matrix.splice(closeClusters[1],1);
|1604|1604| 		for (let i = 0; i < matrix.length; ++i)
|1605|1605| 		{
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|1600|1600| 		clusters.splice(closeClusters[0],1);
|1601|1601| 		clusters.splice(closeClusters[1],1);
|1602|1602| 		matrix.splice(closeClusters[0],1);
|1603|    |-		matrix.splice(closeClusters[1],1);
|    |1603|+		matrix.splice(closeClusters[1], 1);
|1604|1604| 		for (let i = 0; i < matrix.length; ++i)
|1605|1605| 		{
|1606|1606| 			if (matrix[i].length > closeClusters[0])
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|1604|1604| 		for (let i = 0; i < matrix.length; ++i)
|1605|1605| 		{
|1606|1606| 			if (matrix[i].length > closeClusters[0])
|1607|    |-				matrix[i].splice(closeClusters[0],1);
|    |1607|+				matrix[i].splice(closeClusters[0], 1);
|1608|1608| 			if (matrix[i].length > closeClusters[1])
|1609|1609| 				matrix[i].splice(closeClusters[1],1);
|1610|1610| 		}
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|1606|1606| 			if (matrix[i].length > closeClusters[0])
|1607|1607| 				matrix[i].splice(closeClusters[0],1);
|1608|1608| 			if (matrix[i].length > closeClusters[1])
|1609|    |-				matrix[i].splice(closeClusters[1],1);
|    |1609|+				matrix[i].splice(closeClusters[1], 1);
|1610|1610| 		}
|1611|1611| 		// add a new row of distances to the matrix and the new cluster
|1612|1612| 		clusters.push(newCluster);

binaries/data/mods/public/simulation/helpers/Commands.js
|  43| »   if·(g_Commands[cmd.type])
|    | [MAJOR] ESLintBear (no-use-before-define):
|    | 'g_Commands' was used before it was defined.

binaries/data/mods/public/simulation/helpers/Commands.js
|  47| »   »   g_Commands[cmd.type](player,·cmd,·data);
|    | [MAJOR] ESLintBear (no-use-before-define):
|    | 'g_Commands' was used before it was defined.

binaries/data/mods/public/simulation/helpers/Commands.js
| 778| »   »   let·ent·=·pickRandom(cmpRangeManager.GetEntitiesByPlayer(cmd.player).filter(ent·=>·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'ent' is already declared in the upper scope.

binaries/data/mods/public/simulation/helpers/Commands.js
|1263| ····»   »   »   error("[TryConstructWall]·Expected·last·tower·control·group·to·be·available,·none·found·(1st·pass,·iteration·"·+·i·+·")");
|    | [NORMAL] ESLintBear (no-mixed-spaces-and-tabs):
|    | Mixed spaces and tabs.

binaries/data/mods/public/simulation/helpers/Commands.js
|1264| ····»   »   »   break;
|    | [NORMAL] ESLintBear (no-mixed-spaces-and-tabs):
|    | Mixed spaces and tabs.

binaries/data/mods/public/simulation/helpers/Commands.js
|1494| »   »   »   »   var·lastFormationTemplate·=·undefined;
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'lastFormationTemplate' to undefined.

binaries/data/mods/public/simulation/helpers/Commands.js
|1573| »   »   var·closeClusters·=·undefined;
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'closeClusters' to undefined.

binaries/data/mods/public/simulation/helpers/Commands.js
|1590| »   »   for·(let·i·=·0;·i·<·clusters.length;·++i)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/simulation/helpers/Commands.js
|1604| »   »   for·(let·i·=·0;·i·<·matrix.length;·++i)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/simulation/helpers/Commands.js
|  53| var·g_Commands·=·{
|    | [NORMAL] JSHintBear:
|    | 'g_Commands' was used before it was defined.

binaries/data/mods/public/simulation/helpers/Commands.js
| 531| »   »   »   »   ····&&·player·!=·+cmd.owner)
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/helpers/Commands.js
| 718| »   »   »   »   var·cmpGUIInterface·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_GuiInterface);
|    | [NORMAL] JSHintBear:
|    | 'cmpGUIInterface' is already defined.

binaries/data/mods/public/simulation/helpers/Commands.js
| 937| »   »   for·(var·i·=·0;·i·<·length;·++i)
|    | [NORMAL] JSHintBear:
|    | 'i' is already defined.

binaries/data/mods/public/simulation/helpers/Commands.js
| 950| »   »   var·count·=·0;
|    | [NORMAL] JSHintBear:
|    | 'count' is already defined.

binaries/data/mods/public/simulation/helpers/Commands.js
|1097| »   »   var·cmpGuiInterface·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_GuiInterface);
|    | [NORMAL] JSHintBear:
|    | 'cmpGuiInterface' is already defined.

binaries/data/mods/public/simulation/helpers/Commands.js
|1349| »   »   var·piece·=·pieces[j];
|    | [NORMAL] JSHintBear:
|    | 'piece' is already defined.

binaries/data/mods/public/simulation/helpers/Commands.js
|1432| »   »   var·cmpUnitAI·=·Engine.QueryInterface(ent,·IID_UnitAI);
|    | [NORMAL] JSHintBear:
|    | 'cmpUnitAI' is already defined.

binaries/data/mods/public/simulation/helpers/Commands.js
|1471| »   »   »   &&·cmpFormation.GetMemberCount()·==·formation.entities.length)
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/helpers/Commands.js
|1497| »   »   »   »   »   var·cmpUnitAI·=·Engine.QueryInterface(ent,·IID_UnitAI);
|    | [NORMAL] JSHintBear:
|    | 'cmpUnitAI' is already defined.

binaries/data/mods/public/simulation/helpers/Commands.js
|1530| »   »   »   var·cmpFormation·=·Engine.QueryInterface(formationEnt,·IID_Formation);
|    | [NORMAL] JSHintBear:
|    | 'cmpFormation' is already defined.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1052/display/redirect

Harbormaster completed remote builds in B9959: Diff 10274.Nov 5 2019, 3:52 PM
elexis added a comment.Nov 5 2019, 11:11 PM

Notice that no other "aichat" GUIinterface notifications use wrong translation parameters.

There are more than that. For example the developer overlay opening chat message, or PETRA.chatNewTradeRoute, PETRA.chatNewPhase, ...
And then theres more and more, and I got yet another 800 line patch, so yet another diff.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 5 2019, 11:36 PM
Closed by commit rP23137: Delete dead "chat" simulation command, "chat" GUIInterface notification type… (authored by elexis). · Explain Why
This revision was automatically updated to reflect the committed changes.
elexis added a commit: rP23137: Delete dead "chat" simulation command, "chat" GUIInterface notification type….
Owners added a subscriber: Restricted Owners Package.Nov 5 2019, 11:36 PM
elexis mentioned this in D2409: GuiInterface TimeNotifications fixes and cleanup.Nov 6 2019, 9:26 PM
Stan mentioned this in D3502: Remove debug-print action (and corresponding test).Jan 30 2021, 10:37 PM

Revision Contents
Changeset List

PathSize
ps/
trunk/
binaries/
data/
mods/
public/
gui/
session/
20 lines
simulation/
helpers/
10 lines

Diff 10276

View Options

ps/trunk/binaries/data/mods/public/gui/session/messages.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/simulation/helpers/Commands.js

Loading...
Wildfire Games · Phabricator