Added /help command, commands moved to unified global list.
Glad to see any suggestions about descriptions of commands.
Details
Details
- Reviewers
vladislavbelov - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) - Commits
- rP19475: Move lobby chat commands in a global object and add /help command to explain…
- Go to lobby
- Enter /help command
- Check that it's shown with a dependence on your role
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
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/751/ for more details.
Comment Actions
Looks good, nice structure!
We must test that it still works with the kick reason, can make you a temporary mod if you want.
| binaries/data/mods/public/gui/lobby/lobby.js | ||
|---|---|---|
| 214 ↗ | (On Diff #1222) | All chat commands. A (...) sent. |
| 218 ↗ | (On Diff #1222) | to 'Away' |
| 219 ↗ | (On Diff #1222) | fat arrow function |
| 240 ↗ | (On Diff #1222) | why not foo.privileges? |
| 241 ↗ | (On Diff #1222) | Will need a translation comment, otherwise transifex people don't know what the string means. \n shouldn't be part of the translated string. Oh wait, translate() missing too |
| 1264 ↗ | (On Diff #1222) | I've added those @params some time ago, but they are pointless. Comments should only be added if they add non-obvious information |
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/754/ for more details.
| binaries/data/mods/public/gui/lobby/lobby.js | ||
|---|---|---|
| 231 ↗ | (On Diff #1227) | ban should be right below kick. Logical order trumps alphabetical order IMO chat functions should be grouped too |
| 234 ↗ | (On Diff #1227) | Broken, kick is also supported if there is no reason given |
| 235 ↗ | (On Diff #1227) | (I guess someone would suggest using ...args, but this is good as is here) |
| 250 ↗ | (On Diff #1227) | Guess what's missing. Also tooltip.js has this bug, but we shouldn't it repeat here to end a string with a colon and use a sprintf placeholder instead |
| 252 ↗ | (On Diff #1227) | (Alphabetical ordering could be done here, but it's probably better for the user too if it's grouped logically, no?) |
| 255 ↗ | (On Diff #1227) | That command is the command and description is the description is self-evident. What transifex people can't guess from the string itself is where it is used, so the important part of you Translation comment is that it's about chat commands. So perhaps "// Translation: Chat command help format" or something like that might work. Translation and code comments should only contain unique information |
| 274 ↗ | (On Diff #1227) | Either make those privileges properties a boolean or disable chat command if the role is "visitor" for muted people |
| 971 ↗ | (On Diff #1227) | Didn't check the regex voodoo, but perhaps ircSplit could be nuked too then? Perhaps a new regex function used in both places if that is easily possible? |
| 1284 ↗ | (On Diff #1227) | colorizeLobbyChatCommand to avoid name collisions? |
| 219 ↗ | (On Diff #1222) | still arrow functions IMO |
Comment Actions
Added comments, function(args) replaced by args =>, commands disabled for muted people.
| binaries/data/mods/public/gui/lobby/lobby.js | ||
|---|---|---|
| 971 ↗ | (On Diff #1227) | Also ircSplit couldn't nuked so far it's used. Also it has a little bit different task. |
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/774/ for more details.
| binaries/data/mods/public/gui/lobby/lobby.js | ||
|---|---|---|
| 216 ↗ | (On Diff #1264) | g_ChatCommands seems nicer, in particular since g_SpecialCommands sounds like there might be non-special commands. Change was approved by Vladislav in irc. |
| 255 ↗ | (On Diff #1264) | format can be inlined. The translation comment still works, even if there is a sprintf before, as long as the comment is on the line above. Thanks for the better translation comment! |
| 257 ↗ | (On Diff #1264) | unneeded parenthesis |
| 971 ↗ | (On Diff #1264) | (The conjugation of split is split, split split) |
| 973 ↗ | (On Diff #1264) | This fancy parsiong would be good for a command line tool, but it is not used for any of the current commands and it actually reduces the usability of the /kick /ban commands. For example when typing /kick someDude You said "something bad" and then "something worse" therefore gtfo the kick reason will become "You said". We want everything as given from after the nickname (second space, if that space exists) |
| 971 ↗ | (On Diff #1227) | Since it is planned to hide the chat input altogehter for visitors, we don't need visitor checks anymore. They won't be able to /quit or /away manually anymore. Capitis Diminutio Maxima or so. Might be nicer to have g_ChatCommands[cmd] N times instead of N-1 times + one occurance of`cmd in g_SpecialCommands` (approved by Vladislav on irc today) |
| 1284 ↗ | (On Diff #1227) | bump. All the other colors are globals, so this one should be global too. |
Comment Actions
Would be nice to commit this, since the variable object is nicer to read IMO and I've already recommended the help command once when it didn't even exist
| binaries/data/mods/public/gui/lobby/lobby.js | ||
|---|---|---|
| 257 ↗ | (On Diff #1264) | If you agree with the proposed change in D339 that visitors don't have chat input field anymore (which I encourage you to do as they should expect to not be able to type chat anymore), then this should become a moderator boolean until XMPP specs change |
| 973 ↗ | (On Diff #1264) |
|
| 971 ↗ | (On Diff #1227) |
|
| 1284 ↗ | (On Diff #1227) | Nuke function + global color var |
| binaries/data/mods/public/gui/lobby/lobby.js | ||
|---|---|---|
| 255 ↗ | (On Diff #1264) | I don't want to call translate with same params for each command. |
| binaries/data/mods/public/gui/lobby/lobby.js | ||
|---|---|---|
| 255 ↗ | (On Diff #1264) | Not really a performance issue, it is cached in JS (and in C++ too): function translate(message)
{
var translation = g_translations[message];
if (!translation)
return g_translations[message] = Engine.Translate(message);
return translation;
}This function could even be simplified and the removal of creation of the variable could also be removed. (Don't know if the JIT is smart enough to do that on it's own). I wrote D390 so that the unneeded variable creation in translate vanishes too. A JS cache of a JS cache (of a C++ cache) isn't that useful, we don't do that in the overwhelming majority of translate calls, especially if the function is called maybe 3 times per game and is done in microseconds either way, so can just have it shorter too. To measure the exact number of microseconds that we would save, D190 could be used |
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!
http://jw:8080/job/phabricator/905/ for more details.
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!
http://jw:8080/job/phabricator/906/ for more details.
Comment Actions
Notice this assumes all commands start with slash, so "@mute" as proposed by scythetwirler won't work but should be listed in theory
(Also chat commands are bad because noone knows they exist, so noone will know about /help either.
There should be a help button or question mark icon button that shows chat commands and ideally implement one button for each command.)
| binaries/data/mods/public/gui/lobby/lobby.js | ||
|---|---|---|
| 255 ↗ | (On Diff #1264) | 1-2 microseconds for calling translate() 10 times, without D390 |
| 257 ↗ | (On Diff #1264) | Don't see how this was Done |
| 213 ↗ | (On Diff #1511) | Those are 2 sentences, so no comma but period. "if the command shouldn't be sent" kind of wrong, as the command is sent, but it is not sent as text after executing the command. Better return true if the chat should be sent, so that the reader doesn't have to think about the negation. |
| 217 ↗ | (On Diff #1511) | Why no capitalization + periods? |
| 234 ↗ | (On Diff #1511) | args[1] || "" is shorter than the ternary If statements should be avoided too where possible to minimize cyclomatic complexity and nesting. -> Engine.LobbyKick(args[0] || "", args[1] || ""); instead of if (args.length >= 1) Engine.LobbyKick(args[0], args.length >= 2 ? args[1] : ""); |
| 251 ↗ | (On Diff #1511) | (Self-evident that it's a list -> "Chat commands") |
| 252 ↗ | (On Diff #1511) | (If we don't inline those role Engine.LobbyGetPlayerRole to delete the local variables and don't care about performance, then we could have one global and update it once D339 is in) |
| 268 ↗ | (On Diff #1511) | s/usage/example since an example doesn't define the usage |
| 269 ↗ | (On Diff #1511) | args =>false |
| 271 ↗ | (On Diff #1511) | (This bloack should be merged with the "say" from the switch statement sometime) Why do we actually have /say? It will just raise question what that command is useful for and the answer will be nothing. But I recall asking that before 2015-12-03-QuakeNet-#0ad-dev.log: 22:00 < elexis> (uaaah, why do we have /say for the lobby) 22:01 <@leper> elexis: to /say /kick |
| 272 ↗ | (On Diff #1511) | It does not only display it but also sends the message to the room |
| 291 ↗ | (On Diff #1511) | At least move it above the code, so that we have a separation of data and logic. Why does the name Lobby have to appear in the variable name? There is only lobby chat in the lobby |
| 1028 ↗ | (On Diff #1511) | Why isn't there a role check if we already added that information? |
| 1033 ↗ | (On Diff #1511) | the command "foo" is not supported. |
| 1034 ↗ | (On Diff #1511) | (Not an issue, but having the style consistent with the sprintf calls would be preferable) |
| 971 ↗ | (On Diff #1227) | (Having g_ChatCommands[cmd] twice makes it easier to check as compared to two different terms) |
| 1284 ↗ | (On Diff #1227) | Removed function call but not the unused function |
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!
http://jw:8080/job/phabricator/917/ for more details.
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!
http://jw:8080/job/phabricator/918/ for more details.
Comment Actions
Should be, yes, we told about it, I want to split it, not add button and help together.