Page MenuHomeWildfire Games

Rewrite session chat to use class syntax / object-oriented programming
ClosedPublic

Authored by elexis on Oct 5 2019, 12:13 PM.

Details

Summary

In the course of #5387, this patch groups all ingame chat in separate classes, removing all chat globals and localizing functions, defragmenting code.
It moves all JS code from XML files to JS.
It uses JS to register the global hotkeys instead of XML mock objects using rP22851/D2260.

Communication between the different classes is performed by using a simple subscription system similar to the one in rP22985/D2310.
This way mods can insert event handlers and thus have a greater chance of not having to overwrite / modify existing code but adding a hook instead.
Also it means that there is stronger separation of concerns, since one class doesnt have to hardcode logic of some other area of code.
For example when the filter or addressee dropdown selection changes, it doesnt have to hardcode which other GUI obejcts have to be updated.
The chat messages are split into several classes, so that the classes can have their own helper functions and helper objects storing strings without resorting to globals.

Notice that "/team" chat was deleted due to lack of a use case.
The hotkey already directs to /allies since last-man-standing mode integration, and the dropdown item isnt present in the current code either.

Notice that this code now supports changing the language during the game by using markForTranslation in the JS objects storing strings that are initialized only once.

Test Plan

Make sure that every single feature still works as before. That is:

  • Every chat message type that is implemented
  • the three hotkeys
  • PM hotkey where the previously selected addressee went offline and came back online (OFFLINE mock item)

One can use the following commands to start an MP session qiuckly:

play0ad -autostart="scenarios/Arcadia" -autostart-host -autostart-host-players=2 -autostart-playername="Bob" -autostart-player=1
play0ad -autostart="scenarios/Arcadia" -autostart-client=127.0.0.1 -autostart-playername="Alice" -autostart-player=2

Notice that many global references have not been removed from the new classes, most notably g_PlayerAssignments and g_Players.

addChatMessage and clearChatMessages are required by gui/common/network.js and should be removed eventually too, but not here since it will require much more refactoring.

Perhaps this will make #2640 slightly more feasible, but the differences between the three chat parsing pages are still tremendous.

Event Timeline

elexis created this revision.Oct 5 2019, 12:13 PM

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

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

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatMessageFormat.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatMessageFormat.js
|  18|  18| 	registerMessageHandlers(handlerTypes)
|  19|  19| 	{
|  20|  20| 		for (let type in handlerTypes)
|  21|    |-			this.registerMessageHandler(type, new handlerTypes[type]())
|    |  21|+			this.registerMessageHandler(type, new handlerTypes[type]());
|  22|  22| 	}
|  23|  23| 
|  24|  24| 	parseMessage(msg)
|    | [NORMAL] ESLintBear (no-extra-semi):
|    | Unnecessary semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatMessageFormat.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatMessageFormat.js
|  38|  38| 
|  39|  39| 		return undefined;
|  40|  40| 	}
|  41|    |-};
|    |  41|+}
|  42|  42| 
|  43|  43| ChatMessageFormat.System = class
|  44|  44| {

binaries/data/mods/public/gui/session/chat/ChatMessageFormat.js
|  36| »   »   »   »   return·txt;
|    | [NORMAL] ESLintBear (consistent-return):
|    | Method 'parseMessage' expected no return value.

binaries/data/mods/public/gui/session/chat/ChatMessageFormat.js
|  39| »   »   return·undefined;
|    | [NORMAL] ESLintBear (consistent-return):
|    | Method 'parseMessage' expected no return value.

binaries/data/mods/public/gui/session/chat/ChatMessageFormat.js
|  21| »   »   »   this.registerMessageHandler(type,·new·handlerTypes[type]())
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/gui/session/chat/ChatMessageFormat.js
|  41| };
|    | [NORMAL] JSHintBear:
|    | Unnecessary semicolon.
|    | [NORMAL] ESLintBear (no-extra-semi):
|    | Unnecessary semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatMessageFormatSimulation.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatMessageFormatSimulation.js
|   1|   1| class ChatMessageFormatSimulation
|   2|   2| {
|   3|    |-};
|    |   3|+}
|   4|   4| 
|   5|   5| ChatMessageFormatSimulation.attack = class
|   6|   6| {

binaries/data/mods/public/gui/session/chat/ChatMessageFormatSimulation.js
|   3| };
|    | [NORMAL] JSHintBear:
|    | Unnecessary semicolon.

binaries/data/mods/public/gui/session/menu.js
| 383| »   »   button.onPress·=·(function(player,·stance)·{·return·function()·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'stance' is already declared in the upper scope.

binaries/data/mods/public/gui/session/menu.js
| 415| »   »   button.onPress·=·(function(i,·resCode,·button)·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/menu.js
| 415| »   »   button.onPress·=·(function(i,·resCode,·button)·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'resCode' is already declared in the upper scope.

binaries/data/mods/public/gui/session/menu.js
| 415| »   »   button.onPress·=·(function(i,·resCode,·button)·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'button' is already declared in the upper scope.

binaries/data/mods/public/gui/session/menu.js
| 458| »   button.onPress·=·(function(i)·{·return·function()·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/menu.js
| 524| »   button.onPress·=·(function(i,·button)·{·return·function()·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/menu.js
| 524| »   button.onPress·=·(function(i,·button)·{·return·function()·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'button' is already declared in the upper scope.
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 6 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
| 399| 399| 				// Players see colors depending on diplomacy
| 400| 400| 				g_DisplayedPlayerColors[i] =
| 401| 401| 					g_ViewedPlayer == i ? getDiplomacyColor("self") :
| 402|    |-					g_Players[g_ViewedPlayer].isAlly[i] ? getDiplomacyColor("ally") :
|    | 402|+						g_Players[g_ViewedPlayer].isAlly[i] ? getDiplomacyColor("ally") :
| 403| 403| 					g_Players[g_ViewedPlayer].isNeutral[i] ? getDiplomacyColor("neutral") :
| 404| 404| 					getDiplomacyColor("enemy");
| 405| 405| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
| 400| 400| 				g_DisplayedPlayerColors[i] =
| 401| 401| 					g_ViewedPlayer == i ? getDiplomacyColor("self") :
| 402| 402| 					g_Players[g_ViewedPlayer].isAlly[i] ? getDiplomacyColor("ally") :
| 403|    |-					g_Players[g_ViewedPlayer].isNeutral[i] ? getDiplomacyColor("neutral") :
|    | 403|+							g_Players[g_ViewedPlayer].isNeutral[i] ? getDiplomacyColor("neutral") :
| 404| 404| 					getDiplomacyColor("enemy");
| 405| 405| 
| 406| 406| 		g_DisplayedPlayerColors[0] = g_Players[0].color;
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
| 401| 401| 					g_ViewedPlayer == i ? getDiplomacyColor("self") :
| 402| 402| 					g_Players[g_ViewedPlayer].isAlly[i] ? getDiplomacyColor("ally") :
| 403| 403| 					g_Players[g_ViewedPlayer].isNeutral[i] ? getDiplomacyColor("neutral") :
| 404|    |-					getDiplomacyColor("enemy");
|    | 404|+								getDiplomacyColor("enemy");
| 405| 405| 
| 406| 406| 		g_DisplayedPlayerColors[0] = g_Players[0].color;
| 407| 407| 	}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
| 663| 663| 					"civ": setStringTags(g_CivData[g_Players[g_ViewedPlayer].civ].Name, { "font": "sans-bold-stroke-14" }),
| 664| 664| 					"hotkey_civinfo": colorizeHotkey("%(hotkey)s", "civinfo"),
| 665| 665| 					"hotkey_structree": colorizeHotkey("%(hotkey)s", "structree")
| 666|    |-			});
|    | 666|+				});
| 667| 667| 	}
| 668| 668| 
| 669| 669| 	// Following gaia can be interesting on scripted maps
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
|1223|1223| 
|1224|1224| 	let orderHotkeyTooltip = Object.keys(viewablePlayerStates).length <= 1 ? "" :
|1225|1225| 		"\n" + sprintf(translate("%(order)s: %(hotkey)s to change order."), {
|1226|    |-		"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|    |1226|+			"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|1227|1227| 		"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|1228|1228| 	});
|1229|1229| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
|1224|1224| 	let orderHotkeyTooltip = Object.keys(viewablePlayerStates).length <= 1 ? "" :
|1225|1225| 		"\n" + sprintf(translate("%(order)s: %(hotkey)s to change order."), {
|1226|1226| 		"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|1227|    |-		"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|    |1227|+			"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|1228|1228| 	});
|1229|1229| 
|1230|1230| 	let resCodes = g_ResourceData.GetCodes();
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 1.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
|1225|1225| 		"\n" + sprintf(translate("%(order)s: %(hotkey)s to change order."), {
|1226|1226| 		"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|1227|1227| 		"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|1228|    |-	});
|    |1228|+		});
|1229|1229| 
|1230|1230| 	let resCodes = g_ResourceData.GetCodes();
|1231|1231| 	for (let r = 0; r < resCodes.length; ++r)

binaries/data/mods/public/gui/session/session.js
|1084| »   let·getPanelEntNameTooltip·=·panelEntState·=>·"[font=\"sans-bold-16\"]"·+·template.name.specific·+·"[/font]";
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'panelEntState' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1159| »   »   button.onpress·=·(function(i)·{·return·function()·{·performGroup((Engine.HotkeyIsPressed("selection.add")·?·"add"·:·"select"),·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1160| »   »   button.ondoublepress·=·(function(i)·{·return·function()·{·performGroup("snap",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1161| »   »   button.onpressright·=·(function(i)·{·return·function()·{·performGroup("breakUp",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.
|    | [NORMAL] ESLintBear (no-extra-semi):
|    | Unnecessary semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatHistory.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatHistory.js
|  54|  54| 
|  55|  55| 		this.chatMessages.push(historical);
|  56|  56| 	}
|  57|    |-};
|    |  57|+}
|  58|  58| 
|  59|  59| /**
|  60|  60|  * Notice only messages will be filtered that are visible to the player in the first place.

binaries/data/mods/public/gui/session/chat/ChatHistory.js
|  57| };
|    | [NORMAL] JSHintBear:
|    | Unnecessary semicolon.
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 4.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatAddressees.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatAddressees.js
|  38|  38| 
|  39|  39| 		let addressees = this.AddresseeTypes.filter(
|  40|  40| 			addresseeType => addresseeType.isSelectable()).map(
|  41|    |-				addresseeType => ({
|    |  41|+			addresseeType => ({
|  42|  42| 					"label": translateWithContext("chat addressee", addresseeType.label),
|  43|  43| 					"cmd": addresseeType.command
|  44|  44| 				}));
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatAddressees.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatAddressees.js
|  39|  39| 		let addressees = this.AddresseeTypes.filter(
|  40|  40| 			addresseeType => addresseeType.isSelectable()).map(
|  41|  41| 				addresseeType => ({
|  42|    |-					"label": translateWithContext("chat addressee", addresseeType.label),
|    |  42|+				"label": translateWithContext("chat addressee", addresseeType.label),
|  43|  43| 					"cmd": addresseeType.command
|  44|  44| 				}));
|  45|  45| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatAddressees.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatAddressees.js
|  40|  40| 			addresseeType => addresseeType.isSelectable()).map(
|  41|  41| 				addresseeType => ({
|  42|  42| 					"label": translateWithContext("chat addressee", addresseeType.label),
|  43|    |-					"cmd": addresseeType.command
|    |  43|+				"cmd": addresseeType.command
|  44|  44| 				}));
|  45|  45| 
|  46|  46| 		// Add playernames for private messages
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 4.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatAddressees.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatAddressees.js
|  41|  41| 				addresseeType => ({
|  42|  42| 					"label": translateWithContext("chat addressee", addresseeType.label),
|  43|  43| 					"cmd": addresseeType.command
|  44|    |-				}));
|    |  44|+			}));
|  45|  45| 
|  46|  46| 		// Add playernames for private messages
|  47|  47| 		let guids = sortGUIDsByPlayerID();
|    | [NORMAL] ESLintBear (no-extra-semi):
|    | Unnecessary semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatAddressees.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatAddressees.js
|  76|  76| 		this.chatAddressee.list_data = addressees.map(adressee => adressee.cmd);
|  77|  77| 		this.chatAddressee.selected = Math.max(0, this.chatAddressee.list_data.indexOf(oldChatAddressee));
|  78|  78| 	}
|  79|    |-};
|    |  79|+}
|  80|  80| 
|  81|  81| ChatAddressees.prototype.AddresseeTypes = [
|  82|  82| 	{

binaries/data/mods/public/gui/session/chat/ChatAddressees.js
|  79| };
|    | [NORMAL] JSHintBear:
|    | Unnecessary semicolon.
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatWindow.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatWindow.js
|  71|  71| 		{
|  72|  72| 			this.chatDialogPanel.size = this.chatDialogPanelLargeSize;
|  73|  73| 
|  74|    |-			let chatHistoryTextSize = this.chatHistoryText.getComputedSize()
|    |  74|+			let chatHistoryTextSize = this.chatHistoryText.getComputedSize();
|  75|  75| 			let width = this.aspectRatio * (chatHistoryTextSize.bottom - chatHistoryTextSize.top);
|  76|  76| 
|  77|  77| 			let size = this.chatDialogPanel.size;

binaries/data/mods/public/gui/session/chat/ChatWindow.js
|  74| »   »   »   let·chatHistoryTextSize·=·this.chatHistoryText.getComputedSize()
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
|    | [NORMAL] ESLintBear (no-extra-semi):
|    | Unnecessary semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatMessageFormatNetwork.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatMessageFormatNetwork.js
|   1|   1| class ChatMessageFormatNetwork
|   2|   2| {
|   3|    |-};
|    |   3|+}
|   4|   4| 
|   5|   5| ChatMessageFormatNetwork.clientlist = class
|   6|   6| {
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatMessageFormatNetwork.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatMessageFormatNetwork.js
|  61|  61| 				translate("%(player)s has rejoined the game.") :
|  62|  62| 				// Translation: A player joins the game for the first time
|  63|  63| 				translate("%(player)s has joined the game."),
|  64|    |-			{ "player": colorizePlayernameByGUID(msg.guid) })
|    |  64|+			{ "player": colorizePlayernameByGUID(msg.guid) });
|  65|  65| 	}
|  66|  66| };

binaries/data/mods/public/gui/session/chat/ChatMessageFormatNetwork.js
|   3| };
|    | [NORMAL] JSHintBear:
|    | Unnecessary semicolon.

binaries/data/mods/public/gui/session/chat/ChatMessageFormatNetwork.js
|  64| »   »   »   {·"player":·colorizePlayernameByGUID(msg.guid)·})
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
|    | [NORMAL] ESLintBear (space-before-function-paren):
|    | Unexpected space before function parentheses.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/messages.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/messages.js
| 272| 272| 		g_Selection.reset();
| 273| 273| 		g_Selection.addList(selection, false, cmd.type == "gather");
| 274| 274| 	},
| 275|    |-	"play-tracks": function (notification, player)
|    | 275|+	"play-tracks": function(notification, player)
| 276| 276| 	{
| 277| 277| 		if (notification.lock)
| 278| 278| 		{
|    | [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
| 379| 379| 	let notificationText =
| 380| 380| 		notification.instructions.reduce((instructions, item) =>
| 381| 381| 			instructions + (typeof item == "string" ? translate(item) : colorizeHotkey(translate(item.text), item.hotkey)),
| 382|    |-			"");
|    | 382|+		"");
| 383| 383| 
| 384| 384| 	Engine.GetGUIObjectByName("tutorialText").caption = g_TutorialMessages.concat(setStringTags(notificationText, g_TutorialNewMessageTags)).join("\n");
| 385| 385| 	g_TutorialMessages.push(notificationText);
|    | [NORMAL] ESLintBear (no-extra-semi):
|    | Unnecessary semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatSender.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatSender.js
|  57|  57| 		for (let handler of this.chatSubmittedHandlers)
|  58|  58| 			handler();
|  59|  59| 	}
|  60|    |-};
|    |  60|+}

binaries/data/mods/public/gui/session/chat/ChatSender.js
|  60| };
|    | [NORMAL] JSHintBear:
|    | Unnecessary semicolon.
|    | [NORMAL] ESLintBear (no-extra-semi):
|    | Unnecessary semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatMessageFormatPlayer.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatMessageFormatPlayer.js
| 145| 145| 		}
| 146| 146| 		return playerGUID;
| 147| 147| 	}
| 148|    |-};
|    | 148|+}
| 149| 149| 
| 150| 150| /**
| 151| 151|  * Chatmessage shown after commands like /me or /enemies.

binaries/data/mods/public/gui/session/chat/ChatMessageFormatPlayer.js
| 148| };
|    | [NORMAL] JSHintBear:
|    | Unnecessary semicolon.
Executing section cli...

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

elexis edited the summary of this revision. (Show Details)Oct 5 2019, 12:19 PM
Stan added a subscriber: Stan.Oct 5 2019, 12:23 PM
Stan added inline comments.
binaries/data/mods/public/gui/session/chat/ChatAddressees.js
12

Can you use the getter syntax here ? (Like that example P169)

37

Doesn't getSelection handle that last bit ?

82

Why not make that part of the class ?

Adressee look weird, Maybe Recipient ?

binaries/data/mods/public/gui/session/chat/ChatHistory.js
20

Comment on top ?

binaries/data/mods/public/gui/session/chat/ChatMessageFormatNetwork.js
2

This file looks weird. Also would be nice to have JSDOC and @class stuff

binaries/data/mods/public/gui/session/chat/ChatMessageFormatPlayer.js
65

Missing @param with message format.

elexis added inline comments.Oct 5 2019, 12:37 PM
binaries/data/mods/public/gui/session/chat/ChatAddressees.js
12

Can, won't

37

indeed, thanks!

82

It is part of the class

binaries/data/mods/public/gui/session/chat/ChatMessageFormatPlayer.js
65

@param should be used when one wants to comment on a parameter instead of inventing a new custom comment syntax in every file as it used to be. Adding JSdoc comments isnt an ends by itself.

binaries/data/mods/public/gui/session/chat/ChatSender.js
56

(^ this is much cleaner)

elexis updated this revision to Diff 10083.Oct 5 2019, 12:41 PM

Use markForTranslation with chat filters too, fix excess semicolons.

  • TODO: see TODO in the code
  • class descriptions
  • Weird bug: when starting the game and pressing T for the first time, teh chat window is hidden but one can send chat after pressing enter

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

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

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 3 tabs but found 4.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatAddressees.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatAddressees.js
|  38|  38| 
|  39|  39| 		let addressees = this.AddresseeTypes.filter(
|  40|  40| 			addresseeType => addresseeType.isSelectable()).map(
|  41|    |-				addresseeType => ({
|    |  41|+			addresseeType => ({
|  42|  42| 					"label": translateWithContext("chat addressee", addresseeType.label),
|  43|  43| 					"cmd": addresseeType.command
|  44|  44| 				}));
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatAddressees.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatAddressees.js
|  39|  39| 		let addressees = this.AddresseeTypes.filter(
|  40|  40| 			addresseeType => addresseeType.isSelectable()).map(
|  41|  41| 				addresseeType => ({
|  42|    |-					"label": translateWithContext("chat addressee", addresseeType.label),
|    |  42|+				"label": translateWithContext("chat addressee", addresseeType.label),
|  43|  43| 					"cmd": addresseeType.command
|  44|  44| 				}));
|  45|  45| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatAddressees.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatAddressees.js
|  40|  40| 			addresseeType => addresseeType.isSelectable()).map(
|  41|  41| 				addresseeType => ({
|  42|  42| 					"label": translateWithContext("chat addressee", addresseeType.label),
|  43|    |-					"cmd": addresseeType.command
|    |  43|+				"cmd": addresseeType.command
|  44|  44| 				}));
|  45|  45| 
|  46|  46| 		// Add playernames for private messages
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 4.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatAddressees.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/chat/ChatAddressees.js
|  41|  41| 				addresseeType => ({
|  42|  42| 					"label": translateWithContext("chat addressee", addresseeType.label),
|  43|  43| 					"cmd": addresseeType.command
|  44|    |-				}));
|    |  44|+			}));
|  45|  45| 
|  46|  46| 		// Add playernames for private messages
|  47|  47| 		let guids = sortGUIDsByPlayerID();
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 6 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
| 399| 399| 				// Players see colors depending on diplomacy
| 400| 400| 				g_DisplayedPlayerColors[i] =
| 401| 401| 					g_ViewedPlayer == i ? getDiplomacyColor("self") :
| 402|    |-					g_Players[g_ViewedPlayer].isAlly[i] ? getDiplomacyColor("ally") :
|    | 402|+						g_Players[g_ViewedPlayer].isAlly[i] ? getDiplomacyColor("ally") :
| 403| 403| 					g_Players[g_ViewedPlayer].isNeutral[i] ? getDiplomacyColor("neutral") :
| 404| 404| 					getDiplomacyColor("enemy");
| 405| 405| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
| 400| 400| 				g_DisplayedPlayerColors[i] =
| 401| 401| 					g_ViewedPlayer == i ? getDiplomacyColor("self") :
| 402| 402| 					g_Players[g_ViewedPlayer].isAlly[i] ? getDiplomacyColor("ally") :
| 403|    |-					g_Players[g_ViewedPlayer].isNeutral[i] ? getDiplomacyColor("neutral") :
|    | 403|+							g_Players[g_ViewedPlayer].isNeutral[i] ? getDiplomacyColor("neutral") :
| 404| 404| 					getDiplomacyColor("enemy");
| 405| 405| 
| 406| 406| 		g_DisplayedPlayerColors[0] = g_Players[0].color;
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
| 401| 401| 					g_ViewedPlayer == i ? getDiplomacyColor("self") :
| 402| 402| 					g_Players[g_ViewedPlayer].isAlly[i] ? getDiplomacyColor("ally") :
| 403| 403| 					g_Players[g_ViewedPlayer].isNeutral[i] ? getDiplomacyColor("neutral") :
| 404|    |-					getDiplomacyColor("enemy");
|    | 404|+								getDiplomacyColor("enemy");
| 405| 405| 
| 406| 406| 		g_DisplayedPlayerColors[0] = g_Players[0].color;
| 407| 407| 	}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
| 663| 663| 					"civ": setStringTags(g_CivData[g_Players[g_ViewedPlayer].civ].Name, { "font": "sans-bold-stroke-14" }),
| 664| 664| 					"hotkey_civinfo": colorizeHotkey("%(hotkey)s", "civinfo"),
| 665| 665| 					"hotkey_structree": colorizeHotkey("%(hotkey)s", "structree")
| 666|    |-			});
|    | 666|+				});
| 667| 667| 	}
| 668| 668| 
| 669| 669| 	// Following gaia can be interesting on scripted maps
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
|1223|1223| 
|1224|1224| 	let orderHotkeyTooltip = Object.keys(viewablePlayerStates).length <= 1 ? "" :
|1225|1225| 		"\n" + sprintf(translate("%(order)s: %(hotkey)s to change order."), {
|1226|    |-		"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|    |1226|+			"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|1227|1227| 		"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|1228|1228| 	});
|1229|1229| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
|1224|1224| 	let orderHotkeyTooltip = Object.keys(viewablePlayerStates).length <= 1 ? "" :
|1225|1225| 		"\n" + sprintf(translate("%(order)s: %(hotkey)s to change order."), {
|1226|1226| 		"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|1227|    |-		"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|    |1227|+			"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|1228|1228| 	});
|1229|1229| 
|1230|1230| 	let resCodes = g_ResourceData.GetCodes();
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 1.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/session.js
|1225|1225| 		"\n" + sprintf(translate("%(order)s: %(hotkey)s to change order."), {
|1226|1226| 		"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|1227|1227| 		"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|1228|    |-	});
|    |1228|+		});
|1229|1229| 
|1230|1230| 	let resCodes = g_ResourceData.GetCodes();
|1231|1231| 	for (let r = 0; r < resCodes.length; ++r)

binaries/data/mods/public/gui/session/session.js
|1084| »   let·getPanelEntNameTooltip·=·panelEntState·=>·"[font=\"sans-bold-16\"]"·+·template.name.specific·+·"[/font]";
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'panelEntState' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1159| »   »   button.onpress·=·(function(i)·{·return·function()·{·performGroup((Engine.HotkeyIsPressed("selection.add")·?·"add"·:·"select"),·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1160| »   »   button.ondoublepress·=·(function(i)·{·return·function()·{·performGroup("snap",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1161| »   »   button.onpressright·=·(function(i)·{·return·function()·{·performGroup("breakUp",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/menu.js
| 383| »   »   button.onPress·=·(function(player,·stance)·{·return·function()·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'stance' is already declared in the upper scope.

binaries/data/mods/public/gui/session/menu.js
| 415| »   »   button.onPress·=·(function(i,·resCode,·button)·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/menu.js
| 415| »   »   button.onPress·=·(function(i,·resCode,·button)·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'resCode' is already declared in the upper scope.

binaries/data/mods/public/gui/session/menu.js
| 415| »   »   button.onPress·=·(function(i,·resCode,·button)·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'button' is already declared in the upper scope.

binaries/data/mods/public/gui/session/menu.js
| 458| »   button.onPress·=·(function(i)·{·return·function()·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/menu.js
| 524| »   button.onPress·=·(function(i,·button)·{·return·function()·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/menu.js
| 524| »   button.onPress·=·(function(i,·button)·{·return·function()·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'button' is already declared in the upper scope.
|    | [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
| 379| 379| 	let notificationText =
| 380| 380| 		notification.instructions.reduce((instructions, item) =>
| 381| 381| 			instructions + (typeof item == "string" ? translate(item) : colorizeHotkey(translate(item.text), item.hotkey)),
| 382|    |-			"");
|    | 382|+		"");
| 383| 383| 
| 384| 384| 	Engine.GetGUIObjectByName("tutorialText").caption = g_TutorialMessages.concat(setStringTags(notificationText, g_TutorialNewMessageTags)).join("\n");
| 385| 385| 	g_TutorialMessages.push(notificationText);
Executing section cli...

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

elexis added inline comments.Oct 5 2019, 7:05 PM
binaries/data/mods/public/gui/session/chat/ChatAddressees.js
82

Why not make that part of the class ?

It would be nice to define it in the same pair of braces, but that doesnt mean that it's possible without changing the behavior.
If ChatAddressees.prototype.AddresseeTypes is assigned only once when reading the file, then mods can extend it.
If you propose to make it a return value of a getter function (or what else do you mean?), then mods or 0ad code can't extend it from outside.

Adressee look weird, Maybe Recipient ?

weird is subjective, (textbook definitions are not, in case you want to rephrase that).
The name exists since years (including translation context and XML), I dont want to rename it now if I can get away with it.

binaries/data/mods/public/gui/session/messages.js
963

(Re from years ago: .length = 0 may be some picoseconds faster, but = []; reveals to the reader more quickly what the result value of the operation will be. In particular it describes the type (array), whereas it might also be some GUI Object Text type that exposes the length property, or anything else with a length property that isnt an array). And runtime performance is not relevant for this specific function anytime soon, so reading performance seems preferable to me here.)

Stan added inline comments.Oct 5 2019, 7:21 PM
binaries/data/mods/public/gui/session/chat/ChatAddressees.js
82

weird is subjective, (textbook definitions are not, in case you want to rephrase that).

The name exists since years (including translation context and XML), I dont want to rename it now if I can get away with it.

Sure.

If you propose to make it a return value of a getter function (or what else do you mean?), then mods or 0ad code can't extend it from outside.

Well if the goal is to make it easily overridable I guess that's fine. It just seems weird to use the prototype syntax when the patch is about using the class keyword.

So yeah my proposal was to put it in the braces. I assumed mods would be able to override it the same way by defining the prototype in another file.

binaries/data/mods/public/gui/session/chat/ChatMessageFormatPlayer.js
65

Sure I just like it cause I don't have to figure the properties of an object :)

elexis added a comment.Oct 5 2019, 9:35 PM

Next upload includes:

  • add descriptions for the classes. This way gained some clarity and refactored and renamed a bit more.
  • refactored addmessage.
  • fix bug in previous revision (.focus() calls for hidden dialog resulted in chat of a hidden page being sent)
  • fix bug in previous revision of the code (/me) didnt work
  • also focus chat when changing the history (new compared to a23)
  • rename chatmessageformat to chatmessagehandler.
  • move tooltips from session to chatsender too.
  • rename chatsender to chatinput
  • use more bind (thanks Krinkle in https://code.wildfiregames.com/D2240#93108)
elexis added a comment.Oct 6 2019, 4:41 PM

Removed global functions:

-function openChat(command = "")
-function closeChat()
-function initChatWindow()
-function resizeChatWindow()
-function updateChatHistory()
-function onToggleChatWindowExtended()
-function updateChatAddressees()
-function submitChatDirectly(text)
-function submitChatInput()
-function removeOldChatMessage()
-function formatDefeatVictoryMessage(message, players)
-function formatDiplomacyMessage(msg)
-function formatTributeMessage(msg)
-function formatBarterMessage(msg)
-function formatAttackMessage(msg)
-function formatPhaseMessage(msg)
-function formatChatCommand(msg)
-function parseChatAddressee(msg)
-function matchUsername(text)

Added classes:

svn diff | grep "^+class\|= class" | sort
+ChatMessageFormatNetwork.clientlist = class
+ChatMessageFormatNetwork.connect = class
+ChatMessageFormatNetwork.disconnect = class
+ChatMessageFormatNetwork.kicked = class
+ChatMessageFormatNetwork.rejoined = class
+ChatMessageFormatSimulation.attack = class
+ChatMessageFormatSimulation.barter = class
+ChatMessageFormatSimulation.diplomacy = class
+ChatMessageFormatSimulation.phase = class
+ChatMessageFormatSimulation.playerstate = class
+ChatMessageFormatSimulation.tribute = class
+ChatMessageHandler.System = class
+class Chat
+class ChatAddressees
+class ChatHistory
+class ChatInput
+class ChatMessageFormatNetwork
+class ChatMessageFormatPlayer
+class ChatMessageFormatSimulation
+class ChatMessageHandler
+class ChatOverlay
+class ChatWindow

Added local functions

svn diff | grep ^"+" | grep \)$ | grep -v -P '\t\t' | grep -v ^+++ | sort
+	autoComplete()
+	clearChatMessages()
+	closePage()
+	closePage()
+	constructor()
+	constructor()
+	constructor()
+	constructor()
+	constructor()
+	constructor()
+	constructor()
+	constructor()
+	displayChatHistory()
+	getHotkeyTooltip()
+	getSelection()
+	handleMessage(msg)
+	initPage()
+	isExtended()
+	isOpen()
+	matchUsername(text)
+	onChatMessage(msg, chatMessage)
+	onChatMessage(msg, formatted)
+	onSelectionChange()
+	onSelectionChange()
+	onSelectionChange()
+	onSelectionChange(command)
+	onUpdatePlayers()
+	onUpdatePlayers()
+	openPage(command = "")
+	openPage(command)
+	parse()
+	parseMessageAddressee(msg)
+	parseMessage(msg)
+	parse(msg)
+	parse(msg)
+	parse(msg)
+	parse(msg)
+	parse(msg)
+	parse(msg)
+	parse(msg)
+	parse(msg)
+	parse(msg)
+	parse(msg)
+	parse(msg)
+	parse(msg)
+	registerAddresseeTypes(types)
+	registerChatSubmitHandler(handler)
+	registerChatSubmittedHandler(handler)
+	registerMessageFormatClass(formatClass)
+	registerMessageFormat(type, handler)
+	registerMessageHandler(handler)
+	registerSelectionChangeHandler(handler)
+	registerSelectionChangeHandler(handler)
+	removeOldChatMessage()
+	resizeChatWindow()
+	select(command)
+	submitChatInput()
+	submitChat(text, command = "")

Removed global variables:

-var g_ChatTimeout = 30;
-var g_ChatLines = 20;
-var g_ChatMessages = [];
-var g_ChatHistory = [];
-var g_ChatTimers = [];
-var g_LastChatAddressee = "";
-var g_FormatChatMessage = {
-var g_ChatCommands = {
-var g_ChatAddresseeContext = {
-var g_IsChatAddressee = {
-var g_ChatHistoryFilters = [
-var g_DiplomacyMessages = {

Added global variables:

g_Chat = new Chat();
This revision was not accepted when it landed; it landed in state Needs Review.Oct 6 2019, 4:43 PM
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Oct 6 2019, 4:43 PM