HomeWildfire Games

Rewrite lobby page to use class semantics, add more gamedetails labels, improve…
AuditedrP23172

Description

Rewrite lobby page to use class semantics, add more gamedetails labels, improve performance using batch processing and caching and gain possibility for game creation/player-join/leave events, refs #5387.

Game selection details features:

  • Display victory conditions following their sending but missing display following rP14098, refs rP19642.
  • Display the host of the match and the game name in the selected game details following rP19703, refs D1666.
  • Display mods if the mods differ (without having to attempt to join the game prior) following rP21301.

Performance features:

  • Implement batch message processing in the XmppClient to rebuild GUI objects only once when receiving backlog or returning from a match.
  • Implement Game class to cache gamelist, filter and sorting values, as they rarely change but are accessed often.
  • Cache sprintf objects.

Security fixes:

  • Add escapeText in lobby/ to avoid players breaking the lobby for every participant, supersedes D720, comments by bb.
  • Do not hide broadcasted unrecognized chat commands that mods used as leaking private channels, fixes #5615.

Defect fixes:

  • Fix XmppClient.cpp storing unused historic message types resulting in memory waste and unintentional replay of for instance disconnect/announcements messages following both rP20070/D819 and rP22855/D2265, refs #3306.
  • Fix XmppClient.cpp victoryCondition -> victoryConditions gamesetup.js change from rP21474/D1240.
  • Fix leaderboard/profile page cancel hotkey closing the lobby dialog as well and removes cancel hotkey note from lobby_panels.xml from rP20886/D817 since the described issue was fixed by rP22200/D1701.
  • Fix lobby playing menu sound in a running game after having closed the lobby dialog following introduction in rP20886/D817.
  • Fix GUI on nick change by updating g_Username.
  • Update profile panel only with data matching the player requested.

Hack erasure:

  • Object semantics make it cheap to add state and cache values, storing literals in properties while removing globals, adding events while decoupling components and gaining moddability.
  • Erase comments and translation comments stating that this would be IRC!!, supersedes D1136.
  • Introduce Status chat message type to supersede "/special" chat command + "isSpecial" property from rP14098 (formerly g_specialKey rP17360) deluxe hack.
  • Introduce System chat message type to supersede system errors disguising as chat from a mock user called "system".

Code cleanups:

  • Move code from XML to JS.
  • Move size values from JS to XML, especially following rP20886/D817 and rP21003/D1051.
  • Rename "user" to "player".
  • Fix lobby/ eslint warnings, refs D2261.
  • Remove message.nick emptiness check from rP20064/D835, since XEP-0045 dictates that it is non-empty.
  • Add translated string for deleted subjects.
  • Add TODOs for some evident COList issues, refs #5638.

Differential Revision: https://code.wildfiregames.com/D2412

Details

Auditors
Stan
Committed
elexisNov 21 2019, 2:44 PM
Differential Revision
D2412: Lobby class implementation
Parents
rP23171: Fix rP23170
Branches
Unknown
Tags
Unknown
Build Status
Buildable 10074
Build 17058: Trigger Windows Autobuild
Build 17057: Post-Commit BuildJenkins

Event Timeline

Stan added a subscriber: Stan.Nov 21 2019, 4:03 PM

I get this after rebuild

Stan raised a concern with this commit.Nov 21 2019, 4:29 PM
This commit now has outstanding concerns.Nov 21 2019, 4:29 PM
Stan added a comment.Nov 21 2019, 5:13 PM

This commit introduces two files with different cases which is only supported on case sensitive filesystems. ie not windows.

ps/trunk/binaries/data/mods/public/gui/lobby/lobby.js
ps/trunk/binaries/data/mods/public/gui/lobby/Lobby.js
Stan accepted this commit.Nov 21 2019, 7:37 PM

Fixed in rP23172 If someone is on windows or a non case sensitive OS, he needs to be wary that this commit should never be checked out.

All concerns with this commit have now been addressed.Nov 21 2019, 7:37 PM
Krinkle added inline comments.
/ps/trunk/binaries/data/mods/public/gui/lobby/LobbyPage/Game.js
153
ESLint says
/0ad-git/binaries/data/mods/public/gui/lobby/LobbyPage/Game.js
  153:3  warning  Nested block is redundant  no-lone-blocks

Maybe you had a local variable here in an earlier version? As-is this block seems to be redundant.

Krinkle added inline comments.
/ps/trunk/binaries/data/mods/public/gui/lobby/LobbyPage/Game.js
153

Submitted a patch at D2452.

elexis added inline comments.Dec 6 2019, 11:39 AM
/ps/trunk/binaries/data/mods/public/gui/lobby/LobbyPage/Game.js
153

No, I disagree with this eslint warning and wrote the code intentionally this way.
I thoguht I have posted this on D2412, but apparently I didn't.

Just because there is no let keyword doesn't mean that the block is useless.
The purpose of the block is to indicate to the reader that there is one logical chunk, that all line in that chunk are to relate to this block and not to the blocks below, to make it impossible to add a local variable in the blocks that is used outside of the block without declaring it outside of the block (as here with players).
It shall prevent developers from reducing the cohesion by introducing a line into that block that is relating to the code in the blocks further below that would break the first block into incohesive code parts.
Secondly it makes it even more explicit that its specifically that block that is profiled.

It's like splitting a large function into two functions merely to indicate to the reader that these are two logical chunks, even if it would not be necssary code-wise (but we don't get an ESLint warning about that), just that we avoid the cost of 2 function calls by just adding blocks.

elexis added inline comments.Dec 6 2019, 11:46 AM
/ps/trunk/binaries/data/mods/public/gui/lobby/LobbyPage/Game.js
153

Another example D195 / rP19552 introduced scopes as a pattern to avoid people adding globals and not serializing them.
Subsequently jebel_barkal_triggers.js reuses the same scope in advance, but the linter gets the same stroke.

{
	Engine.QueryInterface(SYSTEM_ENTITY, IID_Trigger).RegisterTrigger("OnInitGame", "JebelBarkal_Init", { "enabled": true });
}
binaries/data/mods/public/maps/random/jebel_barkal_triggers.js
| 638| {
|    | [NORMAL] ESLintBear (no-lone-blocks):
|    | Block is redundant.
Krinkle added inline comments.Dec 7 2019, 10:05 PM
/ps/trunk/binaries/data/mods/public/gui/lobby/LobbyPage/Game.js
153

OK. That's fine. I don't normally use this rule either. I'm once again assuming that the linter configuration was agreed upon by maintainers and I'm trying to fit in and not argue about smaller details I don't care about as much.

Anyhow, if you saw this already, it wouldn't hurt to remove the rule while at it. I've now changed D2452 to do so :)