KyoriPowered / adventure

A user-interface library, formerly known as text, for Minecraft: Java Edition
https://docs.advntr.dev/
MIT License
679 stars 105 forks source link

feat: abstract legacy code conversion #903

Closed Camotoy closed 1 year ago

Camotoy commented 1 year ago

Bedrock already has one new "legacy" color code, "Minecoin gold". In 1.19.80, there will be a handful of new legacy color codes. The primary goal of this PR is to allow LegacyComponentSerializer usages to take advantage of these new color codes for hex color downsampling. While designing this PR, I also wanted to allow other non-color codes to be modified, since Bedrock doesn't have underscore and obfuscation. We already work around this, however, so this feature is not important to the PR.

For now, I only request feedback on the general structure of the PR. There are several Javadoc and checkstyle errors that I have yet to address until it is agreed upon that the implementation presented here is acceptable. Also, this PR has not yet been tested, for the same reason.

Joo200 commented 1 year ago

First of all I don't think we should support or add more "legacy color codes". It's legacy because it's outdated and shouldn't be used anymore. New types can be added to MiniMessage with more possibilities and richer formatting.

Imo your changes are limited and adding new types is pretty complicated. A better approach could be an interface (LegacyCodeConverter) which converts the string to a style. E.g.

public interface LegacyCodeConverter {
  /**
   * Convert the text with the legacy char to a format.
   * return: the style, null if no color can be determined.
   */ 
  TextFormat tagToFormat(final char legacy, final String input, final int pos);

  /**
   * Convert the TextFormat to a message.
   * return: a message, null if the color can't be formatted correctly
   */
  String formatToTag(TextFormat format);
}

This would create a few classes more but you would only need to store a List of Tag types and can add simply more formatting tags.

~~But: The more I read that code and think about that topic I disagree on expanding the Legacy Converter logic. Keep it limited, keep it stupid, let's move people to MiniMessage. My suggested changes are already implemented in MiniMessage (with TagResolvers) and there is no need to manage multiple Converters with the same possibilities.~~

Edit: Just got the message that Bedrock doesn't use components at all. Ignore my blame of legacy stuff here.

Camotoy commented 1 year ago

Implemented kashike's suggestions. I don't think this is optimal, but maybe a step in the right direction. It might actually be possible to completely get rid of LegacyCodeConverterImpl now, but the legacyFormat method needs a home.

Camotoy commented 1 year ago

Joo's suggestion is also possible, but implementing such a design would have to account for the different hex character styles and whatnot.

Camotoy commented 1 year ago

I gave this some thought, and I think this could be implemented much more cleanly: https://github.com/Camotoy/adventure/tree/allow-custom-colors-legacy

This removes the LegacyCodeConverter class and has LegacyComponentSerializerImpl process the custom formats.

Camotoy commented 1 year ago

Superseded by #906.