HomeWildfire Games

Rewrite the prelobby pages and add the Terms of Service, Terms of Use and the…
Concern RaisedrP21847

Description

Rewrite the prelobby pages and add the Terms of Service, Terms of Use and the agreement checkbox to the login page.
Ensure lobby players cannot join without acceptance of the terms in case they change, fixes #5218, as agreed in last staff meeting.

Separate lobby entrance, lobby login and lobby register GUI page.
Since rP14098 they were squashed into a single GUI page;
Adding lots of hardcode to set the visibility of GUI objects;
Reinventing a GUI page manager in JS;
Unintentionally persisting data between pages;
Requiring lots of errorprone case distinctions which are unneeded once relevant GUI objects and code is loaded exclusively.

Add the remember-password checkbox from rP21311 / D822 to the registration page too.

Revert the revert of rP17581 in rP17584 and continue to prefer objects with separate functions per C++ GUI message type,
reducing the nesting of conditionals per function and reveal codeflow by making input and output variables explicit.

Fix oversight in rP20064 by moving the registrationrate string from rP19205 from the "disconnect" to the "error" case.

Differential Revision: https://code.wildfiregames.com/D1568
Few comments by: Vladislav, Imarok, gallaecio

Event Timeline

Angen raised a concern with this commit.Jun 12 2018, 8:49 AM
Angen added a subscriber: Angen.
Angen added inline comments.
/ps/trunk/binaries/data/mods/public/gui/prelobby/common/credentials/credentials.xml
11

</object>> -> </object>

This commit now has outstanding concerns.Jun 12 2018, 8:49 AM
Angen added a comment.Jun 12 2018, 8:55 AM

why do I have to read terms of services every single time I want to connect? It is annoying and time consuming.

why do I have to read terms of services every single time I want to connect? It is annoying and time consuming.

Because of what the commit message says and because the patch to persist agreement if the terms did not change is not committed yet.

/ps/trunk/binaries/data/mods/public/gui/prelobby/common/credentials/credentials.xml
11

strange, why doesn't this defacto syntax error trigger any errors?

Stan added a subscriber: Stan.Jun 12 2018, 9:36 AM
Stan added inline comments.
/ps/trunk/binaries/data/mods/public/gui/prelobby/common/credentials/credentials.xml
11

Cause it's allowed to have > in XML as long as it's wrapped correctly with CDATA I guess.

elexis added inline comments.Jun 12 2018, 9:46 AM
/ps/trunk/binaries/data/mods/public/gui/prelobby/common/credentials/credentials.xml
11

Why does gameboy get a syntax error on windows but I don't get one on linux?
This must be a version thing or a platform bug.

https://wildfiregames.com/forum/index.php?/topic/24480-svn21848-error/

ERROR: RelaxNGValidator: Validation error: gui/prelobby/common/credentials/credentials.xml:2: Did not expect text in element object content
ERROR: RelaxNGValidator: Validation failed for '(null)' 
ERROR: CXeromyces: failed to validate XML file gui/prelobby/common/credentials/credentials.xml 

Yet I can trigger for instance this by adding a syntax error

ERROR: RelaxNGValidator: Validation error: gui/prelobby/common/credentials/credentials.xml:4: Did not expect text in element object content

Stan added a comment.Jun 12 2018, 9:56 AM

Different version of libxml maybe ?

why do I have to read terms of services every single time I want to connect? It is annoying and time consuming.

(I have accepted the terms and conditions about 200 times when testing the patch)

elexis requested verification of this commit.Jun 12 2018, 11:21 AM

Fixed by rP21849, blame #5222.

This commit now requires verification by auditors.Jun 12 2018, 11:21 AM
Angen accepted this commit.Jun 12 2018, 11:22 AM
All concerns with this commit have now been addressed.Jun 12 2018, 11:22 AM
JoshuaJB raised a concern with this commit.Jul 16 2018, 4:25 AM
JoshuaJB added a subscriber: JoshuaJB.

There seems to be an issue where we do not always correctly persist the state of the user's acceptance on first-run.
Repro:

  1. Reset to default config
  2. Open lobby, enter incorrect password, accept terms, click login
  3. Observe incorrect password message
  4. Enter correct password
  5. Observe requirement to re-read and accept the terms, even though you already did that during your earlier login attempt
This commit now has outstanding concerns.Jul 16 2018, 4:26 AM
elexis requested verification of this commit.Jul 16 2018, 12:28 PM

This is not a bug, consider two brothers on the same computer with 2 different accounts, one of them tries to login for the other one, but the one logging in is the one who had to read it and the confirmation that the password is correct is the only way of confirming that the one who read the terms is the one who accepted it.

This commit now requires verification by auditors.Jul 16 2018, 12:28 PM
JoshuaJB accepted this commit.Jul 16 2018, 8:18 PM

Okay, seems reasonable; we want the user to verify their identity before accepting the terms. We could ask for the user to re-accept the terms (if they've changed) on another page after the login one to improve the experience. I can look into implementing that (and maybe storing the terms acceptance state per-account on the server) if there are no objections.

All concerns with this commit have now been addressed.Jul 16 2018, 8:18 PM
smiley added a subscriber: smiley.Feb 1 2019, 6:07 PM

Press continue, click and accept any of the prelobby docs, see that the continue button is enabled again and press it.

/ps/trunk/binaries/data/mods/public/gui/prelobby/common/feedback/feedback.js
31

This should check for xmpp client existence.

/ps/trunk/binaries/data/mods/public/gui/prelobby/login/login.js
24

Alternatively, here.

smiley added inline comments.Feb 1 2019, 6:11 PM
/ps/trunk/binaries/data/mods/public/gui/prelobby/common/feedback/feedback.js
15

Using this opportunity to mention whether assignment in the while loop is nicer.

elexis marked an inline comment as done.Feb 2 2019, 12:24 PM

Press continue, click and accept any of the prelobby docs, see that the continue button is enabled again and press it.

Seems like the first steps and the observed problem are missing.

I didn't test yet, I suppose a failed registration attempt leaves g_XmppClient initialized, but it should be destroyed after the failure, so that the user can retry then with another nickname?
If that's the case, then the right fix would be to call StopXmppClient or whatever it was instead of disabling the continue button?

/ps/trunk/binaries/data/mods/public/gui/prelobby/common/feedback/feedback.js
15

I've never been a fan of assignments within conditions, but given that we our codebase uses JS operators the way they work (for example && and || returning not boolean but one of the condtiions), it seems that could become a new JS fetish too. Should just keep these assignments within conditions simple so that noone will mistake it for a read-only line.

Seems like the first steps and the observed problem are missing.

Not registration, just when logging in. Although, the problem would probably exist then as well. The problem is a second xmpp client being created while the earlier one is logging in. And hence an assertion failure.

/ps/trunk/binaries/data/mods/public/gui/prelobby/common/feedback/feedback.js
15
Index: binaries/data/mods/public/gui/lobby/lobby.js
===================================================================
--- binaries/data/mods/public/gui/lobby/lobby.js	(revision 21929)
+++ binaries/data/mods/public/gui/lobby/lobby.js	(working copy)
@@ -1292,13 +1292,10 @@
 	updateTimers();
 
 	let updateList = false;
+	let msg;
 
-	while (true)
+	while ((msg = Engine.LobbyGuiPollNewMessage()))
 	{
-		let msg = Engine.LobbyGuiPollNewMessage();
-		if (!msg)
-			break;
-
 		if (!g_NetMessageTypes[msg.type])
 		{
 			warn("Unrecognised message type: " + msg.type);

Is much nicer IMO. But that's just my opinion.

smiley added inline comments.Feb 2 2019, 12:52 PM
/ps/trunk/binaries/data/mods/public/gui/prelobby/common/feedback/feedback.js
15

it seems that could become a new JS fetish too

Not really JS specific and it would be valid in C/C++ (among others) too. But yeah, I get where you are coming from.

elexis added a comment.Feb 2 2019, 1:52 PM

That let outside of the loop scope is a disadvantage though unless we want to use that variable outside of the loop.

As far as I recall, the C++ g_XmppClient instance should be set if and only if the client has successfully registered an account (and this then nuked) or when having successfully logged in (and thus left this page).
If a registration or login failure occured, the instance should be deleted, so that the user can try again, right?
(Then there's the hypothetical race condition where the user doubleclicks on some XmppClient initialization button twice before it's disabled. But that should be caught already.)

smiley added a comment.EditedFeb 2 2019, 1:55 PM

Then there's the hypothetical race condition where the user doubleclicks on some XmppClient initialization button twice before it's disabled. But that should be caught already

Evidently, it is not. That is exactly the problem (at least that’s the gist of it.)

Can just do let msg = too I suppose.

elexis added a comment.Feb 2 2019, 3:05 PM

Can just do let msg = too I suppose.

(In the while condition? Is that even legal? I'm currently stuck in c++ bytestream hell otherwise I'd have tested or looked up.)

Then there's the hypothetical race condition where the user doubleclicks on some XmppClient initialization button twice before it's disabled. But that should be caught already

Evidently, it is not. That is exactly the problem (at least that’s the gist of it.)

There totally was a commit that precisely disabled some prelobby confirmation button, did silly me unconsciously revert that nonsilly commit of mine? -.-

smiley added a comment.EditedFeb 2 2019, 3:18 PM

(In the while condition? Is that even legal? I'm currently stuck in c++ bytestream hell otherwise I'd have tested or looked up.)

Probably not. (Feeling too meh to test)
Edit: Yes, it is not legal.

There totally was a commit...

I don't think this issue is the same.

I guess patches speak louder than comments on an old commit. Although, speaking loud doesn't necessarily mean someone is listening.

elexis added a comment.Feb 3 2019, 1:42 AM

rP20053
Can you raise a concern, so that we don't forget about it?

wraitii raised a concern with this commit.Tue, Jun 25, 2:37 PM
wraitii added a subscriber: wraitii.

Concern:

  • this reverted rP20053 by accident, and we should again disable the login/registration button when clicking on them to prevent acting twice, which crashes.

See inline for where I think we need to fix this.

/ps/trunk/binaries/data/mods/public/gui/prelobby/login/login.js
24

From what I understand we need Engine.GetGUIObjectByName("continue").enabled = false; here

/ps/trunk/binaries/data/mods/public/gui/prelobby/register/register.js
20

and here

This commit now has outstanding concerns.Tue, Jun 25, 2:37 PM
elexis added inline comments.Tue, Jun 25, 3:06 PM
/ps/trunk/binaries/data/mods/public/gui/prelobby/login/login.js
24

Also important to add a comment // Catch double-click or // Don't crash on doubleclick, otherwise the line will be removed yet again in some future cleanup.

/ps/trunk/binaries/data/mods/public/gui/prelobby/register/register.js
25

Also, in JSI_Lobby::StartXmppClient and ConnectXmppClient, the ENSURE(!g_XmppClient); should be changed to a JS error, because one doesn't need a C++ stacktrace but a JS one to debug the according error.