Page MenuHomeWildfire Games

PM hotkey
ClosedPublic

Authored by OptimusShepard on Feb 24 2017, 10:37 PM.

Details

Reviewers
elexis
Commits
rP19272: Private chat hotkey.
Trac Tickets
#4422
Summary

With "^" you open a privat chat with the player you have spoken at last.

Test Plan

-> 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

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

fix incorrect spelling

Imarok added a subscriber: Imarok.Feb 24 2017, 11:08 PM

[22:54] <elexis> chatAddressee.selected isnt a good identifier
[22:54] <elexis> breaks if a client joins/leaves
[22:55] <elexis> gotta remember nick

Sandarac added inline comments.
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
But the braces aren't needed for this one statement.

Vulcan added a subscriber: Vulcan.Feb 25 2017, 1:16 AM

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.

elexis requested changes to this revision.Feb 25 2017, 5:35 AM
elexis added a reviewer: elexis.
elexis added a subscriber: elexis.

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
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?

232 ↗(On Diff #602)

We have { on a separate line and no pair of braces if there is only one statement in the scope

elexis requested changes to this revision.Feb 25 2017, 5:36 AM
This revision now requires changes to proceed.Feb 25 2017, 5:36 AM

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?

so should I now change the function from "previously private chat" to "previously selected chat?

Yep, that should work as a config name

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.

No problem, I didn't start with the revision.

OptimusShepard edited edge metadata.
  • 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
Imarok added a comment.Mar 4 2017, 3:55 PM

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

elexis added a comment.Mar 4 2017, 4:08 PM
In D163#7235, @Imarok wrote:

I'd remove that switch case and directly pass command to openChat.

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.

replace switch-case

Imarok added inline comments.Mar 4 2017, 4:19 PM
binaries/data/mods/public/gui/session/messages.js
32 ↗(On Diff #708)

[16:17] <elexis> also JSdoc comment for that global

OptimusShepard marked an inline comment as done.

change global comment

elexis added a comment.Mar 4 2017, 4:46 PM

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") ?

OptimusShepard marked an inline comment as done.

replace check

elexis awarded a token.Mar 4 2017, 5:52 PM
elexis accepted this revision.Mar 4 2017, 5:56 PM

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).

This revision is now accepted and ready to land.Mar 4 2017, 5:56 PM
elexis requested changes to this revision.Mar 4 2017, 6:21 PM
elexis removed a reviewer: Imarok.

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

This revision now requires changes to proceed.Mar 4 2017, 6:21 PM
OptimusShepard edited edge metadata.
OptimusShepard marked an inline comment as done.

update of the intro.txt and the updateHotkeyTooltips
thanks for your help

elexis accepted this revision.Mar 4 2017, 10:06 PM

(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
->
Toggle chat window and select the previous private chat partner

binaries/data/mods/public/gui/manual/intro.txt
65 ↗(On Diff #712)

Sent -> Send
+ the
last -> previously
+ parnter

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
last -> previously
+ partner

36 ↗(On Diff #712)

space in newline

binaries/data/mods/public/gui/session/session.js
391 ↗(On Diff #712)

last -> previously

This revision is now accepted and ready to land.Mar 4 2017, 10:06 PM

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.

This revision was automatically updated to reflect the committed changes.

Thanks for the patch!