Page MenuHomeWildfire Games

Fix lobby chat performance issue.
Needs ReviewPublic

Authored by wraitii on Feb 23 2019, 4:18 AM.

Details

Reviewers
vladislavbelov
nani
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Owners Package(Owns No Changed Paths)
Summary

New chat text is added as a copy of all previous chat and new one, use CList so it is not. Also adds a handy JS Interface addItem method to add new comments efficiently.
With this fix you can add a new message every onTick for 5000 ticks and not notice any performance hit (not like the current one that at 500 lines starts to drop fps).


Note that only List gets the additional JS function, even though COList inherits from it, because COList doesn't implement the clever addText setup.

Test Plan

Check text displays correctly when added.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
elexis added a comment.Mar 6 2019, 8:15 PM

Isn't it possible to edit CText.cpp and CInput.cpp like you did with CList.cpp here, without messing with the text owner class at all? An addText or appendLine function in these GUI objects that just adds to m_GeneratedTexts and the caption string without rebuilding the existing code.

nani added a comment.Mar 6 2019, 8:26 PM

Technically yes but I'm not sure it won't break something else (like the charts).

elexis added a comment.Mar 6 2019, 8:48 PM

CChart doesn't inherit anything from CText or CInput, does it? And if we can't tell for CText or CInput whether something might break somewhere, how can we tell for CList?

nani added a comment.Mar 6 2019, 8:59 PM

Not sure. But does look deceptively easy.

nani added a comment.Mar 6 2019, 9:01 PM

Still, it doesn't solve the problem of bloating the base object if we want to make a JS interface for each gui type.

elexis added a comment.Mar 6 2019, 9:37 PM

It'd probably be the cleanest to have one small JSInterface_GUIObjectType class that contains the functions and properties specific to that GUI type, while inheriting or calling a JSInterface_GUIObject with the functions and properties common to all GUI Object types (as mentioned in D1781#72323).
If it's done before, that would leave clean insertions of the proposed new JSInterface GUIObject function/s, but I suppose it can also be done afterwards (never) if it's not a requirement for the CList diff to straighten that. (That commit would also be accompanied by other commits IIRC.)

Still, it doesn't solve the problem of bloating the base object if we want to make a JS interface for each gui type.

Otherwise not sure what base object actually you're refering to.

Also shouldn't forget to check

not regenerating the text if the caption is still the same as before. Might cut 90% of the time if that isn't already the case.

I guess that's what you refered to when not being sure about the consequences of changing the IGUITextOwner? (Since that's the place where it checks for caption property changes).
It looks like it currently regenerates the caption even if it didn't change, and would probably cut a lot of performance in some of the mentioned cases. (And another Gee TODO from 2004 running gag there.)
The consequences of not regenerating the texts might be foreseeable for all child classes and one might conclude whether it's good or not to do so. I expect that all GUI Object types that inherit the TextOwner are expected to keep the caption property in sync with the generated texts, thus it would be harmless and useful with the current code, and a code where CInput and CText receive an AddLine function that extend the caption property and add an item to the m_GeneratedTexts vector.

Stan added a subscriber: Stan.Mar 7 2019, 11:42 AM
Stan added inline comments.
source/gui/CList.cpp
163 ↗(On Diff #7498)

Still accurate ?

440 ↗(On Diff #7498)

Any way to make a helper function with the one below ?

481 ↗(On Diff #7498)

Might want to add doxygen here as well.

source/gui/CList.h
46 ↗(On Diff #7498)

C++ uses Doxygen, not JSDoc, so I'm not sure this works.

https://stackoverflow.com/questions/11991616/use-of-see-or-link-in-doxygen

In case it doesn't, you should add @param

source/gui/IGUIObject.h
133 ↗(On Diff #7498)

@paramsmissing :)

135 ↗(On Diff #7498)

Typo in warning.

source/gui/scripting/JSInterface_IGUIObject.cpp
756 ↗(On Diff #7498)

Add one ;) If you are using vscode, you can configure it to do it for you :)

(See rP22134#32587 on the JSInterface question.)

source/gui/scripting/JSInterface_IGUIObject.cpp
728 ↗(On Diff #7498)
wraitii commandeered this revision.May 28 2019, 10:57 PM
wraitii added a reviewer: nani.
wraitii updated this revision to Diff 8195.EditedMay 28 2019, 11:00 PM
wraitii added a subscriber: Itms.

(This was far harder than it should have been. I feel like a complete refactor of these classes is necessary, but it's a bit out of scope here).

There's a fun little comment in SetupText() from 2006 in CList.cpp:

	// Delete all generated texts. Some could probably be saved,
	//  but this is easier, and this function will never be called
	//  continuously, or even often, so it'll probably be okay.

Turns out... It wasn't.

Indeed regenerating the text every time we append can get slow. Sadly, CText handles text as one big SGUIText and not breaking on new lines, so it's fairly difficult to extend to support appending.

As it turns out, CList is barely better - I went with something that minimises the code duplication, but this really should be refactored entirely. COList is even more annoying with the columns - I've given up.

To make @elexis happy, this only adds "addItem" on the list objects. I think ultimately we could refactor more of the GUI like that.

@Itms , also interested in your opinion of this JS-GUIObject extension

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

Linter detected issues:
Executing section Source...

source/gui/CList.h
|   1| /*·Copyright·(C)·2017·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2017"

source/gui/CList.cpp
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"
Executing section JS...
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|  47|  47|  * The playerlist will be assembled using these values.
|  48|  48|  */
|  49|  49| var g_PlayerStatuses = {
|  50|    |-	"available": { "color": "0 219 0",     "status": translate("Online") },
|    |  50|+	"available": { "color": "0 219 0", "status": translate("Online") },
|  51|  51| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  52|  52| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  53|  53| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space before value for key 'away'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|  48|  48|  */
|  49|  49| var g_PlayerStatuses = {
|  50|  50| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  51|    |-	"away":      { "color": "229 76 13",   "status": translate("Away") },
|    |  51|+	"away": { "color": "229 76 13",   "status": translate("Away") },
|  52|  52| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  53|  53| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  54|  54| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|  48|  48|  */
|  49|  49| var g_PlayerStatuses = {
|  50|  50| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  51|    |-	"away":      { "color": "229 76 13",   "status": translate("Away") },
|    |  51|+	"away":      { "color": "229 76 13", "status": translate("Away") },
|  52|  52| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  53|  53| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  54|  54| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space before value for key 'playing'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|  49|  49| var g_PlayerStatuses = {
|  50|  50| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  51|  51| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  52|    |-	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|    |  52|+	"playing": { "color": "200 0 0",     "status": translate("Busy") },
|  53|  53| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  54|  54| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  55|  55| };
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|  49|  49| var g_PlayerStatuses = {
|  50|  50| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  51|  51| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  52|    |-	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|    |  52|+	"playing":   { "color": "200 0 0", "status": translate("Busy") },
|  53|  53| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  54|  54| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  55|  55| };
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space before value for key 'offline'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|  50|  50| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  51|  51| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  52|  52| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  53|    |-	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|    |  53|+	"offline": { "color": "0 0 0",       "status": translate("Offline") },
|  54|  54| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  55|  55| };
|  56|  56| 
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|  50|  50| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  51|  51| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  52|  52| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  53|    |-	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|    |  53|+	"offline":   { "color": "0 0 0", "status": translate("Offline") },
|  54|  54| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  55|  55| };
|  56|  56| 
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space before value for key 'unknown'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|  51|  51| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  52|  52| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  53|  53| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  54|    |-	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|    |  54|+	"unknown": { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  55|  55| };
|  56|  56| 
|  57|  57| var g_RoleNames = {
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 5 tabs but found 4.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
| 219| 219| 					me ?
| 220| 220| 						translate("You have been muted.") :
| 221| 221| 						translate("%(nick)s has been muted.") :
| 222|    |-				newrole == "moderator" ?
|    | 222|+					newrole == "moderator" ?
| 223| 223| 					me ?
| 224| 224| 						translate("You are now a moderator.") :
| 225| 225| 						translate("%(nick)s is now a moderator.") :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 6 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
| 220| 220| 						translate("You have been muted.") :
| 221| 221| 						translate("%(nick)s has been muted.") :
| 222| 222| 				newrole == "moderator" ?
| 223|    |-					me ?
|    | 223|+						me ?
| 224| 224| 						translate("You are now a moderator.") :
| 225| 225| 						translate("%(nick)s is now a moderator.") :
| 226| 226| 				msg.oldrole == "visitor" ?
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 6.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
| 221| 221| 						translate("%(nick)s has been muted.") :
| 222| 222| 				newrole == "moderator" ?
| 223| 223| 					me ?
| 224|    |-						translate("You are now a moderator.") :
|    | 224|+							translate("You are now a moderator.") :
| 225| 225| 						translate("%(nick)s is now a moderator.") :
| 226| 226| 				msg.oldrole == "visitor" ?
| 227| 227| 					me ?
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 6.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
| 222| 222| 				newrole == "moderator" ?
| 223| 223| 					me ?
| 224| 224| 						translate("You are now a moderator.") :
| 225|    |-						translate("%(nick)s is now a moderator.") :
|    | 225|+							translate("%(nick)s is now a moderator.") :
| 226| 226| 				msg.oldrole == "visitor" ?
| 227| 227| 					me ?
| 228| 228| 						translate("You have been unmuted.") :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 6 tabs but found 4.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
| 223| 223| 					me ?
| 224| 224| 						translate("You are now a moderator.") :
| 225| 225| 						translate("%(nick)s is now a moderator.") :
| 226|    |-				msg.oldrole == "visitor" ?
|    | 226|+						msg.oldrole == "visitor" ?
| 227| 227| 					me ?
| 228| 228| 						translate("You have been unmuted.") :
| 229| 229| 						translate("%(nick)s has been unmuted.") :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
| 224| 224| 						translate("You are now a moderator.") :
| 225| 225| 						translate("%(nick)s is now a moderator.") :
| 226| 226| 				msg.oldrole == "visitor" ?
| 227|    |-					me ?
|    | 227|+							me ?
| 228| 228| 						translate("You have been unmuted.") :
| 229| 229| 						translate("%(nick)s has been unmuted.") :
| 230| 230| 					me ?
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 6.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
| 225| 225| 						translate("%(nick)s is now a moderator.") :
| 226| 226| 				msg.oldrole == "visitor" ?
| 227| 227| 					me ?
| 228|    |-						translate("You have been unmuted.") :
|    | 228|+								translate("You have been unmuted.") :
| 229| 229| 						translate("%(nick)s has been unmuted.") :
| 230| 230| 					me ?
| 231| 231| 						translate("You are not a moderator anymore.") :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 6.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
| 226| 226| 				msg.oldrole == "visitor" ?
| 227| 227| 					me ?
| 228| 228| 						translate("You have been unmuted.") :
| 229|    |-						translate("%(nick)s has been unmuted.") :
|    | 229|+								translate("%(nick)s has been unmuted.") :
| 230| 230| 					me ?
| 231| 231| 						translate("You are not a moderator anymore.") :
| 232| 232| 						translate("%(nick)s is not a moderator anymore.");
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
| 227| 227| 					me ?
| 228| 228| 						translate("You have been unmuted.") :
| 229| 229| 						translate("%(nick)s has been unmuted.") :
| 230|    |-					me ?
|    | 230|+							me ?
| 231| 231| 						translate("You are not a moderator anymore.") :
| 232| 232| 						translate("%(nick)s is not a moderator anymore.");
| 233| 233| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 6.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
| 228| 228| 						translate("You have been unmuted.") :
| 229| 229| 						translate("%(nick)s has been unmuted.") :
| 230| 230| 					me ?
| 231|    |-						translate("You are not a moderator anymore.") :
|    | 231|+								translate("You are not a moderator anymore.") :
| 232| 232| 						translate("%(nick)s is not a moderator anymore.");
| 233| 233| 
| 234| 234| 			addChatMessage({
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 6.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
| 229| 229| 						translate("%(nick)s has been unmuted.") :
| 230| 230| 					me ?
| 231| 231| 						translate("You are not a moderator anymore.") :
| 232|    |-						translate("%(nick)s is not a moderator anymore.");
|    | 232|+								translate("%(nick)s is not a moderator anymore.");
| 233| 233| 
| 234| 234| 			addChatMessage({
| 235| 235| 				"text": "/special " + sprintf(txt, { "nick": msg.nick }),

binaries/data/mods/public/gui/lobby/lobby.js
|1035| »   »   switch·(sortBy)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/lobby/lobby.js
| 729| »   »   case·'name':
|    | [NORMAL] JSHintBear:
|    | Expected a 'break' statement before 'default'.
|    | [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 (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
|1240|1240| 			offset = -Math.min(slideSpeed * dt, maxOffset);
|1241|1241| 	}
|1242|1242| 
|1243|    |-	updateSettingsPanelPosition(offset);	
|    |1243|+	updateSettingsPanelPosition(offset);
|1244|1244| }
|1245|1245| 
|1246|1246| /**
|    | [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
|1774|1774| 	let biomeList;
|1775|1775| 
|1776|1776| 	if (g_GameAttributes.mapType == "random" && g_GameAttributes.settings.SupportedBiomes)
|1777|    |-	{
|    |1777|+	
|1778|1778| 		if (typeof g_GameAttributes.settings.SupportedBiomes == "string")
|1779|1779| 			biomeList = g_Settings.Biomes.filter(biome => biome.Id.startsWith(g_GameAttributes.settings.SupportedBiomes));
|1780|1780| 		else
|1781|1781| 			biomeList = g_Settings.Biomes.filter(
|1782|1782| 				biome => g_GameAttributes.settings.SupportedBiomes.indexOf(biome.Id) != -1);
|1783|    |-	}
|    |1783|+	
|1784|1784| 
|1785|1785| 	g_BiomeList = biomeList && prepareForDropdown(
|1786|1786| 		[{

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|2018| »   while·(g_IsNetworked)
|    | [NORMAL] ESLintBear (no-unmodified-loop-condition):
|    | 'g_IsNetworked' is not modified in this loop.
Executing section cli...

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

wraitii added inline comments.May 29 2019, 9:41 AM
source/gui/CList.cpp
79 ↗(On Diff #8195)

The dynamic_cast is required because we are down casting from a virtual base.

It could be skipped if IGuiObject implemented a virtual "GetThis" method, but that's not much better than dynamic_cast imo.

Alternaties include storing a more specific pointer in a reserved slot and refactoring this entirely.

source/gui/CList.cpp
79 ↗(On Diff #8195)

I disagree, dynamic_cast is worse: slower (~2 times slower on some machines) and less logic safe. Also it's forbidden by CC. dynamic_cast means bad design in that case, because we already have a type of the object on its creation.

Simple virtual method is just faster.

CList* CList::AsCList() override { return this; }
85 ↗(On Diff #8195)

Duplication of code. It can be written clearer.

wraitii marked an inline comment as not done.May 29 2019, 11:12 AM
wraitii added inline comments.
source/gui/CList.cpp
79 ↗(On Diff #8195)

The ugliness is that we have to add AsCList to the IGUIObject's interface.

I'm working on an alternative variant using proxy objects , but tbh it remains kinda ugly so far.

vladislavbelov added inline comments.May 29 2019, 7:55 PM
source/gui/CList.cpp
79 ↗(On Diff #8195)

I don't want to say that virtual functions are best. But imo they're better than dynamic_cast. Also you could use IGUIObject type (probably convert it to an enum value) and static_cast inside a switch.

But I have an idea, how it could be done without much work (I'm not sure, but still).

We create a GUI object definition once, so we can do the same stuff for derived classes using prototypes.

// How we define a class:
JS_InitClass(
	m->m_cx, global, JS::NullPtr(), // <- This nullptr is a parent prototype, like a "base" class.
	clasp, constructor, minArgs, ps, fs, static_ps, static_fs)

We create a GUI object in 2 places:

JSObject* IGUIObject::GetJSObject()
{
	// ...
	m_JSObject.init(cx, m_pGUI->GetScriptInterface()->CreateCustomObject("GUIObject"));
	// ...
}

bool JSI_IGUIObject::construct(JSContext* cx, uint argc, JS::Value* vp)
{
	// ...
	JS::RootedObject obj(cx, pScriptInterface->CreateCustomObject("GUIObject"));
	// ...
}

The first method can be easily modified to use the one virtual method to override a JS class name for derived classes. I didn't look much, but I think the second is solvable too.

source/gui/CList.cpp
79 ↗(On Diff #8195)

The second one you even fixes in D1933.

wraitii added inline comments.Jun 6 2019, 8:06 PM
source/gui/CList.cpp
79 ↗(On Diff #8195)

You mean creating "GUiOBject", then "GuiListObject" for example? The problem with that is that is assumes single inheritance, which isn't the case right now.

I haven't understood your enum comment though.

Stan added inline comments.Jun 6 2019, 8:17 PM
source/gui/CList.cpp
495 ↗(On Diff #8195)

Early return maybe ?

vladislavbelov added inline comments.Jun 7 2019, 8:43 PM
source/gui/CList.cpp
79 ↗(On Diff #8195)

You mean creating "GUiOBject", then "GuiListObject" for example?

Yes, with different functions, the first one is the prototype for the second one.

The problem with that is that is assumes single inheritance, which isn't the case right now.

It's not a problem I think. Most of our multiple inheritances are technical details and can be skipped in the JS interface.

For ex. you may skip some classes in a tree and combine the final object with inherited functions at run time (or even compile time with templates). We don't need to cast different objects in JS, so we can use it.

Like:


class JSObjectCreator {

class A : public B, public C
{
  static Functions GetFunctions()
  {
    return {
      FunctionsOfA(),
      B::GetFunctions(),
      C::GetFunctions()
    };
  }

  static void Create()
  {
    JS_InitClass(
      m->m_cx, global, JS::NullPtr(),
      clasp, constructor, minArgs, ps, GetFunctions(), static_ps, static_fs);
  }
};

I haven't understood your enum comment though.

Usual explicit cast:

enum class GUIObjectType
{
  CHART,
  INPUT,
  // ...
}

class IGUIObject
{
  virtual GUIObjectType GetType() = 0;
}

template<typename T>
T* AsGUIObject(IGUIObject* object)
{
  if (object->GetType != T::Type())
    return nullptr;
  return static_cast<T*>(object);
}

void DoSomething(IGUIObject* object)
{
  switch (object->GetType)
  {
  case GUIObjectType::INPUT: /* ... */ break;
  }
}
elexis added a comment.Jun 7 2019, 9:16 PM

To make @elexis happy

I appreciate it, but it's really in every code users interest to have this file split well, as there are already 3 random functions that are uncommon for JSInterface GUI Objects but defined here (committed or not).
Ticket was created at #5442.

The pattern vladislav posted looks like it separates the concerns (common and specific GUIObject methods) well.
wraitiis version does that too, but splits it at the costs of removing the split of the JSInterface from the GUIObject.

I'm not sure if we need a JSObjectCreator wrapper around class A : public B, public C, as I guess there would be more than class A (we have three use cases for JSInterface functions relating to specific GUI object types).

Also I guess it's better than before, but I still think that you can accomplish the same with CText and an append method, as said. This way the text can become selectable, as CList probably not.

(Another considerable feature was redacting chat messages by moderators after they were posted by sending a redaction chat message, but that's far fetched, and it's rare enough that one can rebuild the chatlog)

source/gui/CList.cpp
77 ↗(On Diff #8195)
124 ↗(On Diff #8195)

This is code that really sounds like it should be in a JSInterface_*GUI*List* file, it's the only one operating on a JS context and, it defacto is the JS Interface.

source/gui/scripting/JSInterface_IGUIObject.cpp
82 ↗(On Diff #8195)

(This idea of #5442 is that this file becomes agnostic of specific GUIObject properties, so ideally this reference (and getTextSize) would become removed here as well.)

wraitiis version does that too, but splits it at the costs of removing the split of the JSInterface from the GUIObject.

TBH I found myself wondering how useful that split is. Separating things in another file wouldn't be too difficult I suppose.

The actual code to enable appending on lists or text is difficult to write because the whole class isn't designed for it, so I suggest we fix this in a somewhat 'quick' way and then come back and clean it up, possibly.

source/gui/CList.cpp
79 ↗(On Diff #8195)

Might be the best we'll get indeed.

source/gui/scripting/JSInterface_IGUIObject.cpp
82 ↗(On Diff #8195)

That's unfortunately impossible/annoying to do while we still override the "get" property. That needs to be changed since SM5X+ won't support it anymore, but until then the easiest is a line there.

wraitii updated this revision to Diff 8426.EditedJun 10 2019, 9:38 PM

Cleaner diff, without dynamic_cast.

I went with a virtual method returning a pointer to void*. It looks ugly, but it kind of is (as discussed before), and using an enum would require changing a common file every time we add a new type - which seems annoying and slow to compile to me.

(also moves the function to its own scripting file).

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

Linter detected issues:
Executing section Source...

source/gui/CList.cpp
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"

source/gui/IGUIObject.cpp
|   1| /*·Copyright·(C)·2016·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2016"

source/gui/CList.h
|   1| /*·Copyright·(C)·2017·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2017"
Executing section JS...
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|  47|  47|  * The playerlist will be assembled using these values.
|  48|  48|  */
|  49|  49| var g_PlayerStatuses = {
|  50|    |-	"available": { "color": "0 219 0",     "status": translate("Online") },
|    |  50|+	"available": { "color": "0 219 0", "status": translate("Online") },
|  51|  51| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  52|  52| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  53|  53| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space before value for key 'away'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|  48|  48|  */
|  49|  49| var g_PlayerStatuses = {
|  50|  50| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  51|    |-	"away":      { "color": "229 76 13",   "status": translate("Away") },
|    |  51|+	"away": { "color": "229 76 13",   "status": translate("Away") },
|  52|  52| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  53|  53| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  54|  54| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|  48|  48|  */
|  49|  49| var g_PlayerStatuses = {
|  50|  50| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  51|    |-	"away":      { "color": "229 76 13",   "status": translate("Away") },
|    |  51|+	"away":      { "color": "229 76 13", "status": translate("Away") },
|  52|  52| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  53|  53| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  54|  54| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space before value for key 'playing'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|  49|  49| var g_PlayerStatuses = {
|  50|  50| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  51|  51| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  52|    |-	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|    |  52|+	"playing": { "color": "200 0 0",     "status": translate("Busy") },
|  53|  53| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  54|  54| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  55|  55| };
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|  49|  49| var g_PlayerStatuses = {
|  50|  50| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  51|  51| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  52|    |-	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|    |  52|+	"playing":   { "color": "200 0 0", "status": translate("Busy") },
|  53|  53| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  54|  54| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  55|  55| };
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space before value for key 'offline'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|  50|  50| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  51|  51| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  52|  52| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  53|    |-	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|    |  53|+	"offline": { "color": "0 0 0",       "status": translate("Offline") },
|  54|  54| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  55|  55| };
|  56|  56| 
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|  50|  50| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  51|  51| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  52|  52| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  53|    |-	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|    |  53|+	"offline":   { "color": "0 0 0", "status": translate("Offline") },
|  54|  54| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  55|  55| };
|  56|  56| 
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space before value for key 'unknown'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|  51|  51| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  52|  52| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  53|  53| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  54|    |-	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|    |  54|+	"unknown": { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  55|  55| };
|  56|  56| 
|  57|  57| var g_RoleNames = {
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 5 tabs but found 4.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
| 219| 219| 					me ?
| 220| 220| 						translate("You have been muted.") :
| 221| 221| 						translate("%(nick)s has been muted.") :
| 222|    |-				newrole == "moderator" ?
|    | 222|+					newrole == "moderator" ?
| 223| 223| 					me ?
| 224| 224| 						translate("You are now a moderator.") :
| 225| 225| 						translate("%(nick)s is now a moderator.") :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 6 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
| 220| 220| 						translate("You have been muted.") :
| 221| 221| 						translate("%(nick)s has been muted.") :
| 222| 222| 				newrole == "moderator" ?
| 223|    |-					me ?
|    | 223|+						me ?
| 224| 224| 						translate("You are now a moderator.") :
| 225| 225| 						translate("%(nick)s is now a moderator.") :
| 226| 226| 				msg.oldrole == "visitor" ?
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 6.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
| 221| 221| 						translate("%(nick)s has been muted.") :
| 222| 222| 				newrole == "moderator" ?
| 223| 223| 					me ?
| 224|    |-						translate("You are now a moderator.") :
|    | 224|+							translate("You are now a moderator.") :
| 225| 225| 						translate("%(nick)s is now a moderator.") :
| 226| 226| 				msg.oldrole == "visitor" ?
| 227| 227| 					me ?
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 6.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
| 222| 222| 				newrole == "moderator" ?
| 223| 223| 					me ?
| 224| 224| 						translate("You are now a moderator.") :
| 225|    |-						translate("%(nick)s is now a moderator.") :
|    | 225|+							translate("%(nick)s is now a moderator.") :
| 226| 226| 				msg.oldrole == "visitor" ?
| 227| 227| 					me ?
| 228| 228| 						translate("You have been unmuted.") :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 6 tabs but found 4.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
| 223| 223| 					me ?
| 224| 224| 						translate("You are now a moderator.") :
| 225| 225| 						translate("%(nick)s is now a moderator.") :
| 226|    |-				msg.oldrole == "visitor" ?
|    | 226|+						msg.oldrole == "visitor" ?
| 227| 227| 					me ?
| 228| 228| 						translate("You have been unmuted.") :
| 229| 229| 						translate("%(nick)s has been unmuted.") :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
| 224| 224| 						translate("You are now a moderator.") :
| 225| 225| 						translate("%(nick)s is now a moderator.") :
| 226| 226| 				msg.oldrole == "visitor" ?
| 227|    |-					me ?
|    | 227|+							me ?
| 228| 228| 						translate("You have been unmuted.") :
| 229| 229| 						translate("%(nick)s has been unmuted.") :
| 230| 230| 					me ?
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 6.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
| 225| 225| 						translate("%(nick)s is now a moderator.") :
| 226| 226| 				msg.oldrole == "visitor" ?
| 227| 227| 					me ?
| 228|    |-						translate("You have been unmuted.") :
|    | 228|+								translate("You have been unmuted.") :
| 229| 229| 						translate("%(nick)s has been unmuted.") :
| 230| 230| 					me ?
| 231| 231| 						translate("You are not a moderator anymore.") :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 6.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
| 226| 226| 				msg.oldrole == "visitor" ?
| 227| 227| 					me ?
| 228| 228| 						translate("You have been unmuted.") :
| 229|    |-						translate("%(nick)s has been unmuted.") :
|    | 229|+								translate("%(nick)s has been unmuted.") :
| 230| 230| 					me ?
| 231| 231| 						translate("You are not a moderator anymore.") :
| 232| 232| 						translate("%(nick)s is not a moderator anymore.");
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
| 227| 227| 					me ?
| 228| 228| 						translate("You have been unmuted.") :
| 229| 229| 						translate("%(nick)s has been unmuted.") :
| 230|    |-					me ?
|    | 230|+							me ?
| 231| 231| 						translate("You are not a moderator anymore.") :
| 232| 232| 						translate("%(nick)s is not a moderator anymore.");
| 233| 233| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 6.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
| 228| 228| 						translate("You have been unmuted.") :
| 229| 229| 						translate("%(nick)s has been unmuted.") :
| 230| 230| 					me ?
| 231|    |-						translate("You are not a moderator anymore.") :
|    | 231|+								translate("You are not a moderator anymore.") :
| 232| 232| 						translate("%(nick)s is not a moderator anymore.");
| 233| 233| 
| 234| 234| 			addChatMessage({
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 6.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/lobby/lobby.js
| 229| 229| 						translate("%(nick)s has been unmuted.") :
| 230| 230| 					me ?
| 231| 231| 						translate("You are not a moderator anymore.") :
| 232|    |-						translate("%(nick)s is not a moderator anymore.");
|    | 232|+								translate("%(nick)s is not a moderator anymore.");
| 233| 233| 
| 234| 234| 			addChatMessage({
| 235| 235| 				"text": "/special " + sprintf(txt, { "nick": msg.nick }),

binaries/data/mods/public/gui/lobby/lobby.js
|1035| »   »   switch·(sortBy)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/lobby/lobby.js
| 729| »   »   case·'name':
|    | [NORMAL] JSHintBear:
|    | Expected a 'break' statement before 'default'.
|    | [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 (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
|1240|1240| 			offset = -Math.min(slideSpeed * dt, maxOffset);
|1241|1241| 	}
|1242|1242| 
|1243|    |-	updateSettingsPanelPosition(offset);	
|    |1243|+	updateSettingsPanelPosition(offset);
|1244|1244| }
|1245|1245| 
|1246|1246| /**
|    | [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
|1774|1774| 	let biomeList;
|1775|1775| 
|1776|1776| 	if (g_GameAttributes.mapType == "random" && g_GameAttributes.settings.SupportedBiomes)
|1777|    |-	{
|    |1777|+	
|1778|1778| 		if (typeof g_GameAttributes.settings.SupportedBiomes == "string")
|1779|1779| 			biomeList = g_Settings.Biomes.filter(biome => biome.Id.startsWith(g_GameAttributes.settings.SupportedBiomes));
|1780|1780| 		else
|1781|1781| 			biomeList = g_Settings.Biomes.filter(
|1782|1782| 				biome => g_GameAttributes.settings.SupportedBiomes.indexOf(biome.Id) != -1);
|1783|    |-	}
|    |1783|+	
|1784|1784| 
|1785|1785| 	g_BiomeList = biomeList && prepareForDropdown(
|1786|1786| 		[{

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|2018| »   while·(g_IsNetworked)
|    | [NORMAL] ESLintBear (no-unmodified-loop-condition):
|    | 'g_IsNetworked' is not modified in this loop.
Executing section cli...

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

nani awarded a token.Jun 11 2019, 9:57 PM

This is extremely non-trivial to fix in CText, as Gui::GenerateText supports wrapping around images. If we break down text in substrings, we will break this behaviour.
A workaround would be to create CTextList for separating elements manually. That's very equivalent to CList at the moment.

So the debate, which has two parts, becomes:

  1. Which component to use?
    1. Do we fix CText to support some Rope-like data structure, but including images? This sounds so hard to do.
    2. Do we implement a CTextList class?
    3. Do we use CList for this
  2. Which JS property to use
    1. Use a different JSClass altogether, overloading GetProperty?
    2. Use the implemented system of defining functions manually on the JSClass object?

I've spent enough time on this, so my pick is what I've currently implemented, aka options 1-C and 2-B.

Krinkle added a subscriber: Krinkle.

This is extremely non-trivial to fix in CText, as Gui::GenerateText supports wrapping around images. If we break down text in substrings, we will break this behaviour.

CText inherits GUITextOwner that stores a vector of texts and provides an AddText function, that's insurmountably incompatible?

source/gui/CList.cpp
90 ↗(On Diff #8426)

That's much better than before! Even cleaner would be registering it as a custom JSClass and registering the variable the way it should be directly instead of late on first JSObject* CList::GetJSObject() call and then checking that for initialized every call.

In D1781#89177, @elexis wrote:

This is extremely non-trivial to fix in CText, as Gui::GenerateText supports wrapping around images. If we break down text in substrings, we will break this behaviour.

CText inherits GUITextOwner that stores a vector of texts and provides an AddText function, that's insurmountably incompatible?

I think I was trying a little too hard there perhaps. My point was that if you take a paragraph with images, and split that in several text elements (to enable inserting in the middle etc.) then you have a serious problem.

If you let the user add different paragraphs and treat each as its own text bit, then it works better and it sounds doable.

source/gui/CList.cpp
90 ↗(On Diff #8426)

I originally did not want to use a different JSClass, but I suppose we don't actually care that our elements are different classes, so it could be done that way.

So the JS registration seems unimprovable i.e. ideal, beside the cast.

source/gui/CList.cpp
79 ↗(On Diff #8426)

(unneeded variable initialised)

82 ↗(On Diff #8426)

(If they are part of the namespace (like with the other JSInterface GUI class specs), then one doesn'T have to modify GetJSObject in order to register a new function, but can do it independently.)

90 ↗(On Diff #8426)

Actually cant do it on init but have to refer to first use, since that was a design choice, from its definition in IGUIObject.cpp:

Cache the object when somebody first asks for it, because otherwise
we end up doing far too much object allocation.

source/gui/IGUIObject.h
503 ↗(On Diff #8426)

righteous to make them protected, let the inheriting classes access, but there is no reason to make it public yet

source/gui/scripting/JSInterface_CList.cpp
32 ↗(On Diff #8426)

dynamic_cast(static_cast()) worked for me without the DerivedPointer in D2136. Also matches the theory that dynamic_casts are used for polymorphism?

source/gui/scripting/JSInterface_CList.h
25 ↗(On Diff #8426)

ok

elexis added inline comments.Jul 30 2019, 8:10 PM
source/gui/scripting/JSInterface_CList.cpp
32 ↗(On Diff #8426)

A greater problem with the GetDerivedPointer approach is that it won't work if there is a class that inherits two classes that inherit IGUIObject, because this statement is calling GetDerivedPointer from the base class, so it will only return one of the two classes.

Consider for example class CText : public IGUIScrollBarOwner, public IGUITextOwner or class CList : public IGUIScrollBarOwner, public IGUITextOwner. If there is a JSInterface for the ScrollBar, TextOwner (D2136) then one of the two sets of registered functions should break.

elexis retitled this revision from Fix chat performance issue. to Fix lobby chat performance issue..Sep 10 2019, 9:21 PM

Other than the problem that the entire chattext is rebuilt instead of partially rebuilt (only appended line built), there is another effect, similar to the one in #3386:
If there are N different GUI messages in the queue (that will then be pulled on the same onTick), there will be N updates to that GUIObject, but the GUI object only needs to be updated once per frame.
With this patch, only the N addList consecutive entries are built, so that's good.

I still think however that this thing should not become a CList but a CInput with selectable readonly and tag-parsed text.
I had started writing the patch in P167, but it's not so easy.
This feature would also be used for the Intro.txt Manual page, and for the Terms of Service page.

We can still add the CList patch, but it still means that we have to rewrite the entire feature from scratch if we want to serve the use case of selecting the colorized text with partial text rebuilds.

wraitii planned changes to this revision.EditedSun, May 24, 9:54 AM

A look at this with a pair of fresh eyes and some better understanding of JS and SpiderMonkey (I think).
Recap: how our GUI works:

  • We have C++ objects, inheriting 1+ GUI classes. Most are children of IGUIObjects, though some (Scrollbar, TextOwner) are "capabilities" that are added by composition (-ish, they're inherited but those classes keep a reference to the GUI object instead of calling this). That's since D2325, following itself D2136. Fundamentally, that was a change towards Composition, which is preferred here I think. Anyways, irrelevant here.
  • We have JS objects. JS objects can't inherit multiple prototypes. Thus, we can only have on JSCLass per C++ type _if_ composition over inheritance is the rule. Even with the rework above, I don't think we really fit the mold.
  • The JS objects have a custom JSClass, with a custom prototype and a getter/setter hook (which gets moved around in SM52, but that's fine).

So we have three code-paths to get C++ values from JS:

  • Settings, such as .caption. These are not defined on the JS object, but instead the getProperty hook returns the value from the C++ object directly. As such, they are of course not enumerable.
  • Class Functions, such as .focus and .blur, are defined directly on the prototype via JS_InitClass.
  • Custom functions (.getTextSize), since D2136, are defined directly on the object, as properties.

In SM parlance, our objects are NONNATIVE objects, i.e. they really have no JS-only values (in fact, you can't set any property from JS). We must implement manually, in C++, all functionality.
The approach of a custom JSCLass for this, like we do, is recommended against in Modern SM, starting with SM52 I think, in favour of using a Proxy Object.
In the comments for Proxy.h on modern SM, you see this:

* Native objects with custom JSClassOps / ObjectOps are used when the object
* behaves very similar to a normal object such as the ArrayObject and it's
* length property. Most usages wrapping a C++ or other type should prefer
* using a Proxy. Using the proxy approach makes it much easier to create an
* ECMAScript and JIT compatible object, particularly if using an appropriate
* base class.

This is valuable because if somebody were to rework this, we'd probably want to implement a Proxy object instead.

Anyways. What we want, from a C++ perspective, is to be able to add custom properties (not values or functions) to GUI objects that define a C++ Object. Define some of those only on those objects (eg here "addText" only for List objects).
To me, the simplest solution for that is the following:

  • Each C++ Object-Class defines a JSI_XX class. That class is a Proxy.
  • When a GUIObject's script is requested, we create a new proxy instance.

What this means:

  • The actual GUIObjects are very thin wrappers -> save on memory, we possibly might even skip the lazy initialisation scheme.
  • We can easily define arbitrary functions and values in c++ by calling "RegisterScriptFunctions" and "RegisterScriptSettings" when initially creating the prototype. This supports composition, with the assumption that composited-objects won't accidentally overwrite properties (doesn't seem like a big problem).

This design does not require adding more and more exceptions to the "GetSetting" property hook, since we'll simply follow the prototype chain, and it works with c++ virtual functions : define a "GetPrototype" function and IGUIObject will do the rest.

With that being said, I will proceed to work on this. Ping @Itms because this is related to SM52. Ping @elexis because this is of interest to you.

wraitii updated this revision to Diff 12012.Mon, May 25, 8:35 PM

Same fix, rebased on top of D2768 and the gamesetup rewrite.

I haven't been able to figure out what the lobby Flushmessages did, so I moved the call.

Build failure - The Moirai have given mortals hearts that can endure.

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

Build failure - The Moirai have given mortals hearts that can endure.

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

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/827/display/redirect

wraitii updated this revision to Diff 12020.Tue, May 26, 8:58 AM
wraitii edited the summary of this revision. (Show Details)

Slightly cleaner code with D2768 improvements.

Build failure - The Moirai have given mortals hearts that can endure.

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

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/833/display/redirect

wraitii added inline comments.Tue, May 26, 9:01 AM
source/gui/Scripting/JSInterface_IGUIObject.h
160

Er, forgot to add the correct using here.

wraitii added inline comments.Tue, May 26, 9:04 AM
source/gui/Scripting/JSInterface_IGUIObject.h
160

This isn't detected at compile time because I'm using the actual return type of the function pointer in the template code, as the using isn't 'virtual'...

I think I can fix this with CRTP, but it might be a tad tricky.

Build failure - The Moirai have given mortals hearts that can endure.

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