Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update for RGB #17

Closed
wants to merge 2 commits into from
Closed

Update for RGB #17

wants to merge 2 commits into from

Conversation

Narimm
Copy link

@Narimm Narimm commented Sep 4, 2020

Updates to support rgb

}
for (char color : ansiCodes.keySet()) {
String cString = COLOR_CHAR + String.valueOf(color);
String escCode = ansiCodes.get(color);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be changed with looping ansiCodes.entrySet instead.

public void testRgbColor() {
assertEquals("World",format("§x§F§F§0§0§5§5World",false));
assertEquals("\u001B[38;2;255;0;85mWorld",format("§x§F§F§0§0§5§5World",true));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty line

@Narimm
Copy link
Author

Narimm commented Sep 6, 2020

PaperMC/Paper#4221

@zml2008
Copy link

zml2008 commented Sep 6, 2020

I'm not a fan of adding support for spigot's non-standard format directly to TCA -- a TCA PR should really just remove TCA's dependence on MC's legacy text formatting.

@Narimm
Copy link
Author

Narimm commented Sep 7, 2020

I'm not a fan of adding support for spigot's non-standard format directly to TCA -- a TCA PR should really just remove TCA's dependence on MC's legacy text formatting.

What if Paper forks TCA

@zml2008
Copy link

zml2008 commented Sep 9, 2020

I'd really rather not -- a number of projects use TCA -- forge for one -- so any improvements made should really be designed to benefit them as well.

@stephan-gh
Copy link
Member

I agree with @zml2008. Unless other projects like Sponge, Forge, ... agree on the same convention I would rather not support the non-standard Spigot format here.

Actually, the only reason MinecraftFormattingConverter exists in TerminalConsoleAppender at all is that back when I integrated better console support into Paper/Forge/SpongeVanilla/... all the projects needed exactly the same and I did not want to duplicate the code everywhere. In general, TCA is supposed to entirely independent of Minecraft so preferably Minecraft-specific code should not be here at all.

If the code can no longer be shared in a reasonable way it's really better to just put special converters into the projects itself - there is really not much of a difference because it's so easy to hook into Log4j2 with the annotations.

@electronicboy
Copy link

the code can no longer be shared in a reasonable way it's really better to just put special converters into the projects itself - there is really not much of a difference because it's so easy to hook into Log4j2 with the annotations

I'm inclined to ask for this, 'lest I go break the existing log4j configs people are using to add a bukkit RGB aware impl of this class

@stephan-gh
Copy link
Member

For the reasons already mentioned above (and also mentioned in #18) I will close this. However, @Narimm thanks a lot for making the PR! I believe something like your code made it into Paper as part of PaperMC/Paper#5205.

As inspiration for all others who also want to add RGB console color to their platform, I've also documented a potential quite different approach in #18 that would avoid legacy color codes entirely. I think this would be the ideal solution but I don't really have time at the moment to investigate it myself.

@stephan-gh stephan-gh closed this May 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants