[gui] separate top_panel.js file
AbandonedPublic
Actions

Authored by Nescio on Mar 22 2018, 9:48 AM.

Details

Reviewers

bb
elexis

Summary

This patch separates the updateTopPanel() function from the very large session.js file into a new top_panel.js file. The advantage of this is that if a mod has a different set of resources and wants to change the resource bar, the mod no longer has to maintain a full copy of the session.js file, but can edit only the small, new top_panel.js file.

Update: also moves the updateViewedPlayerDropdown function into the new file.

Test Plan

Test if it works as intended.

Diff Detail

Repository

rP 0 A.D. Public Repository

Branch

/ps/trunk

Lint

Lint OK

Unit

No Unit Test Coverage

Build Status

Buildable 5663
Build 9523: Vulcan Build	Jenkins
Build 9522: arc lint + arc unit

Event Timeline

Nescio created this revision.Mar 22 2018, 9:48 AM

Owners added a subscriber: Restricted Owners Package.Mar 22 2018, 9:48 AM

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 (indent):
|    | Expected indentation of 4 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/top_panel.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/top_panel.js
|  17|  17| 					"civ": setStringTags(g_CivData[g_Players[g_ViewedPlayer].civ].Name, { "font": "sans-bold-stroke-14" }),
|  18|  18| 					"hotkey_civinfo": colorizeHotkey("%(hotkey)s", "civinfo"),
|  19|  19| 					"hotkey_structree": colorizeHotkey("%(hotkey)s", "structree")
|  20|    |-			});
|    |  20|+				});
|  21|  21| 	}
|  22|  22| 
|  23|  23| 	Engine.GetGUIObjectByName("optionFollowPlayer").hidden = !g_IsObserver ||
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 6 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
| 412| 412| 				// Players see colors depending on diplomacy
| 413| 413| 				g_DisplayedPlayerColors[i] =
| 414| 414| 					g_ViewedPlayer == i ? getDiplomacyColor("self") :
| 415|    |-					g_Players[g_ViewedPlayer].isAlly[i] ? getDiplomacyColor("ally") :
|    | 415|+						g_Players[g_ViewedPlayer].isAlly[i] ? getDiplomacyColor("ally") :
| 416| 416| 					g_Players[g_ViewedPlayer].isNeutral[i] ? getDiplomacyColor("neutral") :
| 417| 417| 					getDiplomacyColor("enemy");
| 418| 418| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
| 413| 413| 				g_DisplayedPlayerColors[i] =
| 414| 414| 					g_ViewedPlayer == i ? getDiplomacyColor("self") :
| 415| 415| 					g_Players[g_ViewedPlayer].isAlly[i] ? getDiplomacyColor("ally") :
| 416|    |-					g_Players[g_ViewedPlayer].isNeutral[i] ? getDiplomacyColor("neutral") :
|    | 416|+							g_Players[g_ViewedPlayer].isNeutral[i] ? getDiplomacyColor("neutral") :
| 417| 417| 					getDiplomacyColor("enemy");
| 418| 418| 
| 419| 419| 		g_DisplayedPlayerColors[0] = g_Players[0].color;
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
| 414| 414| 					g_ViewedPlayer == i ? getDiplomacyColor("self") :
| 415| 415| 					g_Players[g_ViewedPlayer].isAlly[i] ? getDiplomacyColor("ally") :
| 416| 416| 					g_Players[g_ViewedPlayer].isNeutral[i] ? getDiplomacyColor("neutral") :
| 417|    |-					getDiplomacyColor("enemy");
|    | 417|+								getDiplomacyColor("enemy");
| 418| 418| 
| 419| 419| 		g_DisplayedPlayerColors[0] = g_Players[0].color;
| 420| 420| 	}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|1183|1183| 
|1184|1184| 	let orderHotkeyTooltip = Object.keys(viewablePlayerStates).length <= 1 ? "" :
|1185|1185| 		"\n" + sprintf(translate("%(order)s: %(hotkey)s to change order."), {
|1186|    |-		"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|    |1186|+			"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|1187|1187| 		"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|1188|1188| 	});
|1189|1189| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|1184|1184| 	let orderHotkeyTooltip = Object.keys(viewablePlayerStates).length <= 1 ? "" :
|1185|1185| 		"\n" + sprintf(translate("%(order)s: %(hotkey)s to change order."), {
|1186|1186| 		"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|1187|    |-		"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|    |1187|+			"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|1188|1188| 	});
|1189|1189| 
|1190|1190| 	let resCodes = g_ResourceData.GetCodes();
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 1.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|1185|1185| 		"\n" + sprintf(translate("%(order)s: %(hotkey)s to change order."), {
|1186|1186| 		"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|1187|1187| 		"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|1188|    |-	});
|    |1188|+		});
|1189|1189| 
|1190|1190| 	let resCodes = g_ResourceData.GetCodes();
|1191|1191| 	for (let r = 0; r < resCodes.length; ++r)
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|1683|1683| 	for (let rct of resourcesCounterTypes)
|1684|1684| 		for (let rt of resourcesTypes)
|1685|1685| 			reportObject[rt + rct.substr(9)] = playerStatistics[rct][rt];
|1686|    |-			// eg. rt = food rct.substr = Gathered rct = resourcesGathered
|    |1686|+	// eg. rt = food rct.substr = Gathered rct = resourcesGathered
|1687|1687| 
|1688|1688| 	reportObject.vegetarianFoodGathered = playerStatistics.resourcesGathered.vegetarianFood;
|1689|1689| 	for (let type of unitsClasses)

binaries/data/mods/public/gui/session/session.js
|1011| »   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
|1086| »   »   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
|1087| »   »   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
|1088| »   »   button.onpressright·=·(function(i)·{·return·function()·{·performGroup("breakUp",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

Link to build: https://jenkins.wildfiregames.com/job/differential/300/display/redirect

Harbormaster completed remote builds in B5661: Diff 6254.Mar 22 2018, 9:55 AM

Nescio mentioned this in D1324: [gui] vertically space objects and separate top_panel.js file.Mar 22 2018, 9:56 AM

Also moved the updateViewedPlayerDropdown function into the new file (per @bb ).

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 (indent):
|    | Expected indentation of 4 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/top_panel.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/top_panel.js
|  17|  17| 					"civ": setStringTags(g_CivData[g_Players[g_ViewedPlayer].civ].Name, { "font": "sans-bold-stroke-14" }),
|  18|  18| 					"hotkey_civinfo": colorizeHotkey("%(hotkey)s", "civinfo"),
|  19|  19| 					"hotkey_structree": colorizeHotkey("%(hotkey)s", "structree")
|  20|    |-			});
|    |  20|+				});
|  21|  21| 	}
|  22|  22| 
|  23|  23| 	Engine.GetGUIObjectByName("optionFollowPlayer").hidden = !g_IsObserver ||
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 6 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
| 412| 412| 				// Players see colors depending on diplomacy
| 413| 413| 				g_DisplayedPlayerColors[i] =
| 414| 414| 					g_ViewedPlayer == i ? getDiplomacyColor("self") :
| 415|    |-					g_Players[g_ViewedPlayer].isAlly[i] ? getDiplomacyColor("ally") :
|    | 415|+						g_Players[g_ViewedPlayer].isAlly[i] ? getDiplomacyColor("ally") :
| 416| 416| 					g_Players[g_ViewedPlayer].isNeutral[i] ? getDiplomacyColor("neutral") :
| 417| 417| 					getDiplomacyColor("enemy");
| 418| 418| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
| 413| 413| 				g_DisplayedPlayerColors[i] =
| 414| 414| 					g_ViewedPlayer == i ? getDiplomacyColor("self") :
| 415| 415| 					g_Players[g_ViewedPlayer].isAlly[i] ? getDiplomacyColor("ally") :
| 416|    |-					g_Players[g_ViewedPlayer].isNeutral[i] ? getDiplomacyColor("neutral") :
|    | 416|+							g_Players[g_ViewedPlayer].isNeutral[i] ? getDiplomacyColor("neutral") :
| 417| 417| 					getDiplomacyColor("enemy");
| 418| 418| 
| 419| 419| 		g_DisplayedPlayerColors[0] = g_Players[0].color;
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
| 414| 414| 					g_ViewedPlayer == i ? getDiplomacyColor("self") :
| 415| 415| 					g_Players[g_ViewedPlayer].isAlly[i] ? getDiplomacyColor("ally") :
| 416| 416| 					g_Players[g_ViewedPlayer].isNeutral[i] ? getDiplomacyColor("neutral") :
| 417|    |-					getDiplomacyColor("enemy");
|    | 417|+								getDiplomacyColor("enemy");
| 418| 418| 
| 419| 419| 		g_DisplayedPlayerColors[0] = g_Players[0].color;
| 420| 420| 	}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|1174|1174| 
|1175|1175| 	let orderHotkeyTooltip = Object.keys(viewablePlayerStates).length <= 1 ? "" :
|1176|1176| 		"\n" + sprintf(translate("%(order)s: %(hotkey)s to change order."), {
|1177|    |-		"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|    |1177|+			"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|1178|1178| 		"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|1179|1179| 	});
|1180|1180| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|1175|1175| 	let orderHotkeyTooltip = Object.keys(viewablePlayerStates).length <= 1 ? "" :
|1176|1176| 		"\n" + sprintf(translate("%(order)s: %(hotkey)s to change order."), {
|1177|1177| 		"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|1178|    |-		"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|    |1178|+			"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|1179|1179| 	});
|1180|1180| 
|1181|1181| 	let resCodes = g_ResourceData.GetCodes();
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 1.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|1176|1176| 		"\n" + sprintf(translate("%(order)s: %(hotkey)s to change order."), {
|1177|1177| 		"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|1178|1178| 		"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|1179|    |-	});
|    |1179|+		});
|1180|1180| 
|1181|1181| 	let resCodes = g_ResourceData.GetCodes();
|1182|1182| 	for (let r = 0; r < resCodes.length; ++r)
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|1674|1674| 	for (let rct of resourcesCounterTypes)
|1675|1675| 		for (let rt of resourcesTypes)
|1676|1676| 			reportObject[rt + rct.substr(9)] = playerStatistics[rct][rt];
|1677|    |-			// eg. rt = food rct.substr = Gathered rct = resourcesGathered
|    |1677|+	// eg. rt = food rct.substr = Gathered rct = resourcesGathered
|1678|1678| 
|1679|1679| 	reportObject.vegetarianFoodGathered = playerStatistics.resourcesGathered.vegetarianFood;
|1680|1680| 	for (let type of unitsClasses)

binaries/data/mods/public/gui/session/session.js
|1002| »   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
|1077| »   »   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
|1078| »   »   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
|1079| »   »   button.onpressright·=·(function(i)·{·return·function()·{·performGroup("breakUp",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

Link to build: https://jenkins.wildfiregames.com/job/differential/302/display/redirect

Harbormaster completed remote builds in B5663: Diff 6256.Mar 22 2018, 10:12 AM

Agreeing on the change, testing says front doesn't fall => accept
(will commit after release as we move some translated strings around)

binaries/data/mods/public/gui/session/top_panel.js
35–36	seems to be added intentional in rP18964, whatever meh

This revision is now accepted and ready to land.Mar 22 2018, 11:41 AM

No, not for 75 lines.

Is this still good or has something changed in the past nine months?

Nothing changed (as in me still being against this diff).

The only argument I see for this is to be able to mod more comfortably without overwriting entire session.js, but that already is possible by replacing the function (nanis does that for his mods for instance, fcxSanya did it also for another mod).
So if that's the only argument, we don't need to do anything to achieve what we want to achieve.

I find this ugly because it only considers 70 of the 1700 lines and we already had in history tiny files that were not split logically well, for instance rP19238 .
The idea was to not keep many files around if they are logically very similar.
If that argument to move this to a new small file would be correct and sufficient to move this to a tiny new file, then all functions would should be considered to be separated, not only one.

So requesting either an abandoning of this diff or there should be some greater plan to refactor the entire session GUI Code (I suspect it will be much cleaner when split into many objects/managers, 200-300 lines of code each, each in one file, OOP, #5322).

(In particular it's a a bit troublesome if there are many globals scattered across multiple files. (Though in this file there are only 3 or 4 or so used, so that's not so bad for only these instances, but increases with every file split, but I guess that will be inevitable).)
(It would be nicer to remove the old code and put the new code into the ideal way into the new places, rather than 1:1 moving, although I recon that 1:1 moves don't introduce bugs)

This revision now requires changes to proceed.Dec 28 2018, 9:09 PM

Thank you for your clarification. I think I now understand what you mean.

Perhaps @nani can post you a link to a mod with such an examplary function-replacement if there is some desire for it.
The examples mod that contains a bunch of... examples as described in #5366, could also receive an example how to insert such a proxy function.

@elexis You mean creating a new file, loaded after session.js, that overwrites the function?
If so, I would agree with you, however that seems like it depends on the loading order of files, which is dangerous.

The loading order is deterministic / alphabetic, so one can use session_changes.js to modify session.js as the shortest path.
Notice the replacing function can also call the replaced function, so this would also add compatibility for multiple mods to modify the same file. (How are mods going to be useful if you can only launch one at a time)

You can see an example at the gamesetup_*.js file at https://wildfiregames.com/forum/index.php?/topic/24649-autociv-mod-less-clicks-more-civs/

Feels like killing puppies :-/

I think the right approach is to educate modders how to mod functions without copying the entire file.
Something for the wiki ModdingPage, word of mouth propagation and a possible pyrogenesis-examples mod.

Was this diff done for any specific mod that wants to change this specific function? Then we can inform that modder and perhaps provide a patch, or just the link to nanis example.