Page MenuHomeWildfire Games
Differential D396

Lobby GUI cleanup and disconnect enhancements
ClosedPublic
Actions

Authored by elexis on Apr 29 2017, 11:33 PM.

Details

Reviewers
vladislavbelov
Commits
rP19501: JS lobby cleanup.
Summary

As proposed by Vladislav, disable the toggle buddy button upon disconnect too.
Hide the chat input then as well, since it's going to become invisible with D339 too.
Moves the code to open and close the leaderboard from XML to JS, this way it is easier to read, compare, change and extend.
Remove a return from displayProfile which is never triggered.
Split displayProfile into two functions as merging them just produced spaghetti code.

Test Plan

Open the lobby window in one instance. Open the lobby window with the same account in a second instance.
See that the first instance became disconnected.
Type a non-existing name in the user profile button and verify that there is no fake profile displayed of a non-existing user.
Click on some playernames of the leaderboard.

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

elexis created this revision.Apr 29 2017, 11:33 PM
Harbormaster completed remote builds in B1344: Diff 1544.Apr 29 2017, 11:33 PM
Vulcan added a subscriber: Vulcan.Apr 30 2017, 12:27 AM

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!

http://jw:8080/job/phabricator/929/ for more details.

Harbormaster completed remote builds in B1344: Diff 1544.Apr 30 2017, 12:28 AM
vladislavbelov edited edge metadata.Apr 30 2017, 2:14 AM

We shouldn't hide current description in the left when open Rating/Lookup, because it strange disappear and appear again after closing.

binaries/data/mods/public/gui/lobby/lobby.js
137 ↗(On Diff #1544)

for (let button in ...)

elexis updated this revision to Diff 1547.Apr 30 2017, 2:41 AM

Improvement suggested by Vladislav: Only hide the previously selected playerinfo if there was no player selected yet.

Harbormaster completed remote builds in B1347: Diff 1547.Apr 30 2017, 2:41 AM
Vulcan added a comment.Apr 30 2017, 3:57 AM

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!

http://jw:8080/job/phabricator/932/ for more details.

Harbormaster completed remote builds in B1347: Diff 1547.Apr 30 2017, 3:57 AM
vladislavbelov requested changes to this revision.May 1 2017, 1:25 AM
vladislavbelov added inline comments.
binaries/data/mods/public/gui/lobby/lobby.js
619 ↗(On Diff #1547)

Duplicate, I think, because all these functions are the same, except initialization, so it could like:

function setLeaderboardVisibility(visible)
{
    if (visible)
    {
        Engine.SendGetBoardList();
        lookupSelectedUserProfile("leaderboardBox");
    }
    Engine.GetGUIObjectByName("leaderboard").hidden = !visible;
    Engine.GetGUIObjectByName("fade").hidden = !visible;
}
634 ↗(On Diff #1547)

Too obvious duplicate, it'd better to have like:

function setUserProfileVisibility(visible)
{
    Engine.GetGUIObjectByName("profileFetch").hidden = !visible;
    Engine.GetGUIObjectByName("fade").hidden = !visible;
}
This revision now requires changes to proceed.May 1 2017, 1:25 AM
elexis marked 3 inline comments as done.May 1 2017, 1:39 AM

Ogled that unification too, but didn't think of an if statement to fix the non-unified part. That proposal looks better indeed. That's the good side of moving the code from XML to JS.
Also there has been a bug, if one opens the leaderboard and then closes it again, it should have selected the playerlist entry again (obvious from the XML too).

binaries/data/mods/public/gui/lobby/lobby.js
137 ↗(On Diff #1544)

(doesn't work)

elexis marked an inline comment as done.May 1 2017, 1:39 AM

Ogled that unification too, but didn't think of an if statement to fix the non-unified part. That proposal looks better indeed. That's the good side of moving the code from XML to JS.
Also there has been a bug, if one opens the leaderboard and then closes it again, it should have selected the playerlist entry again (obvious from the XML too).

elexis updated this revision to Diff 1566.May 1 2017, 1:40 AM
elexis edited edge metadata.

See above

vladislavbelov requested changes to this revision.May 1 2017, 2:12 AM
vladislavbelov added inline comments.
binaries/data/mods/public/gui/lobby/lobby.js
624 ↗(On Diff #1566)

Why playersBox? There is a bug, when click on Leaderboard, fast close it by Escape, and open User Profile Lookup, then it'll try automatically search a selected user. Keyword: fast click, until it loading, I think.

This revision now requires changes to proceed.May 1 2017, 2:12 AM
elexis requested review of this revision.May 1 2017, 2:33 AM
elexis edited edge metadata.
elexis added inline comments.
binaries/data/mods/public/gui/lobby/lobby.js
624 ↗(On Diff #1566)

Why playersBox?

Because that is already the case in the current system. When closing the window, it should display the previously selected profile again.

There is a bug, when click on Leaderboard, fast close it by Escape, and open User Profile Lookup, then it'll try automatically search a selected user. Keyword: fast click, until it loading, I think.

Exists already as well. It's because the current code assumes that the server is faster than the user. It will not check whether the received profile is the one of the user requested. It would have to process things synchroneously and remember which request goes to which window. (So a simple ignoring wouldn't fix all cases).

Would be a rewrite and if that is worthwhile at all, should be done in a separate patch.

Vulcan added a comment.May 1 2017, 4:27 AM

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!

http://jw:8080/job/phabricator/947/ for more details.

Harbormaster completed remote builds in B1367: Diff 1566.May 1 2017, 4:27 AM
elexis updated this revision to Diff 1577.May 1 2017, 7:20 AM

Remove "page" comment, since the leaderboard / user profile isn't a page.

Vulcan added a comment.May 1 2017, 10:17 AM

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!

http://jw:8080/job/phabricator/951/ for more details.

Harbormaster completed remote builds in B1379: Diff 1577.May 1 2017, 10:17 AM
vladislavbelov accepted this revision.May 1 2017, 11:43 AM

I've tested it, it works as before patch. The code looks good.

This revision is now accepted and ready to land.May 1 2017, 11:43 AM
Closed by commit rP19501: JS lobby cleanup. (authored by elexis). · Explain WhyMay 1 2017, 3:36 PM
This revision was automatically updated to reflect the committed changes.
elexis added a comment.May 1 2017, 3:36 PM

Thanks for the review!

Langbart mentioned this in D4262: Restoring the colored profile name with the ranking and using the leaderboard names for autocompletion in the profile player search field..Sep 8 2021, 5:41 AM

Revision Contents
Changeset List

PathSize
ps/
trunk/
binaries/
data/
mods/
public/
gui/
lobby/
67 lines
36 lines

Diff 1585

View Options

ps/trunk/binaries/data/mods/public/gui/lobby/lobby.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/lobby/lobby.xml

Loading...
Wildfire Games · Phabricator