Page MenuHomeWildfire Games

Colorize username in chat when is ready and leaves match

Authored by Silier on Mar 23 2020, 10:14 PM.



When user leaves lobby he is colorized also when he clicks is ready button in a23b.
After rP23374 it is not case anymore.

@elexis since it was your patch, do you agree we should revert to that behaviour or are you strongly against ?

Test Plan

Agree it is nice to have them colorized.

Diff Detail

rP 0 A.D. Public Repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Silier created this revision.Mar 23 2020, 10:14 PM
Owners added a subscriber: Restricted Owners Package.Mar 23 2020, 10:15 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build:

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.

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

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

I wanted to avoid copying the same function to another two classes.
That line was not intentional. I planed to remove it in next update or commit.

I already tested changes.

Silier updated this revision to Diff 11648.Apr 8 2020, 8:29 PM

remove global keyword
revert empty line change
make ready message bold for consistency with other non player written messages

Vulcan added a comment.Apr 8 2020, 8:33 PM

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

Link to build:

nani added inline comments.Apr 8 2020, 8:35 PM
23 ↗(On Diff #11648)

,this -> , this :)

Silier updated this revision to Diff 11651.Apr 8 2020, 9:22 PM

space after comma

Vulcan added a comment.Apr 8 2020, 9:26 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build:

elexis accepted this revision.Apr 9 2020, 1:00 AM

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.

This revision is now accepted and ready to land.Apr 9 2020, 1:00 AM
Silier added a comment.Apr 9 2020, 9:49 AM

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