From 1b2830a3b3aa5ecf4d2f44696654fc028a2c53a0 Mon Sep 17 00:00:00 2001 From: Aikar Date: Mon, 1 Jun 2020 19:19:42 +1000 Subject: [PATCH] SPIGOT-4441: Fix serializing Components to and from Legacy While cfeef75cd97 might of semi helped being able to save black text lore, it actually took a fundamental problem with the legacy serialization code and expanded it to break even more aspects of the server when dealing with Component to Legacy conversion. This is causing data loss in Spigot with cases such as setting an item name to white gets stripped resulting in it being italic. Additionally, things such as book pages have been returning black formatting codes for the end of the line even when the user doesn't have colors in the book. The root issue is that the "Default Color" system is fundamentally wrong. Components do not and should not care about what element of the game they are being used by, and that's what the default color system did. It results in components that if obtained from 1 source such as a Book where the default / rendered color is black, is then copied to another source such as an Entity name, the black is carried forward and shown in the Entity name, when in reality it should have been white. This commit reverts cfeef75cd97 and fixes the underlying serialization issues when it comes to Legacy to and From conversions. There was quite a number of issues with this code overall, in how it handles inserting color codes, new line parsing and such. Books was using mojangs own "getLegacyString" which doesn't match behavior. We also do not want to use Mojangs method as there is no guarantee they don't remove that in future. Plus, everything about books uses the CB implementation anyways, and it should be consistent (this was mandatory to avoid serialization format changes on old vs new) These changes as is results in Item Stacks already serialized will not change contents when they go to component and back, so this won't impact any existing data. Newly created books though for example will change behavior in that they will no longer insert black color codes in the serialized data and will only represent intentional color changes by the creator of the book. This will result in cleaner data on them, and books are the only thing I'm aware of that has a behavioral shift due to the likelyhood of the default color system kicking in on other parts of the string. A unit test has been added to verify integrity of serialization to ensure that any legacy string that is converted into Components will always re-encode back in the same way when going back to Legacy. --- nms-patches/PlayerConnection.patch | 2 +- .../craftbukkit/entity/CraftEntity.java | 3 +- .../craftbukkit/entity/CraftPlayer.java | 3 +- .../craftbukkit/inventory/CraftMetaBook.java | 2 +- .../craftbukkit/inventory/CraftMetaItem.java | 15 ++-- .../craftbukkit/util/CraftChatMessage.java | 64 +++++++------ .../util/CraftChatMessageTest.java | 89 +++++++++++++++++++ 7 files changed, 139 insertions(+), 39 deletions(-) create mode 100644 src/test/java/org/bukkit/craftbukkit/util/CraftChatMessageTest.java diff --git a/nms-patches/PlayerConnection.patch b/nms-patches/PlayerConnection.patch index 9602cddff9..3ca314c5f8 100644 --- a/nms-patches/PlayerConnection.patch +++ b/nms-patches/PlayerConnection.patch @@ -133,7 +133,7 @@ + // CraftBukkit start + @Deprecated public void disconnect(IChatBaseComponent ichatbasecomponent) { -+ disconnect(CraftChatMessage.fromComponent(ichatbasecomponent, EnumChatFormat.WHITE)); ++ disconnect(CraftChatMessage.fromComponent(ichatbasecomponent)); + } + // CraftBukkit end + diff --git a/src/main/java/org/bukkit/craftbukkit/entity/CraftEntity.java b/src/main/java/org/bukkit/craftbukkit/entity/CraftEntity.java index 1ec15f492a..30bcfae653 100644 --- a/src/main/java/org/bukkit/craftbukkit/entity/CraftEntity.java +++ b/src/main/java/org/bukkit/craftbukkit/entity/CraftEntity.java @@ -135,7 +135,6 @@ import net.minecraft.server.EntityWolf; import net.minecraft.server.EntityZombie; import net.minecraft.server.EntityZombieHusk; import net.minecraft.server.EntityZombieVillager; -import net.minecraft.server.EnumChatFormat; import net.minecraft.server.IChatBaseComponent; import net.minecraft.server.NBTTagCompound; import org.bukkit.EntityEffect; @@ -782,7 +781,7 @@ public abstract class CraftEntity implements org.bukkit.entity.Entity { @Override public String getName() { - return CraftChatMessage.fromComponent(getHandle().getDisplayName(), EnumChatFormat.WHITE); + return CraftChatMessage.fromComponent(getHandle().getDisplayName()); } @Override diff --git a/src/main/java/org/bukkit/craftbukkit/entity/CraftPlayer.java b/src/main/java/org/bukkit/craftbukkit/entity/CraftPlayer.java index 729444c16c..c17c125a09 100644 --- a/src/main/java/org/bukkit/craftbukkit/entity/CraftPlayer.java +++ b/src/main/java/org/bukkit/craftbukkit/entity/CraftPlayer.java @@ -35,7 +35,6 @@ import net.minecraft.server.Container; import net.minecraft.server.Entity; import net.minecraft.server.EntityLiving; import net.minecraft.server.EntityPlayer; -import net.minecraft.server.EnumChatFormat; import net.minecraft.server.EnumColor; import net.minecraft.server.EnumGamemode; import net.minecraft.server.IChatBaseComponent; @@ -220,7 +219,7 @@ public class CraftPlayer extends CraftHumanEntity implements Player { @Override public String getPlayerListName() { - return getHandle().listName == null ? getName() : CraftChatMessage.fromComponent(getHandle().listName, EnumChatFormat.WHITE); + return getHandle().listName == null ? getName() : CraftChatMessage.fromComponent(getHandle().listName); } @Override diff --git a/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaBook.java b/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaBook.java index cdd7342d0b..6a044c3451 100644 --- a/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaBook.java +++ b/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaBook.java @@ -127,7 +127,7 @@ public class CraftMetaBook extends CraftMetaItem implements BookMeta { if (hasPages()) { NBTTagList list = new NBTTagList(); for (IChatBaseComponent page : pages) { - list.add(NBTTagString.a(page == null ? "" : page.getLegacyString())); + list.add(NBTTagString.a(page == null ? "" : CraftChatMessage.fromComponent(page))); } itemData.set(BOOK_PAGES.NBT, list); } diff --git a/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaItem.java b/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaItem.java index aa34e7af14..c7612457a7 100644 --- a/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaItem.java +++ b/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaItem.java @@ -37,7 +37,6 @@ import java.util.logging.Logger; import javax.annotation.Nonnull; import javax.annotation.Nullable; import net.minecraft.server.ChatComponentText; -import net.minecraft.server.EnumChatFormat; import net.minecraft.server.EnumItemSlot; import net.minecraft.server.GenericAttributes; import net.minecraft.server.IChatBaseComponent; @@ -675,7 +674,7 @@ class CraftMetaItem implements ItemMeta, Damageable, Repairable, BlockDataMeta { NBTTagList tagList = new NBTTagList(); for (IChatBaseComponent value : list) { // SPIGOT-5342 - horrible hack as 0 version does not go through the Mojang updater - tagList.add(NBTTagString.a(version <= 0 || version >= 1803 ? CraftChatMessage.toJSON(value) : CraftChatMessage.fromComponent(value, EnumChatFormat.DARK_PURPLE))); // SPIGOT-4935 + tagList.add(NBTTagString.a(version <= 0 || version >= 1803 ? CraftChatMessage.toJSON(value) : CraftChatMessage.fromComponent(value))); // SPIGOT-4935 } return tagList; @@ -755,7 +754,7 @@ class CraftMetaItem implements ItemMeta, Damageable, Repairable, BlockDataMeta { @Override public String getDisplayName() { - return CraftChatMessage.fromComponent(displayName, EnumChatFormat.WHITE); + return CraftChatMessage.fromComponent(displayName); } @Override @@ -770,7 +769,7 @@ class CraftMetaItem implements ItemMeta, Damageable, Repairable, BlockDataMeta { @Override public String getLocalizedName() { - return CraftChatMessage.fromComponent(locName, EnumChatFormat.WHITE); + return CraftChatMessage.fromComponent(locName); } @Override @@ -883,7 +882,7 @@ class CraftMetaItem implements ItemMeta, Damageable, Repairable, BlockDataMeta { @Override public List getLore() { - return this.lore == null ? null : new ArrayList(Lists.transform(this.lore, (line) -> CraftChatMessage.fromComponent(line, EnumChatFormat.DARK_PURPLE))); + return this.lore == null ? null : new ArrayList(Lists.transform(this.lore, CraftChatMessage::fromComponent)); } @Override @@ -1219,14 +1218,14 @@ class CraftMetaItem implements ItemMeta, Damageable, Repairable, BlockDataMeta { @Overridden ImmutableMap.Builder serialize(ImmutableMap.Builder builder) { if (hasDisplayName()) { - builder.put(NAME.BUKKIT, CraftChatMessage.fromComponent(displayName, EnumChatFormat.WHITE)); + builder.put(NAME.BUKKIT, CraftChatMessage.fromComponent(displayName)); } if (hasLocalizedName()) { - builder.put(LOCNAME.BUKKIT, CraftChatMessage.fromComponent(locName, EnumChatFormat.WHITE)); + builder.put(LOCNAME.BUKKIT, CraftChatMessage.fromComponent(locName)); } if (hasLore()) { - builder.put(LORE.BUKKIT, ImmutableList.copyOf(Lists.transform(lore, (line) -> CraftChatMessage.fromComponent(line, EnumChatFormat.DARK_PURPLE)))); + builder.put(LORE.BUKKIT, ImmutableList.copyOf(Lists.transform(lore, CraftChatMessage::fromComponent))); } if (hasCustomModelData()) { diff --git a/src/main/java/org/bukkit/craftbukkit/util/CraftChatMessage.java b/src/main/java/org/bukkit/craftbukkit/util/CraftChatMessage.java index 2e162b9ea3..a44602ce01 100644 --- a/src/main/java/org/bukkit/craftbukkit/util/CraftChatMessage.java +++ b/src/main/java/org/bukkit/craftbukkit/util/CraftChatMessage.java @@ -38,7 +38,9 @@ public final class CraftChatMessage { } private static final class StringMessage { - private static final Pattern INCREMENTAL_PATTERN = Pattern.compile("(" + String.valueOf(org.bukkit.ChatColor.COLOR_CHAR) + "[0-9a-fk-or])|(\\n)|((?:(?:https?):\\/\\/)?(?:[-\\w_\\.]{2,}\\.[a-z]{2,4}.*?(?=[\\.\\?!,;:]?(?:[" + String.valueOf(org.bukkit.ChatColor.COLOR_CHAR) + " \\n]|$))))", Pattern.CASE_INSENSITIVE); + private static final Pattern INCREMENTAL_PATTERN = Pattern.compile("(" + String.valueOf(org.bukkit.ChatColor.COLOR_CHAR) + "[0-9a-fk-or])|((?:(?:https?):\\/\\/)?(?:[-\\w_\\.]{2,}\\.[a-z]{2,4}.*?(?=[\\.\\?!,;:]?(?:[" + String.valueOf(org.bukkit.ChatColor.COLOR_CHAR) + " \\n]|$))))|(\\n)", Pattern.CASE_INSENSITIVE); + // Separate pattern with no group 3, new lines are part of previous string + private static final Pattern INCREMENTAL_PATTERN_KEEP_NEWLINES = Pattern.compile("(" + String.valueOf(org.bukkit.ChatColor.COLOR_CHAR) + "[0-9a-fk-or])|((?:(?:https?):\\/\\/)?(?:[-\\w_\\.]{2,}\\.[a-z]{2,4}.*?(?=[\\.\\?!,;:]?(?:[" + String.valueOf(org.bukkit.ChatColor.COLOR_CHAR) + " ]|$))))", Pattern.CASE_INSENSITIVE); private final List list = new ArrayList(); private IChatBaseComponent currentChatComponent = new ChatComponentText(""); @@ -55,20 +57,23 @@ public final class CraftChatMessage { } list.add(currentChatComponent); - Matcher matcher = INCREMENTAL_PATTERN.matcher(message); + Matcher matcher = (keepNewlines ? INCREMENTAL_PATTERN_KEEP_NEWLINES : INCREMENTAL_PATTERN).matcher(message); String match = null; + boolean needsAdd = false; while (matcher.find()) { int groupId = 0; while ((match = matcher.group(++groupId)) == null) { // NOOP } - appendNewComponent(matcher.start(groupId)); + int index = matcher.start(groupId); + if (index > currentIndex) { + needsAdd = false; + appendNewComponent(index); + } switch (groupId) { case 1: EnumChatFormat format = formatMap.get(match.toLowerCase(java.util.Locale.ENGLISH).charAt(1)); - if (format == EnumChatFormat.RESET) { - modifier = new ChatModifier(); - } else if (format.isFormat()) { + if (format.isFormat() && format != EnumChatFormat.RESET) { switch (format) { case BOLD: modifier.setBold(Boolean.TRUE); @@ -91,26 +96,27 @@ public final class CraftChatMessage { } else { // Color resets formatting modifier = new ChatModifier().setColor(format); } + needsAdd = true; break; case 2: - if (keepNewlines) { - currentChatComponent.addSibling(new ChatComponentText("\n")); - } else { - currentChatComponent = null; - } - break; - case 3: if (!(match.startsWith("http://") || match.startsWith("https://"))) { match = "http://" + match; } modifier.setChatClickable(new ChatClickable(EnumClickAction.OPEN_URL, match)); appendNewComponent(matcher.end(groupId)); modifier.setChatClickable((ChatClickable) null); + break; + case 3: + if (needsAdd) { + appendNewComponent(index); + } + currentChatComponent = null; + break; } currentIndex = matcher.end(groupId); } - if (currentIndex < message.length()) { + if (currentIndex < message.length() || needsAdd) { appendNewComponent(message.length()); } @@ -118,12 +124,12 @@ public final class CraftChatMessage { } private void appendNewComponent(int index) { - if (index <= currentIndex) { - return; - } IChatBaseComponent addition = new ChatComponentText(message.substring(currentIndex, index)).setChatModifier(modifier); currentIndex = index; modifier = modifier.clone(); + if (modifier.getColor() == EnumChatFormat.RESET) { + modifier.setColor(null); + } if (currentChatComponent == null) { currentChatComponent = new ChatComponentText(""); list.add(currentChatComponent); @@ -160,21 +166,29 @@ public final class CraftChatMessage { return new StringMessage(message, keepNewlines).getOutput(); } - public static String fromComponent(IChatBaseComponent component) { - return fromComponent(component, EnumChatFormat.BLACK); - } - public static String toJSON(IChatBaseComponent component) { return IChatBaseComponent.ChatSerializer.a(component); } - public static String fromComponent(IChatBaseComponent component, EnumChatFormat defaultColor) { + public static String fromComponent(IChatBaseComponent component) { if (component == null) return ""; StringBuilder out = new StringBuilder(); - for (IChatBaseComponent c : (Iterable) component) { + boolean hadFormat = false; + for (IChatBaseComponent c : component) { ChatModifier modi = c.getChatModifier(); - out.append(modi.getColor() == null ? defaultColor : modi.getColor()); + EnumChatFormat color = modi.getColor(); + if (!c.getText().isEmpty() || color != null) { + if (color != null) { + out.append(color); + if (color != EnumChatFormat.RESET) { + hadFormat = true; + } + } else if (hadFormat) { + out.append(ChatColor.RESET); + hadFormat = false; + } + } if (modi.isBold()) { out.append(EnumChatFormat.BOLD); } @@ -192,7 +206,7 @@ public final class CraftChatMessage { } out.append(c.getText()); } - return out.toString().replaceFirst("^(" + defaultColor + ")*", ""); + return out.toString(); } public static IChatBaseComponent fixComponent(IChatBaseComponent component) { diff --git a/src/test/java/org/bukkit/craftbukkit/util/CraftChatMessageTest.java b/src/test/java/org/bukkit/craftbukkit/util/CraftChatMessageTest.java new file mode 100644 index 0000000000..5972b79e3b --- /dev/null +++ b/src/test/java/org/bukkit/craftbukkit/util/CraftChatMessageTest.java @@ -0,0 +1,89 @@ +package org.bukkit.craftbukkit.util; + +import net.minecraft.server.IChatBaseComponent; +import org.bukkit.support.AbstractTestingBase; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; + +public class CraftChatMessageTest extends AbstractTestingBase { + + @Test + public void testSimpleStrings() { + // These should be able to go from legacy to comp to legacy back without data changing + testString("§fFoo"); + testString("§fFoo§f§l"); // Keeps empty format at end + testString("Foo"); + testString("§r§oFoo"); // Retains reset at start (item names can use this to get rid of italics) + testString("Foo§bBar"); + testString("F§loo§b§oBa§b§lr"); // any non color formatting code implies previous color code. + // So §l at start has no inherited color code, so that's fine, but the one at the end, + // while Ba§l would work visually, serializing back will include the implied color + + testString("F§loo§rBa§lr"); // But if reset was used before.... then it can be standalone + testString("§fFoo§bBar"); + testString("§fFoo§bBar§rBaz"); + } + + @Test + public void testNewLineBehavior() { + // new line retain should stay as 1 comp + testString("Hello§0\n§rFoo\n§5Test", true); + testString("§0Foo!\n", true); + testString("§0Foo!§0\\n§0\\n§0Bar\n", true); + + // dont retain line returns multiple components + IChatBaseComponent[] components = CraftChatMessage.fromString("Hello§0\n§rFoo\n§5Test"); + assertEquals("Has 3 components", 3, components.length); + assertEquals("Hello§0", CraftChatMessage.fromComponent(components[0])); + assertEquals("§rFoo", CraftChatMessage.fromComponent(components[1])); + assertEquals("§5Test", CraftChatMessage.fromComponent(components[2])); + } + + @Test + public void testComponents() { + testComponent("Foo§bBar§rBaz", create("Foo", "§bBar", "Baz")); + testComponent("§fFoo§bBar§rBaz", create("", "§fFoo", "§bBar", "Baz")); + testComponent("§fFoo§bBar§rBaz", create("", "§fFoo", "§bBar", "", "Baz")); + testComponent("§fFoo§bBar§rBaz", create("§fFoo", "§bBar", "Baz")); + testComponent("Foo§bBar§rBaz", create("", "Foo", "§bBar", "Baz")); + testComponent("§fFoo§bBar§rBaz", create("§fFoo", "§bBar", "Baz")); + testComponent("F§foo§bBar§rBaz", create("F§foo", "§bBar", "Baz")); + } + + private IChatBaseComponent create(String txt, String ...rest) { + IChatBaseComponent cmp = CraftChatMessage.fromString(txt, false)[0]; + for (String s : rest) { + cmp.addSibling(CraftChatMessage.fromString(s, true)[0]); + } + + return cmp; + } + + private void testString(String expected) { + testString(expected, false); + } + + private void testString(String expected, boolean keepNewLines) { + testString(expected, expected, keepNewLines); + } + + private void testString(String input, String expected) { + testString(input, expected, false); + } + + private void testString(String input, String expected, boolean keepNewLines) { + IChatBaseComponent cmp = CraftChatMessage.fromString(input, keepNewLines)[0]; + String actual = CraftChatMessage.fromComponent(cmp); + assertEquals("\nComponent: " + cmp + "\n", expected, actual); + } + + private void testComponent(String expected, IChatBaseComponent cmp) { + String actual = CraftChatMessage.fromComponent(cmp); + assertEquals("\nComponent: " + cmp + "\n", expected, actual); + + IChatBaseComponent expectedCmp = CraftChatMessage.fromString(expected, true)[0]; + String actualExpectedCmp = CraftChatMessage.fromComponent(expectedCmp); + assertEquals("\nComponent: " + expectedCmp + "\n", expected, actualExpectedCmp); + } +}