rework of the color code with & and standard codes

[tonitch]

This is a draft for the rework of the chat, as explain in #5287 (no response so I assume this should be fine)

I want to make it as close as common server as possible so I want to follow this image and so using the & symbol

[tonitch]

I think I'm ready for review,
I hope that you will understand my changes. I think it is quite important that we follow standard of minecraft, even community's one, so people are not disturbed by it

I tried to replace every bit that uses style in the code with this new guideline there is only one part I'm not sure...

cuberite/src/ClientHandle.cpp

Lines 1563 to 1568 in 3c75377

	cCompositeChat Msg;
	AString Color = m_Player->GetColor();
	if (Color.length() == 3)
	{
	Color = AString("@") + Color[2];
	}

I'm not sure if ranks color are implemented and so how should I change that... I haven't seens any way of setting the color of a rank....

[tonitch]

Is there any update on the review ?

[madmaxoft]

The Server/Plugins/APIDump/APIDesc.lua also needs to be updated - there's a cCompositeChat description there. What about plugin compatibility, did you test any plugins actually using chat colors?

Also, what about cChatColor, do we want to keep it? Is it up to date?

[tonitch]

The Server/Plugins/APIDump/APIDesc.lua also needs to be updated - there's a cCompositeChat description there.

I did my best, english is a foreign language for me so I might not be clear in my way of explaining it... sorry :)

What about plugin compatibility, did you test any plugins actually using chat colors?

I didn't, I honestly didn't even tried plugin once since I'm in the cuberite project... So I don't really know which would break except for Core for whom I submitted a patch
I can maybe do a deprecated backward compatibility for colors as it uses @ and can be detected but for other style effect I don't think it's worth the effort...

Also, what about cChatColor, do we want to keep it? Is it up to date?

I didn't even knew it was a thing, by the look of things this is used by Cuberite/Core... I think it's not a bad thing that both exist as this make an altenative way of coding plugins. either typing the color or going for the standardized minecraft color code
and I also think that if plugin compatibility is a concern, then making cChatColor deprecated at the same time would break every single plugin that uses chat color

[12xx12]

I think we should check the plugins released with the cuberite github profile (check the Lua-Repos that are not archived)

[12xx12]

Just bumping @tonitch to check if the other plugins were checked.
These changes are good.

[tonitch]

Just bumping @tonitch to check if the other plugins were checked.
These changes are good.

Sorry I didn't had much time, and will not have for the next two weeks... I will do that as soon as I can!
Thanks for the bump 😊

[tonitch]

I'm back! Here is a list of what I checked and what I did!

checked

• Core - Colorcode fix #5416@cuberite Core#247
• MagicCarpet - nothing
• Gallery Correcting color code for future update gallery#73
• ChatLog - nothing
• Essentials - Nothing (uses sendMessage{info/success/warning/fail/...})
• WorldEdit - Nothing (uses cChatColor)
• HungerGames - Nothing (uses cChatColor)
• APIFuzzing
• ProtectionAreas
• Coiny - Have to modify can't make a pr as this repo is archived
• Trady
• transapi
• transapi-client
• SpawnProtect
• Handy
• ChunkWorx
• ChestShop
• Commandy

[tonitch]

All seems good to me ^^
Tell if I need to do anything else ?!

[tonitch]

i'm sorry if i'm rude but will this be pulled someday ? ^^

[12xx12]

I'm handing in my bachelor thesis on Wednesday. Then your time has come

[tonitch]

Super! I can wait more, I was just a bit in the dark here ^^
Have fun and best of luck to you!

[12xx12]

So in total, the changes look great. I have some minor comments.

[12xx12]

This is good to go except we need to update the plugin submodules. So the plugin updates need to be merged first. Maybe @tigerw @bearbin or @NiLSPACE can merge those

[mathiascode]

I can't review this PR right now, but I can merge some of the plugin PRs once this PR is merged.

A few questions:

• I assume plugins using the old formatting/color codes keep working without any fatal errors?
• Existing motd messages that use the old color codes will break once the Core PR is merged. Do we care about this?

[12xx12]

Nothing will break (in the sense of the application crashing). Using the old color Codes will just result in a weird & character in messages.

Since we provide a proper API to methods in cCompositeChat this might have been dodged.

Since I am writing this on mobile, so I can't check if those methods will stop working (no formatting) because some indicators were changed

Edit: since the codes for the bold, strike, italic,... are changed to other Charakters, this might break. @tonitch did you check this?

[tonitch]

All plugins using the API are fine ... The only thing that would break is if plugin writer have implemented color code in the form of "@x"... These character will just be untouched and might be weird but as asked, I checked every plugin in the cuberite's GitHub and I found really little amount of dev using @ char in their code... They almost all use api and so are fine
Except those i made a PR ofc

[12xx12]

I meant if a plugin uses 'AddChatPartStyle' but uses the old keys. Then there would be no formatting without a warning.

[12xx12]

Maybe we should add a switch/case case that triggers on the old formatting codes informing the admin that the cold is outdated

[tonitch]

Maybe we should add a switch/case case that triggers on the old formatting codes informing the admin that the cold is outdated

If I remember correctly, I though about that but I'm pretty sure there is conflict... Maybe I can do that only for those that are no more. Should i do anything else for the conflict ?

[12xx12]

I don't think so. The new character should be noticed fast :D

[tonitch]

I'll do warnings then, but there should be a way of handling deprecation in cuberite... Like when a function is deprecated after x commit/version/month, this warning can be left out. I think this would avoid long portion of code that would literally do nothing 🙂

[12xx12]

Yeah the deprecation handling is quite inconsistent and since we don't do releases, no dates are set.

The requested changes were applied

[12xx12]

Last one. Then I'll approve

[12xx12]

Waiting a day for any vetos

[bearbin]

Looks fine generally, I will not veto. I'm not totally confident with your refactor to the chat parsing logic, you seem to have made more changes than strictly necessary here and it's some complicated spaghettti with intrinsically unsafe data structures.

In an ideal world that whole mess would be refactored with modern C++ and use some safe primitives. But that's a bigger job, this is fine to get finally merged in.