Stay ready button
ClosedPublic

Authored by Imarok on Jan 6 2017, 9:50 PM.

Details

Reviewers
elexis
Commits
rP19216: Stay ready button
Trac Tickets
#4369
Summary

Use the I'm ready button, as a tristate button: first click: player gets ready -> second click: player stays ready -> third click: player isn't ready anymore
When the player stays ready, his ready state won't be reset when the gamesetup gets changed
Introduced blue as color for stay ready.

Test Plan

Tested the tristate button works.
Tested the right ready chat message gets displayed.
Tested the player stays ready after gamestup changes when he has clicked on stay ready

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.
Imarok updated this revision to Diff 153.Jan 6 2017, 9:50 PM
Imarok retitled this revision from to Stay ready button.
Imarok updated this object.
Imarok edited the test plan for this revision. (Show Details)
Imarok updated the Trac tickets for this revision.
Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Jan 6 2017, 9:50 PM
Vulcan added a subscriber: Vulcan.Jan 7 2017, 12:56 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

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

Imarok updated this revision to Diff 161.Jan 7 2017, 3:34 PM

Bump year

Vulcan added a comment.Jan 7 2017, 7:09 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

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

Perhaps we could include a warning the first time the player clicks this about the host being able to pull a fast one on them? :P

Why not a checkbox for staying ready?

Imarok added a comment.EditedJan 10 2017, 4:45 PM
In D49#1634, @leper wrote:

Why not a checkbox for staying ready?

  • I don't think its that intuitive.
  • It needs more space and where would you place it? Between the back and the ready button? right of the ready button? left of the back button?
  • How should the checkbox work? When enabled you use the ready button to toggle between stay ready and not ready?
elexis added a subscriber: elexis.Jan 11 2017, 3:36 AM

ffm has complained about the absence of this button and having to click a couple of dozen times per match on this button for years. He suggested to also add a countdown before the gamestart, so that players still have some seconds to verify the gamesettings. We could do that in another ticket.

About layout and graphics design:
A checkbox could be placed left of the back button but would indeed eat a lot of space that is currently used for the cheat warning and the tooltip:


However the layout shouldn't determine which controls we can use, the layout has to adapt to our requirements.

With the current layout, the checkbox could be a binary state button to fit to the other two buttons. But then again having two buttons with the labels "Don't stay ready" and "I'm not ready" simultaneously seems weird.

Use cases:

  1. A player accepts settings by default (and wants to avoid clicking on ready).
  2. A player accepts settings by default, but sometimes wants to revoke the ready state.
  3. A player accepts settings only after careful examination.
  4. A player accepts settings only after careful examination, but sometimes wants to revoke the ready state.

Comparing Use Cases against the implementation:
From the usability pov, case 1 and 3 are handled equally well with both approaches as only one click once / once per accepted gamesettings.
Revoking is the only part making a difference.

Tristate approach:

  1. This may be considered a common case, since players might most of the time agree with the hosts proposals. The player once doubleclicks on the button to stay ready. If new gamesettings want to be revoked, the player has to click once. A doubleclick is needed to get back to the "stay ready" state.
  2. This is an uncommon case, as the player by definition only clicks on ready after having read the gamesettings and since every change resets the state. In order to revoke the ready state, the player now needs a doubleclick, having the disadvantage of consuming a bit more time to revoke the readystate than a singleclick. This can be relevant if one noticed something just before the host wants to start the game.

Checkbox approach:

  1. The player checks the checkbox. Doing that implies setting the state for ready, so as to save one click. In order to revoke the readystate, one has to move the mouse and click on "Not Ready" (while the "Stay Ready" checkbox is still checked). Effort is comparable to the doubleclick. Plus it seems like a contradiction to have "Stay Ready" enabled while not being ready.
  2. This case is handled better with the checkbox approach since the checkbox is never touched but only the ready button. No doubleclicks nor mouse movements involved. However this case should occur less often.
elexis requested changes to this revision.Jan 11 2017, 8:01 PM
elexis edited edge metadata.
elexis added inline comments.
binaries/data/mods/public/gui/gamesetup/gamesetup.js
713 ↗(On Diff #161)

It were preferable to not hardcode the design decision to not display changes to stay-ready here.

Improvement: Stay-Ready chat message, can be done in a separate simple ticket.

However if we display a stay-ready chatmessage, then we will get one more chat message if people doubeclick (which might occur often).
So there could be a timer in gamesetup.js that is started if the message is received here.
If that client sent the stay-ready signal within N seconds, it could only display the "player stays ready" chat message,
whereas if the client doesn't send it in time, display both chat messages.

715 ↗(On Diff #161)

This g_FormatChatMessage could just be "ready", the status could be passed as an argument and the translated strings could be moved to the proposed array below.

When not implementing the timer proposed above, g_FormatChatMessage can return an empty string for the stay-ready case for now to still remove both hardcodings.

1864 ↗(On Diff #161)

I guess the only reason we have that function is to not add a global ref to the XML file.

1878 ↗(On Diff #161)

We don't construct these constant arrays again and again but init them only once. It could become a global array with 3 elements, each being an object with caption and tooltip. While at it move the colors there as well.

source/network/NetServer.cpp
777 ↗(On Diff #161)

Would be nice to use an enum for this.

This revision now requires changes to proceed.Jan 11 2017, 8:01 PM
Imarok marked an inline comment as done.Jan 15 2017, 11:00 PM
Imarok added inline comments.
binaries/data/mods/public/gui/gamesetup/gamesetup.js
1864 ↗(On Diff #161)

probably

Imarok updated this revision to Diff 252.Jan 15 2017, 11:25 PM
Imarok edited edge metadata.
Imarok marked an inline comment as done.

Apply elexis' remarks beside the enum and remove an useless !g_IsNetworked check

Imarok marked 3 inline comments as done.Jan 15 2017, 11:28 PM

Done everything besides the stay ready message (will be a new simple ticket) and the enum (not needed).

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

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

Imarok changed the visibility from "All Users" to "Public (No Login Required)".Jan 30 2017, 7:51 PM
elexis requested changes to this revision.Feb 8 2017, 5:41 PM

Looks good, only nitpicking

binaries/data/mods/public/gui/gamesetup/gamesetup.js
55 ↗(On Diff #252)

The first check seems okay, if the intention is to mute errors sent by a trollhost. But in that case the g_PlayerAssignments[guid].status].color should also have a check. If we don't care about trollhosts, then this check can be removed.

I'd prefer to remove the second check and add an empty string to the global array, so it's more obvious that nothing will be printed in that case and that the array always has all properties.

135 ↗(On Diff #252)

(would move the "and" to the line above, since that's also the place where the comma would be placed)

138 ↗(On Diff #252)

Better move it above g_FormatChatMessage, so that it's defined before being refered to (though only a code style thing, since the format chat functions won't be executed before the ready data is defined)

143 ↗(On Diff #252)

! -> .
Perhaps it should also or instead state that the player accepts the settings.

144 ↗(On Diff #252)

\n after the comma

148 ↗(On Diff #252)

Stay ready comma

153 ↗(On Diff #252)

\n after }

source/network/NetServer.cpp
776 ↗(On Diff #252)

range based for? There are two or three others that could be fixed in the same go

This revision now requires changes to proceed.Feb 8 2017, 5:41 PM
Imarok added inline comments.Feb 8 2017, 8:23 PM
binaries/data/mods/public/gui/gamesetup/gamesetup.js
148 ↗(On Diff #252)

sure?

elexis added inline comments.Feb 8 2017, 8:28 PM
binaries/data/mods/public/gui/gamesetup/gamesetup.js
148 ↗(On Diff #252)
Imarok updated this revision to Diff 484.Feb 8 2017, 8:37 PM
Imarok edited edge metadata.

Fix remarks.
(Don't add a comma see e.g. http://www.chompchomp.com/terms/subordinateclause.htm)

Imarok added inline comments.Feb 8 2017, 8:42 PM
binaries/data/mods/public/gui/gamesetup/gamesetup.js
148 ↗(On Diff #252)

yeah and between main clause and subordinate clause comes no comma.

elexis added inline comments.Feb 8 2017, 9:22 PM
binaries/data/mods/public/gui/gamesetup/gamesetup.js
713 ↗(On Diff #161)

The < 2 check is unneeded, right? as that function will return an empty string and that wont print anything

55 ↗(On Diff #252)

First check should still be removed as the patch doesn't care about trollhosts otherwise

148 ↗(On Diff #252)

thanks for looking it up :)

48 ↗(On Diff #484)

You removed the check but didn't add the empty string here, sounds like breakage

source/network/NetServer.cpp
776 ↗(On Diff #252)

thanks

779 ↗(On Diff #484)

p -> assignment?

Vulcan added a comment.Feb 8 2017, 9:33 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

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

Imarok added inline comments.Feb 9 2017, 4:25 PM
source/network/NetServer.cpp
779 ↗(On Diff #484)

At first I used assignment too, but to be consistent with the rest of the file I changed it to p

Imarok updated this revision to Diff 486.Feb 9 2017, 4:49 PM

Fixed js part

Vulcan added a comment.Feb 9 2017, 5:33 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

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

elexis requested changes to this revision.Feb 9 2017, 6:37 PM

Looks quite clean, doesn't it?

Also thought about adding a new option, thus making the entire discussion about binary or ternary button obsolete, but that should wait until we have tabs for the options dialog.

I didn't see it before, but those setReady calls with boolean arguments are wrong, it must be numbers now.
Host should appear as "always ready", right? So that would be a setReady(2); instead of true. But actually why should we do it in JS and not C++ when clearing ready? Saves us sending of a message and correcting a state uselessly.
All +g_IsReady should become g_IsReady

As a nice sideeffect of this feature, readybuttonspam is nerfed a bit.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
47 ↗(On Diff #486)

That color is unacceptably ugly. Perhaps 150 150 250

1918 ↗(On Diff #486)

Trailing space

This revision now requires changes to proceed.Feb 9 2017, 6:37 PM
In D49#4743, @elexis wrote:

But actually why should we do it in JS and not C++ when clearing ready? Saves us sending of a message and correcting a state uselessly.

Then we should probably completely remove g_IsReady, but this should be another patch/ticket...

Imarok updated this revision to Diff 502.Feb 10 2017, 4:38 PM
Imarok edited edge metadata.

Change stay ready color. Remove trailing withespace. Use g_IsReady as number everywhere.

elexis accepted this revision.Feb 10 2017, 4:52 PM

Still don't like those "== 2" checks, could be replaced with constants in JS and an enum in C++ sometime if one has time.
Also the option should be kept in mind in case some players or devs don't like this feature.

Great addition, looking forward to not click that button anymore!

This revision is now accepted and ready to land.Feb 10 2017, 4:52 PM
This revision was automatically updated to reflect the committed changes.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

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