Use builder class for creating formatted chat messages

[mangstadt]

The ChatBuilder class encapsulates everything related to chat message formatting into a single class. This class is especially useful when logic is involved in the building of a chat message (like in the ServerListPlusCore.buildCommandHelp method) because the StringBuilder object you would normally use is encapsulated inside of ChatBuilder. The builder pattern also makes coding a little easier by leveraging the IDE's autocomplete functionality.

Modifying the ServerCommandSender.sendMessage() method signature to accept a CharSequence instead of a String would eliminate the need to call toString() at the end of every message. Doing this would still allow you to pass String objects into the method because String implements CharSequence.

[stephan-gh]

I personally don't see much of a difference between

help.append(ChatFormat.RED).append("/slp");

and

help.red().append("/slp");

it's minimally shorter but the above is at least as understandable as the below.

And the ChatBuilder kind of requires using the StringBuilder style everywhere now. In the other cases I kind of prefer the readability of the string concatenation with + over having .append() everywhere.

Overall, looking at the stat (+251 −46 lines) this seems to be making things more complex overall, not simpler. The ChatBuilder also mostly duplicates the definition in ChatFormat (including repeating all the magic format chars like c, f and so on).

[mangstadt]

The ChatFormat class could be deleted, since ChatBuilder handles all of the magic formatting codes.

I guess I'm thinking about it from an encoding perspective. Just like you wouldn't insert arbitrary text into an HTML page without encoding it first, having a separate class for creating chat messages allows you to encapsulate any special encoding schemes that may be required in one central location. To take an extreme example: if you later discovered that a certain character causes the Minecraft client to crash, all you'd have to do is edit the ChatBuilder.append method to filter out that character.

Come to think of it, I wonder if any checking should be done to ensure that the magic character is escaped if it makes its way into a chat message somehow (like an injection attack).

I'm also not a fan of using enums as glorified string constants, but I can't think of a technical reason why you shouldn't, so I guess it boils down to personal preference. :)

Just my two cents. Do with it as you will! :)

[stephan-gh]

If you would design this code from scratch in a modern way nowadays you should use a library like Adventure that represents the modern JSON chat components in Minecraft, with no magic characters, fully type-safe and with many additional features.

But, this is old code, made to cover a wide range of server implementations long before implementation-agnostic libraries like Adventure were made. I'm not convinced it's worth replacing this with something that still maintains the same underlying outdated approach.

I do feel kind of bad for using the "this is old code that better not be touched" argument so often, but the sad truth is that SLP is just that, for the most part. I would write so much of the SLP code differently nowadays, but I never managed to finish the SLP 4.0 rewrite. At this point it's basically "too late" because I have lost pretty much all interest in Minecraft. I'm only here because this project used to be very important to me and I don't want it to just die because of minor things.

This means that generally I appreciate your contributions, and I'm happy to merge them if they are straightforward enough and have a clear advantage. But I'm wary to take other large changes because if something breaks those might cause me a lot of work later in case something breaks.

And this patch represents something that one would probably do quite differently nowadays (e.g. with Adventure), but integrating that properly into SLP would require a full rewrite (which is frankly long overdue if you ask me). I hope that's understandable.

[mangstadt]

I did not know about Adventure. Looks like I was on my way to reinventing the wheel! :)

I understand about not wanting to break stuff, that makes total sense. I'd still like to trawl through the code for fun to see what else I can find if that's OK. I'll limit myself to small, straightforward changes only. Thanks for letting me know so I don't waste time making large changes!