With "^" you open a privat chat with the player you have spoken at last.
Details
Details
- Reviewers
elexis - Commits
- rP19272: Private chat hotkey.
-> open the chat in witch way you want
-> Select a privat player (only privatchats, others are ignored)
-> write your text and enter
-> the next time you want to write a private message to the same person your open the chat with "^"
if you open the chat with "^" bevore you have ever write a privat message, it will open the normal chat without selecting
if you select an other player, the next time you open the privat chat it will select the new chat to the last spoken player
Sorry for my english ^^
Diff Detail
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
[22:54] <elexis> chatAddressee.selected isnt a good identifier
[22:54] <elexis> breaks if a client joins/leaves
[22:55] <elexis> gotta remember nick
| binaries/data/mods/public/gui/session/menu.js | ||
|---|---|---|
| 209 ↗ | (On Diff #603) | There shouldn't be a space after teamChat = false |
| 230 ↗ | (On Diff #603) | Opening braces should start on a new line: http://trac.wildfiregames.com/wiki/Coding_Conventions#Formatting |
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/403/ for more details.
Comment Actions
See rP18369 for an example of persisting the selected addressee choice. That patch also adds a placeholder item if the user went offline (so that it doesnt select something else, as it often occured that players typed team chat, someone went offline, the item selected the default public choice, and then they sent their teamchat publicly). It sounds like we should reuse some of that code (while trying to avoid copying it by using a helper function and using the new global variable in both cases or something). Using the placeholder if the previously selected guy is offline doesn't sound wrong.
| binaries/data/config/default.cfg | ||
|---|---|---|
| 170 ↗ | (On Diff #603) | That sounds like the US guys wouldn't be able to use it. I know we have a shortage of unassigned buttons, but if you don't find anything suitable, you could make a rarely used existing hotkey a hotkey combination, like Alt+Foo instead of Foo and then use Foo here. |
| binaries/data/mods/public/gui/session/menu.js | ||
| 232 ↗ | (On Diff #602) | We have { on a separate line and no pair of braces if there is only one statement in the scope |
| 38 ↗ | (On Diff #603) | g_LastChatAddressee |
| 209 ↗ | (On Diff #603) | If this were a string argument having the possible balues "team", "public" and "private" would exclude argument combinations that don't make sense (true, true for example). Hypothetically we might want to extend this in the future (observer hotkey for players), then that problem would increase. |
| 231 ↗ | (On Diff #603) | This persistance can be done in updateChatAddressees then I guess. Also the "3" check is suboptimal as that manifests an assumption which could change in the future (rather had a check for a human readable string, or better avoid it altogether as far as possible) By removing the check, pressing the hotkey would mean the previously selected addresse will be reselected, so if public was selected public will be selected again, if observers are selected, observers will be selected again, if Imarok was selected, Imarok will be selected again. Sounds even more useful than the original ticket description. If you change person to addressee, then it's not only more consistent with the phrasing otherwise, but also describes this function correctly, right? |
Comment Actions
First of all, thank you for all your feedbacks. I will try to realize your hints and advices.
elexis so should I now change the function from "previously private chat" to "previously selected chat?
Comment Actions
Use case 1: Having a PM conversation with someone and a team or public conversation simultaneously
Use case 2: Using the proposed hotkey to reopen the last thing selected without having to worry whether to press T or enter (as that is sometimes confused) nor having to select the player again
As we can implement a hotkey for only one of those 2 use cases and as Imarok agrees that use case 1 is more important, I have to revert my suggestion to remove that > 3 check and keep it a "player only" hotkey.
The check to skip the first 3 elements can be replaced with a more readable check for /msg (as you have to check for the content of the selected dropdown item anyway). Sorry for the inconvenience.
Comment Actions
- open private chat with "L"
- change the saved number of the last selcted person to string
- change the boolean from teamChat and privateChat into String
Comment Actions
I'd remove that switch case and directly pass command to openChat.
| binaries/data/mods/public/gui/session/menu.js | ||
|---|---|---|
| 228 ↗ | (On Diff #706) | nope |
Comment Actions
That means having two global variable names in the XML files and people forgetting to look for xml files when doing renames, but I'm okay with the recommendation to avoid the switch actually. Patch looks good.
| binaries/data/mods/public/gui/session/messages.js | ||
|---|---|---|
| 32 ↗ | (On Diff #708) | [16:17] <elexis> also JSdoc comment for that global |
Comment Actions
Code also looks like it works with people who went offline too, see updateChatAddressees.
While at it might want to change selectedName.substr(0, 4) == "/msg" to startsWith too.
Your code only saves the last selected addressee when sending a message to him/her.
This means when selecting a different one but not sending the message, it will not recall the selection. Is that the right choice from a use case analysis pov? Probably isn't wrong.
| binaries/data/mods/public/gui/session/messages.js | ||
|---|---|---|
| 759 ↗ | (On Diff #710) | selectedChat.startsWith("/msg") ? |
Comment Actions
Take care, when changing methods you have to confirm that all occurances are updated. You forgt one openChat(); one in menu.js that is triggered when clicking on the menu item. (I should have done a complete review instead of posting one observed issue each time you updated the patch).
Other than that I accept the patch, give you a big thanks and let Imarok do the testing and committing (I guess Imarok can insert the empty string there when committing).
Comment Actions
We decided I can finish this review xD
Tested and works very well, also with offline chat partners.
But I did recall one more addition that should be done: updateHotkeyTooltips in session.js should mention the new hotkey, hoping that players will read it eventually.
Also I'm surprised I didn't post it before, you need to extend intro.txt and update http://trac.wildfiregames.com/wiki/HotKeys equally once it's committed
Comment Actions
(Confirmed the new strings with Imarok in the alpha 21 game you had played an hour ago + in the lobby)
Also d'accord with not replacing the startsWith(0, 4) == "/msg" occurance with a startsWith as suggested above, as having it this way makes the selectedName.substr(5) check more readable
Thanks for the patch!
| binaries/data/config/default.cfg | ||
|---|---|---|
| 170 ↗ | (On Diff #603) | Toggle chat window with last selected private chat |
| binaries/data/mods/public/gui/manual/intro.txt | ||
| 65 ↗ | (On Diff #712) | Sent -> Send |
| binaries/data/mods/public/gui/session/hotkeys/misc.xml | ||
| 8 ↗ | (On Diff #712) | Decided with Imarok in that alpha 21 game you played that we use "" as a default argument, as we have two occurances with the one in menu.js |
| 15 ↗ | (On Diff #712) | spaces -> tabs, whitespace in empty lines should be removed |
| binaries/data/mods/public/gui/session/messages.js | ||
| 33 ↗ | (On Diff #712) | + Command to send to the |
| 36 ↗ | (On Diff #712) | space in newline |
| binaries/data/mods/public/gui/session/session.js | ||
| 391 ↗ | (On Diff #712) | last -> previously |
Comment Actions
Private chat hotkey.
Going to commit this once the repository server works again:
Private chat hotkey.
Patch by: OptimusShepard
Differential Revision: https://code.wildfiregames.com/D163
Fixes #4422.