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.
Details
Details
- Reviewers
elexis - Commits
- rP19216: Stay ready button
- Trac Tickets
- #4369
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
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
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.
Comment Actions
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.
Comment Actions
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
Comment Actions
- 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?
Comment Actions
However the layout shouldn't determine which controls we can use, the layout has to adapt to our requirements.
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:
- A player accepts settings by default (and wants to avoid clicking on ready).
- A player accepts settings by default, but sometimes wants to revoke the ready state.
- A player accepts settings only after careful examination.
- 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:
- 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.
- 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:
- 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.
- 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.
| 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). |
| 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. |
| binaries/data/mods/public/gui/gamesetup/gamesetup.js | ||
|---|---|---|
| 1864 ↗ | (On Diff #161) | probably |
Comment Actions
Done everything besides the stay ready message (will be a new simple ticket) and the enum (not needed).
Comment Actions
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.
Comment Actions
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) | ! -> . |
| 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 |
| binaries/data/mods/public/gui/gamesetup/gamesetup.js | ||
|---|---|---|
| 148 ↗ | (On Diff #252) | sure? |
| binaries/data/mods/public/gui/gamesetup/gamesetup.js | ||
|---|---|---|
| 148 ↗ | (On Diff #252) | No, but it appears to be a https://en.wikipedia.org/wiki/Conjunction_(grammar)#Subordinating_conjunctions |
Comment Actions
Fix remarks.
(Don't add a comma see e.g. http://www.chompchomp.com/terms/subordinateclause.htm)
| binaries/data/mods/public/gui/gamesetup/gamesetup.js | ||
|---|---|---|
| 148 ↗ | (On Diff #252) | yeah and between main clause and subordinate clause comes no comma. |
| binaries/data/mods/public/gui/gamesetup/gamesetup.js | ||
|---|---|---|
| 48 ↗ | (On Diff #484) | You removed the check but didn't add the empty string here, sounds like breakage |
| 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 :) |
| 713 ↗ | (On Diff #161) | The < 2 check is unneeded, right? as that function will return an empty string and that wont print anything |
| source/network/NetServer.cpp | ||
| 779 ↗ | (On Diff #484) | p -> assignment? |
| 776 ↗ | (On Diff #252) | thanks |
Comment Actions
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.
| 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 |
Comment Actions
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.
Comment Actions
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 |
Comment Actions
Then we should probably completely remove g_IsReady, but this should be another patch/ticket...
Comment Actions
Change stay ready color. Remove trailing withespace. Use g_IsReady as number everywhere.
Comment Actions
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!
Comment Actions
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.
Comment Actions
disadvantage of consuming a bit more time to revoke the readystate than a singleclick
Solved by reseting upon rightclick.