The button should not be clicked again once we start connecting, until the connection is lost and the page reloaded, or succeeded and the page closed.
Details
Details
Login. Try to register an existing nick. Click on canel while connecting.
Diff Detail
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
Comment Actions
I've tested it, it works good for me. The nice patch!
I suggest to move the Engine.GetGUIObjectByName(...).enabled = false; lines before the:
if (Engine.HasXmppClient()) Engine.StopXmppClient();
Because it stops the xmpp client and it doesn't exit from functions. It costs time, so we should prevent user clicks while it happens.
Comment Actions
Can you name a situation where g_IsConnecting is false but Engine.HasXmppClient() true?
Comment Actions
Then why we need these conditions? If Engine.HasXmppClient() is always false when g_IsConnecting is false, we should remove it.
Anyway, if we have the condition with some cost, we need to move .enabled = false before it.
Comment Actions
I think it's to prevent a crash in case someone breaks the GUI or encounters a rare GUI bug.
Doesn't necessarily mean we have to add these checks when writing code, but it also doesn't mean that we have to remove them now IMO.
Once could delete the call and replace the ENSURE(!g_XmppClient); in StartRegisterXmppClient with SAFE_DELETE(g_XmppClient).
But then again I'd rather have it crash the JS check in case someone breaks the GUI than adding code to catch that beforehand.
if we have the condition with some cost, we need to move .enabled = false before it.
If that case occurs close to never, then the code should be optimized for readability (grouping GUI code) IMO, because it is read close to always.
Maybe I have a terrible sense of code taste (prefering code appearance over logic), but I'd prefer grouping the GUI logic unless someone demonstrates the relevance.
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Checking XML files...
http://jw:8080/job/phabricator/1911/ for more details.
Comment Actions
Executing section Default...
Executing section Source...
Executing section JS...
binaries/data/mods/public/gui/lobby/prelobby.js
| 165| » » switch(message.level)·{
| | [NORMAL] ESLintBear (default-case):
| | Expected a default case.
binaries/data/mods/public/gui/lobby/prelobby.js
| 225| » switch(page)
| | [NORMAL] ESLintBear (default-case):
| | Expected a default case.
binaries/data/mods/public/gui/lobby/prelobby.js
| 157| » while·((message·=·Engine.LobbyGuiPollMessage())·!=·undefined)
| | [NORMAL] JSHintBear:
| | Use '!==' to compare with 'undefined'.
Executing section XML GUI...
Executing section Python...
Executing section Perl...http://jw:8080/job/phabricator_lint/436/ for more details.
Comment Actions
The other GUI stuff should be done before XmppClient deletion as well then. I'm not willing to investigate further whether we can delete the deletion until this page is cleaned properly.
In fact because of the mixing of different dialogs in the same dialog, I wouldn't be surprised if there were some bugs like that (bugs like we had numerous before).