Agree it is nice to have them colorized.
Thank you for picking up nani's report and writing a patch for it!
On the feature description of this diff:
It wasn't obvious to me whether it was an oversight or intended.
I remember two aspects while working with that code:
- The status chat messages are not fully consistently formatted. For example: Ready message is not bold, but Join messages are
- One of the differential revision iterations of D2483 had that ClientReady message worng. (It had shown "(playerID) is ready").
So it seems like it indeed was an oversight and not intended to drop the color.
Style consistency for those who opt into that:
- No global keyword (not done elsewhere except when its not possible to avoid it, or in music.js which has always had unique style, grep -R global | grep js | sort).
- Not that newline.
I didn't apply the patch but I assume it is easy enough to test before committing.
The mistake could have been systematic and one should also not only consider this gamesetup commit, but the current code base without prejudice from possible historical debt.
- One consistency that had been there since many years(?) is that the Join message is not colorized, but the Leave message is.
That must be due to assigning the player from -1 to an empty slot after the Join chat message.
That could be changed by broadcasting the Join in the place after the empty-slot assign routine had been performed to make events explicit, ensuring the order of events (instead of intransparently and fragily changing the order of subscription insertion).
- Related chat formatting inconsistency was mentioned in the memories above ("ready message is not bold, but Join messages are"). I didn't decide about that.
(I didn't check further inconsistencies, they could be found by comparing all chat message types, perhaps also in the session. You can chose the scope and what you commit as long as its not bugged and reasonable. Also I offer the whatever card.)
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... Executing section JS... | | [NORMAL] ESLintBear (comma-spacing): | | A space is required after ','. |----| | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/Pages/GameSetupPage/Panels/Chat/ChatMessages/ClientReady.js | |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/Pages/GameSetupPage/Panels/Chat/ChatMessages/ClientReady.js | 20| 20| return; | 21| 21| | 22| 22| this.args.username = colorizePlayernameByGUID(message.guid); | 23| |- this.chatMessagesPanel.addText(setStringTags(sprintf(text, this.args),this.MessageTags)); | | 23|+ this.chatMessagesPanel.addText(setStringTags(sprintf(text, this.args), this.MessageTags)); | 24| 24| } | 25| 25| }; | 26| 26| Executing section cli...
Dunno if the "is ready!" substring should be bold too, I guess it's a bit arbitrary. One can be conservative unless one discovered explicit reason to change something (as was done in the class rewrite). I didn't compare all chat messages, you can decide, patch seems fine otherwise. I suppose I can risk an acceptance without having tested. Thank you again for writing the patch.
My reasoning was that every other message in chat about status of player or setting changes is bold and only what player types is not.
18:10 < nani> just add it the way is consistent and easy to read 18:23 < Freagarach> It stands out nicely, which is good since it an important informative message :) 18:24 < nani> I prefer no bold but whatever bold also looks nice