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

§ Section Sign Exception #931

Closed TiffApps closed 1 year ago

TiffApps commented 1 year ago

Expected behavior

I coded a recursive /tpall command that uses MiniMessage to add a click component to ask for confirmation to the sender first (code below). MiniMessage should allow the use of the section sign § instead of throwing an exception, with maybe a warning at most (in some config/properties: warn-about-legacy-usage-of-section-sign=true by default, with whatever better name someone will come up with).

Observed/Actual behavior

image

Steps/models to reproduce

  /**
   * Builds a string that can be parsed by MiniMessage to add a run command click component to a piece of text.
   *
   * @param command The command to run.
   * @param text    The text that should have a click component added to it.
   * @return String that can be parsed by MiniMessage
   */
  public static String buildClickCommand(String command, String text) {
    String clickText = "<click:run_command:/%s>" + text + "<reset>";
    return clickText.formatted(command.replace("/", ""));
  }

@Override
  public void handle(CommandSenderAdapter sender, String[] args) {
    final PaperCommandSenderAdapter paperSender = (PaperCommandSenderAdapter) sender;
    final Player player = ((Player) paperSender.getCommandSender());
    final int len = args.length;
    final String keyword = "§confirm";

    if (len >= 1) {
      if (args[len - 1].equalsIgnoreCase(keyword)) {
        for (Player target : Bukkit.getOnlinePlayers()) {
          target.teleport(player.getLocation());
        }
        return;
      }
    }
    sendMessage(sender,
        buildClickCommand("tpall " + String.join(" ", args) + " " + keyword, "Are you sure?")
        );
  }

Plugin and Datapack List

LuckPerms, Utilities (custom Essentials), PluginCore (custom)

Paper version

This server is running Paper version git-Paper-540 (MC: 1.19.4) (Implementing API version 1.19.4-R0.1-SNAPSHOT) (Git: f9f9079) You are running the latest version Previous version: git-Paper-527 (MC: 1.19.4)

Other

NB: This is only about the wrong exception thrown when § is used outside of its legacy color code usage, not about how I could do an anti-player filter more efficiently etc... I don't really care as it's not that much of an issue with the very few people who would have the tpall permission + a modified client to allow typing the section sign + would know this cmd would work with a confirm keyword as well. And it would only need to bypass a simple click.

cc. @Machine-Maker

kezz commented 1 year ago

I'm going to close this issue as not planned as there is currently no good reason for us to modify the handling of section signs in MiniMessage. In your example, the client would be kicked if they were to run such a command anyway. Additionally, this is not a good reason for us to spend a good deal of time rewriting the parsing logic to handle this as entity/player selectors are already a thing that exist in vanilla Minecraft. Finally, Adventure also has click callback functionality so you don't even need to write your own system like this anyway.

tl;dr: Unless someone has a valid reason for needing section signs in MiniMessage that justifies the time that would be spent, we will not be making such a change.

TiffApps commented 1 year ago

Sometimes I really wonder why is it worth spending 30min writing an issue with repro etc when you guys don't even read correctly or until the end with the Nota Bene... Anyway, thanks for the quick answer I guess.

kezz commented 1 year ago

I did read your note and the issue as a whole. You mention in your note:

This is only about the wrong exception thrown when § is used outside of its legacy color code usage

The exception is not wrong - it says "Legacy formatting codes have been detected in a MiniMessage string" which is correct. In your example, legacy formatting codes were in the MiniMessage string, behaviour which is unsupported by MiniMessage at this time.

Additionally, you mention at the beginning of your message that "MiniMessage should allow the use of the section sign § instead of throwing an exception". This will not and never will be the case because we want MiniMessage to be a consistent format that is the same for all end users of the library. This is the same reason why we don't allow changing the tag characters, for example.

The only part of your issue that needed addressing was whether we should allow section signs in areas that won't be rendered as colour codes, such as the "run command" click action. That was where my response came from, that it is not worth us spending the time to fix such a minor issue, especially when there are myriad workarounds (e.g., use a different symbol, write a custom tag, replace after the component is created).