[PATCH] Unify Chat Parsing

[Josh]

Currently we have three, mostly separate chat parsers. One for the multiplayer lobby chat, a second for the gamesetup chat, and a final third for the in-game chat. These three parsers should be at least partially unified and have feature parity.

[Matt Doerksen]

New file (functions_chat.js) for chat stuff. Currently only constants.

[leper]

You're breaking translations a lot with that patch. translate(foo + bar) doesn't work if foo+bar wasn't in another translate call as a string already.

[Matt Doerksen]

Replying to leper:

You're breaking translations a lot with that patch. translate(foo + bar) doesn't work if foo+bar wasn't in another translate call as a string already.

Can you clarify? I'm missing where/how things are broken since it's just wrapping what was already there.

For example, in gamesetup.js we have this:

var formattedUsername = '[color="'+ color +'"]' + username + 'color'; formatted = '[font="sans-bold-13"] ' + sprintf(translate("== %(message)s"), { message: sprintf(translate("%(username)s has joined"), { username: formattedUsername }) }) + 'font';

and what should be the equivalent of:

boldText(sprintfStr("== " + sprintfStr(username + CONNECT_TEXT)));

Breaking things down I have:

sprintf(translate("%(username)s has joined"), { username: formattedUsername })

which should be the equivalent of:

sprintfStr(username + CONNECT_TEXT)

since it just wraps the sprintf translate.

And then that gets "== " appended to it and is retranslated and bolded.

[trompetin17]

Replying to MattDoerksen:

Replying to leper:

You're breaking translations a lot with that patch. translate(foo + bar) doesn't work if foo+bar wasn't in another translate call as a string already.

Can you clarify? I'm missing where/how things are broken since it's just wrapping what was already there.

For example, in gamesetup.js we have this:

var formattedUsername = '[color="'+ color +'"]' + username + 'color'; formatted = '[font="sans-bold-13"] ' + sprintf(translate("== %(message)s"), { message: sprintf(translate("%(username)s has joined"), { username: formattedUsername }) }) + 'font';

and what should be the equivalent of:

boldText(sprintfStr("== " + sprintfStr(username + CONNECT_TEXT)));

Breaking things down I have:

sprintf(translate("%(username)s has joined"), { username: formattedUsername })

which should be the equivalent of:

sprintfStr(username + CONNECT_TEXT)

since it just wraps the sprintf translate.

And then that gets "== " appended to it and is retranslated and bolded.

There is a script that extract all "translate('text')" and get 'text'. thats why there are some "sprintf" functions to add "placeholder" variables.

Base in your changes it will break this script because you have translate("%(player)s" + CONNECT_TEXT_INGAME)".

And all your constant wont be translate neither.

[leper]

Any updates?

[Matt Doerksen]

I've been sidetracked with work for the past while. I'll get back to aggregating the messaging functionality hopefully this week.

[Matt Doerksen]

Move game setup chat functionality into its own file. Part one of aggregating and simplifying chat functionality.

[Matt Doerksen]

Move in-game chat functionality to functions_chat.js for aggregation.

[Itms]

Don't forget to follow the instructions detailed in SubmittingPatches, especially regarding the tagging of Trac tickets.

Thanks for working on this :)

[Matt Doerksen]

Includes content from 2640_1 and 2640_2 patches. This moves all chat functionality (gamesetup, lobby, ingame) to functions_chat.js.

[Itms]

Hello Matt, thanks for working on this.

Your patch is nice but it would be even better to have only one parsing function in chat.js, so all files would call the same addChatMessage function.

Maybe this is difficult to achieve nicely. Don't hesitate to ask for directions and feedback! :)

[elexis]

1) Notice that the chat functions are not identical: For example "%(username)s has joined" appears ingame while "%(player)s is starting to rejoin the game." appears in the lobby, similar the disconnect messages. Also there are many messages ingame that don't make sense in the lobby or gamesetup, like being attacked or changing diplomacy (unless we make it a new feature :-p). The gamesetup / ingame kick feature #3241 will also add code to these functions later.

Looking at the code, I think it is not possible to merge those functions. If you can find some similarities or identical code, those places should be unified. If there is actually nothing to unify, then there is nothing to code here and the ticket must be closed (although it would have been nice if we could unify it).

2) Also I noticed some changes in the beginning of lobby.js which are not relevant (maybe file ending?). When making a patch with a changed lobby.js I had the exact same problem btw.

[Itms]

Yes, it is a bit of a challenge (the idea is to rewrite these parsers in a unified way).

But even if it is too difficult, getting rid of the three different addChatMessageFoo is a must have.

[Matt Doerksen]

@ltms, @elexis Absolutely, the original idea was to compress them all, but like I was originally trying to do (and like elexis pointed out), there's some functionality that isn't nicely re-usable between the lobby, setup and in-game. So, what I've done for now is just to aggregate everything so it's no longer in three files.

I don't believe it's impossible to merge the three functions; it's just going to become very big (potentially slower, and definitely less maintainable). I'll start going through what I now have to see what can be pulled out into separate re-usable pieces (or at least smaller so the main functionality of chat is less obstructed).

[elexis]

leper had some thoughts on irc on how to refactor this succesfully: the specific logic can be done in handlers, for a rough idea take a look at the gui code for panels. check if it can be handled, if yes handle, else fall back to default. default might rely on some things that are specific to that chat, but uses the same naming/vars.

[Itms]

How far did you get? No pressure, I'm just going through the review queue to make it a bit less full ;)

[Itms]

I'm pushing this for now, but if you come up with a new version we can try to include it in A19. Thanks for working on this :)

[elexis]

Can anyone show me any kind of shared code? I see exactly one line (g_ChatMessages.push(formatted);) that is done equally in all three addChatMessage functions. I suggest closing this as won't fix, mission impossible.

[leper]

Have one object with commands per current chat code use, have the command parsing thing done in the common one. Mostly this ticket was about having the same features in all of the chat versions (eg /me).

[Imarok]

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