[PATCH] Highlight playernames in gamesetup and session chat

[elexis]

It is a bit unexpected that the name of the current player is not highlighted in the gamesetup if mentioned, since we all know that feature from the lobby. See

function addChatMessage(msg)
...
// Highlight local user's nick
if (msg.text.indexOf(g_Name) != -1 && g_Name != msg.from)
	msg.text = msg.text.replace(new RegExp('\\b' + '\\' + g_Name + '\\b', "g"), colorPlayerName(g_Name));

While we're at it, we could integrate it for the session chat too. functions_utility.js might be a good place to move the shared highlighting function, until we unified the chat somehow (#2640).

Notice that colorPlayerName returns the color of the name in the lobby and that the player color is different in the gamesetup / session.

Refs #2640 #3281

[leper]

functions_utility_chat.js would probably be nice (there should be some other related functions that could go there). Also note the comment 2 at #3281.

[Stan]

WIP patch.

[elexis]

This patch introduces the Engine function GetPlayerGUID, which returns the guid of the current player. One of the cleaner ways to check if a chat message was sent by a different player and to find out the name of the local player. Required for #3270 too.

[elexis]

• Doesn't address session chat.
• Splitting the text by spaces does not work, since playernames can contain whitespace too.
• Various whitespace issues. Don't add spaces to the function arguments.
• No need to create an array of colorized playernames since you can replace inplace
• The names of unassigned players / observers are not highlighted. You highlight all playernames, not only the one of the current user (which might be good too).
• The playername should be escaped as well.

[elexis]

1. Accesses g_PlayerAssignments and colorizePlayernameByGUID directly instead of passing it as an argument. This way it becomes more obvious which things you are calling there.

2. The first loop is not needed:

• playernames.push(player.name); is done in both cases of the if, so should be done only once outside of the if
• The if (player.player > -1) check is not needed, as colorizePlayernameByGUID already sets the correct observercolor (which might be different from the messagetextcolor).
• You don't need the first loop, since you can access the name and colorizedName inside that other loop directly by calling these functions.
• Notice one could also check for the names of offline players or bots, but that will open a whole new can of worms.

3. The second loop is broken:

• Your loop is broken as it replaces the user with the first GUID encountered and escapes the text left of it. In case of a name with a different GUID appearing before this, your loop would look inside the escaped text as well and thus might not find the playername.

4. Minor style optimizations:

• The "" argument of colorizePlayernameByGUID can be omitted, as it's equal to the default.
• Renamed variables.
• Missing spaces.
• Unused variable ret.
• Use of continue in a for-loop.

As seen in some other ticket, doing a simple replace can yield separate pitfalls, for example having both "username" and "username2". We also can't replace() if we want to escapeText the non-replaced text.

[elexis]

RegEx proposed by sanderd17: ​https://jsfiddle.net/n96gkx74/1/

Notice it first escapes the whole string, then replaces the escaped usernames in the escaped text. Im not font of the escapeRegExp function which uses a regex to escapes all characters which a regex can use.

I guess we need a third opinion.

[sanderd17]

The function to escape a regex is quite standard (and should probably be included in the RegExp prototype at some point). See ​http://stackoverflow.com/questions/3446170/escape-string-for-use-in-javascript-regex or ​https://developer.mozilla.org/en/docs/Web/JavaScript/Guide/Regular_Expressions#Using_special_characters I find the version with escaped special characters safer, and it's allowed to always escape special characters anyway (else you need to trust on the order of the characters to not create something special).

Using regexes is also a lot faster, as JS isn't very performant when it comes to concatenating strings (strings are immutable, so it just copies the memory on every concatenation). Just see the speed difference: ​https://jsfiddle.net/n96gkx74/2/

Results on my computer (FF 45):

• regex replacement on 1000 individual strings: ~18ms
• concatenation replacement on 1000 individual strings: ~70ms
• regex replacement on a concatenation of 1000 strings: 0-1ms
• concatenation replacement on a concatenation of 1000 strings: ~1600ms

Note that using the loop on a long string is really bad for performance, as there are a lot of concatenations happening, and every time the string is copied. While long strings are just beneficial for regex, as it doesn't need the overhead of the loop around it.

[elexis]

The number of string concatenations in my code can be reduced to the number of playernames occuring +1 by just increasing a numerical index instead of concatenating each character.

Few remarks by leper on 2016-04-23:

23:38 <@leper> sanderd17: "The nice thing about regexes, no matter how complex you make them, once they're compiles, their run time is linear to the length of the source string." [citation needed]
23:40 <@leper> now that is one regex that makes one appreciate sed, given that you can use everything instead of / and don't need to escape anything (not that that would make things a lot more readable)
23:42 <@leper> elexis: both are kinda ugly, do we really care if some highlights don't work if the users are trying hard to be stupid?
23:43 <@leper> the trac patch could just count up until it finds something that it would consider the start of some string, and only if that was an actual match get the string up to that point
23:45 <@leper> elexis: should probably also use Knuth-Morris-Pratt since that nearly does what is wanted

​https://en.wikipedia.org/wiki/Knuth%E2%80%93Morris%E2%80%93Pratt_algorithm

Less likely to have name conflicts in a game, but in the highlighting of lobbychat (post r17041) I noticed them a couple of times (so would be nice to do it properly).

(Notice playernames can currently contain arbitrary characterset (for example chinese names) including backslash, but that messes with many places in the code)

[sanderd17]

Proof for regex running in linear time is easy: RegExp defines the regular languages, which is equal to DFA's which run in linear time by definition.

So once a RegExp is compiled to a DFA, it's linear. But ofc, that doesn't say anything about how it's implemented in Spidermonkey. But the timings I gave seem to point in the direction that the implementation is quite good.

And the problem with concatenation isn't only the number of concatenation, it's also the length of the concatenated strings. As you need a copy every time you concatenate, the longer the string is, the longer a single concatenation operation takes. So the code looks linear (only looping over the string once), but with the concatenation operation being also linear, there's some sort of hidden nested loop.

The KMP algorithm is nice, but it's only a string search algorithm, while the difficulty is in the replacement part. And I'd prefer to not reinvent the wheel, and use some methods that already are available in the engine (and very well optimised for JIT).

[elexis]

Taking out of the review queue since we don't have a commitable patch using regex and the other one should be simplified.

In case we go with the non-regex version, that would have to use a numerical increase instead of contatenating the every character individiually in case its not a name. Also, since the GUIDs don't depend on the chatmessage, the runtime for that patch is linear as well. The regex is shorter and might be as unreadable as the non-regex one.

Both versions should be committable, just need to be polished.

[elexis]

In 18534:

Submit and display more information about matches in the lobby. Patch by Imarok, refs #3476.

Includes team numbers, online/offline- and won/defeated state, AI type and difficulty for running games and
only the playernames with observer-player distinction in the gamesetup.
Use JSON format inside the XML stanza and minimize traffic by packing teams.

Use the observer distinction to correctly apply the "full games" trigger in the lobby, fixes #3143.

XPartaMupp patch applied by scythetwirler.
unescapeText function by sanderd17, refs #3409.

[elexis]

Backlogging due to lack of progress.

[Imarok]

Move tickets to Misc. UI as "UI & Simulation" got some sub components.