Fix some (previously) existing issues with name/lore saving, cleanup

common/src/main/java/nl/matsv/viabackwards/api/data/MappedItem.java

@@ -9,7 +9,7 @@ public class MappedItem {
 
     public MappedItem(int id, String name) {
         this.id = id;
-        this.jsonName = ChatRewriter.legacyTextToJsonString(name);
+        this.jsonName = ChatRewriter.legacyTextToJsonString("§f" + name);
     }
 
     public int getId() {

common/src/main/java/nl/matsv/viabackwards/api/data/MappedLegacyBlockItem.java

@@ -15,7 +15,7 @@ public class MappedLegacyBlockItem {
     public MappedLegacyBlockItem(int id, short data, @Nullable String name, boolean block) {
         this.id = id;
         this.data = data;
-        this.name = name != null ? "§r" + name : null;
+        this.name = name != null ? "§f" + name : null;
         this.block = block ? new Block(id, data) : null;
     }
 

common/src/main/java/nl/matsv/viabackwards/api/rewriters/EnchantmentRewriter.java

@@ -2,7 +2,6 @@ package nl.matsv.viabackwards.api.rewriters;
 
 import us.myles.ViaVersion.api.minecraft.item.Item;
 import us.myles.ViaVersion.protocols.protocol1_13to1_12_2.ChatRewriter;
-import us.myles.viaversion.libs.opennbt.tag.builtin.ByteTag;
 import us.myles.viaversion.libs.opennbt.tag.builtin.CompoundTag;
 import us.myles.viaversion.libs.opennbt.tag.builtin.ListTag;
 import us.myles.viaversion.libs.opennbt.tag.builtin.ShortTag;
@@ -11,22 +10,26 @@ import us.myles.viaversion.libs.opennbt.tag.builtin.Tag;
 
 import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 
+/**
+ * Rewriter to handle the addition of new enchantments.
+ */
 public class EnchantmentRewriter {
 
     private final Map<String, String> enchantmentMappings = new HashMap<>();
-    private final String nbtTagName;
+    private final ItemRewriter itemRewriter;
     private final boolean jsonFormat;
 
-    public EnchantmentRewriter(String nbtTagName, boolean jsonFormat) {
+    public EnchantmentRewriter(ItemRewriter itemRewriter, boolean jsonFormat) {
-        this.nbtTagName = nbtTagName;
+        this.itemRewriter = itemRewriter;
         this.jsonFormat = jsonFormat;
     }
 
-    public EnchantmentRewriter(String nbtTagName) {
+    public EnchantmentRewriter(ItemRewriter itemRewriter) {
-        this(nbtTagName, true);
+        this(itemRewriter, true);
     }
 
     public void registerEnchantment(String key, String replacementLore) {
@@ -49,10 +52,10 @@ public class EnchantmentRewriter {
         CompoundTag tag = item.getTag();
         if (tag == null) return;
 
-        if (tag.contains(nbtTagName + "|Enchantments")) {
+        if (tag.contains(itemRewriter.getNbtTagName() + "|Enchantments")) {
             rewriteEnchantmentsToServer(tag, false);
         }
-        if (tag.contains(nbtTagName + "|StoredEnchantments")) {
+        if (tag.contains(itemRewriter.getNbtTagName() + "|StoredEnchantments")) {
             rewriteEnchantmentsToServer(tag, true);
         }
     }
@@ -60,86 +63,67 @@ public class EnchantmentRewriter {
     public void rewriteEnchantmentsToClient(CompoundTag tag, boolean storedEnchant) {
         String key = storedEnchant ? "StoredEnchantments" : "Enchantments";
         ListTag enchantments = tag.get(key);
-        ListTag remappedEnchantments = new ListTag(nbtTagName + "|" + key, CompoundTag.class);
+        List<Tag> loreToAdd = new ArrayList<>();
-        List<Tag> lore = new ArrayList<>();
+        boolean changed = false;
-        for (Tag enchantmentEntry : enchantments.clone()) {
+
-            Tag idTag = ((CompoundTag) enchantmentEntry).get("id");
+        Iterator<Tag> iterator = enchantments.iterator();
+        while (iterator.hasNext()) {
+            CompoundTag enchantmentEntry = (CompoundTag) iterator.next();
+            StringTag idTag = enchantmentEntry.get("id");
             if (idTag == null) continue;
 
-            String newId = (String) idTag.getValue();
+            String enchantmentId = idTag.getValue();
-            String enchantmentName = enchantmentMappings.get(newId);
+            String remappedName = enchantmentMappings.get(enchantmentId);
-            if (enchantmentName != null) {
+            if (remappedName != null) {
-                enchantments.remove(enchantmentEntry);
+                if (!changed) {
-                Number level = (Number) ((CompoundTag) enchantmentEntry).get("lvl").getValue();
+                    // Backup original before doing modifications
-                String loreValue = enchantmentName + " " + getRomanNumber(level.intValue());
+                    itemRewriter.saveListTag(tag, enchantments);
-                if (jsonFormat) {
+                    changed = true;
-                    loreValue = ChatRewriter.legacyTextToJson(loreValue).toString();
                 }
 
-                lore.add(new StringTag("", loreValue));
+                iterator.remove();
-                remappedEnchantments.add(enchantmentEntry);
+
+                Number level = (Number) enchantmentEntry.get("lvl").getValue();
+                String loreValue = remappedName + " " + getRomanNumber(level.intValue());
+                if (jsonFormat) {
+                    loreValue = ChatRewriter.legacyTextToJsonString(loreValue);
+                }
+
+                loreToAdd.add(new StringTag("", loreValue));
             }
         }
-        if (!lore.isEmpty()) {
+
+        if (!loreToAdd.isEmpty()) {
+            // Add dummy enchant for the glow effect if there are no actual enchantments left
             if (!storedEnchant && enchantments.size() == 0) {
                 CompoundTag dummyEnchantment = new CompoundTag("");
                 dummyEnchantment.put(new StringTag("id", ""));
                 dummyEnchantment.put(new ShortTag("lvl", (short) 0));
                 enchantments.add(dummyEnchantment);
-
-                tag.put(new ByteTag(nbtTagName + "|dummyEnchant"));
             }
 
-            tag.put(remappedEnchantments);
-
             CompoundTag display = tag.get("display");
             if (display == null) {
                 tag.put(display = new CompoundTag("display"));
             }
+
             ListTag loreTag = display.get("Lore");
             if (loreTag == null) {
                 display.put(loreTag = new ListTag("Lore", StringTag.class));
+            } else {
+                // Save original lore
+                itemRewriter.saveListTag(display, loreTag);
             }
 
-            lore.addAll(loreTag.getValue());
+            loreToAdd.addAll(loreTag.getValue());
-            loreTag.setValue(lore);
+            loreTag.setValue(loreToAdd);
         }
     }
 
     public void rewriteEnchantmentsToServer(CompoundTag tag, boolean storedEnchant) {
+        // Just restore the original tag ig present (lore is always restored in the item rewriter)
         String key = storedEnchant ? "StoredEnchantments" : "Enchantments";
-        ListTag remappedEnchantments = tag.get(nbtTagName + "|" + key);
+        itemRewriter.restoreListTag(tag, key);
-        ListTag enchantments = tag.get(key);
-        if (enchantments == null) {
-            enchantments = new ListTag(key, CompoundTag.class);
-        }
-
-        if (!storedEnchant && tag.remove(nbtTagName + "|dummyEnchant") != null) {
-            for (Tag enchantment : enchantments.clone()) {
-                String id = (String) ((CompoundTag) enchantment).get("id").getValue();
-                if (id.isEmpty()) {
-                    enchantments.remove(enchantment);
-                }
-            }
-        }
-
-        CompoundTag display = tag.get("display");
-        // A few null checks just to be safe, though they shouldn't actually be
-        ListTag lore = display != null ? display.get("Lore") : null;
-        for (Tag enchantment : remappedEnchantments.clone()) {
-            enchantments.add(enchantment);
-            if (lore != null && lore.size() != 0) {
-                lore.remove(lore.get(0));
-            }
-        }
-        if (lore != null && lore.size() == 0) {
-            display.remove("Lore");
-            if (display.isEmpty()) {
-                tag.remove("display");
-            }
-        }
-        tag.put(enchantments);
-        tag.remove(remappedEnchantments.getName());
     }
 
     public static String getRomanNumber(int number) {

common/src/main/java/nl/matsv/viabackwards/api/rewriters/ItemRewriter.java

@@ -32,7 +32,7 @@ public abstract class ItemRewriter<T extends BackwardsProtocol> extends ItemRewr
             if (name != null) {
                 String newValue = translatableRewriter.processText(name.getValue()).toString();
                 if (!newValue.equals(name.getValue())) {
-                    saveNameTag(display, name);
+                    saveStringTag(display, name);
                 }
 
                 name.setValue(newValue);
@@ -40,7 +40,6 @@ public abstract class ItemRewriter<T extends BackwardsProtocol> extends ItemRewr
 
             ListTag lore = display.get("Lore");
             if (lore != null) {
-                ListTag original = null;
                 boolean changed = false;
                 for (Tag loreEntryTag : lore) {
                     if (!(loreEntryTag instanceof StringTag)) continue;
@@ -48,16 +47,13 @@ public abstract class ItemRewriter<T extends BackwardsProtocol> extends ItemRewr
                     StringTag loreEntry = (StringTag) loreEntryTag;
                     String newValue = translatableRewriter.processText(loreEntry.getValue()).toString();
                     if (!changed && !newValue.equals(loreEntry.getValue())) {
+                        // Backup original lore before doing any modifications
                         changed = true;
-                        original = lore.clone();
+                        saveListTag(display, lore);
                     }
 
                     loreEntry.setValue(newValue);
                 }
-
-                if (changed) {
-                    saveLoreTag(display, original);
-                }
             }
         }
 

common/src/main/java/nl/matsv/viabackwards/api/rewriters/ItemRewriterBase.java

@@ -3,7 +3,6 @@ package nl.matsv.viabackwards.api.rewriters;
 import nl.matsv.viabackwards.api.BackwardsProtocol;
 import org.jetbrains.annotations.Nullable;
 import us.myles.ViaVersion.api.minecraft.item.Item;
-import us.myles.viaversion.libs.opennbt.conversion.builtin.CompoundTagConverter;
 import us.myles.viaversion.libs.opennbt.tag.builtin.CompoundTag;
 import us.myles.viaversion.libs.opennbt.tag.builtin.ListTag;
 import us.myles.viaversion.libs.opennbt.tag.builtin.StringTag;
@@ -11,7 +10,6 @@ import us.myles.viaversion.libs.opennbt.tag.builtin.Tag;
 
 public abstract class ItemRewriterBase<T extends BackwardsProtocol> extends Rewriter<T> {
 
-    protected static final CompoundTagConverter CONVERTER = new CompoundTagConverter();
     protected final String nbtTagName;
     protected final boolean jsonNameFormat;
 
@@ -40,12 +38,30 @@ public abstract class ItemRewriterBase<T extends BackwardsProtocol> extends Rewr
         return item;
     }
 
-    protected void saveNameTag(CompoundTag displayTag, StringTag original) {
+    protected boolean hasBackupTag(CompoundTag displayTag, String tagName) {
-        displayTag.put(new StringTag(nbtTagName + "|o" + original.getName(), original.getValue()));
+        return displayTag.contains(nbtTagName + "|o" + tagName);
     }
 
-    protected void saveLoreTag(CompoundTag displayTag, ListTag original) {
+    protected void saveStringTag(CompoundTag displayTag, StringTag original) {
-        displayTag.put(new ListTag(nbtTagName + "|o" + original.getName(), original.getValue()));
+        // Multiple places might try to backup data
+        String name = nbtTagName + "|o" + original.getName();
+        if (!displayTag.contains(name)) {
+            displayTag.put(new StringTag(name, original.getValue()));
+        }
+    }
+
+    protected void saveListTag(CompoundTag displayTag, ListTag original) {
+        // Multiple places might try to backup data
+        String name = nbtTagName + "|o" + original.getName();
+        if (!displayTag.contains(name)) {
+            // Clone all tag entries
+            ListTag listTag = new ListTag(name);
+            for (Tag tag : original.getValue()) {
+                listTag.add(tag.clone());
+            }
+
+            displayTag.put(listTag);
+        }
     }
 
     protected void restoreDisplayTag(Item item) {
@@ -57,18 +73,29 @@ public abstract class ItemRewriterBase<T extends BackwardsProtocol> extends Rewr
             if (display.remove(nbtTagName + "|customName") != null) {
                 display.remove("Name");
             } else {
-                restoreDisplayTag(display, "Name");
+                restoreStringTag(display, "Name");
             }
 
             // Restore lore
-            restoreDisplayTag(display, "Lore");
+            restoreListTag(display, "Lore");
         }
     }
 
-    protected void restoreDisplayTag(CompoundTag displayTag, String tagName) {
+    protected void restoreStringTag(CompoundTag tag, String tagName) {
-        Tag original = displayTag.remove(nbtTagName + "|o" + tagName);
+        StringTag original = tag.remove(nbtTagName + "|o" + tagName);
         if (original != null) {
-            displayTag.put(original);
+            tag.put(new StringTag(tagName, original.getValue()));
         }
     }
+
+    protected void restoreListTag(CompoundTag tag, String tagName) {
+        ListTag original = tag.remove(nbtTagName + "|o" + tagName);
+        if (original != null) {
+            tag.put(new ListTag(tagName, original.getValue()));
+        }
+    }
+
+    public String getNbtTagName() {
+        return nbtTagName;
+    }
 }

common/src/main/java/nl/matsv/viabackwards/api/rewriters/LegacyBlockItemRewriter.java

@@ -240,7 +240,7 @@ public abstract class LegacyBlockItemRewriter<T extends BackwardsProtocol> exten
         CompoundTag tag = new CompoundTag("");
         tag.put(new CompoundTag("display"));
         text = "§r" + text;
-        ((CompoundTag) tag.get("display")).put(new StringTag("Name", jsonNameFormat ? ChatRewriter.legacyTextToJson(text).toString() : text));
+        ((CompoundTag) tag.get("display")).put(new StringTag("Name", jsonNameFormat ? ChatRewriter.legacyTextToJsonString(text) : text));
         return tag;
     }
 

common/src/main/java/nl/matsv/viabackwards/protocol/protocol1_12_2to1_13/packets/BlockItemPackets1_13.java

@@ -574,7 +574,7 @@ public class BlockItemPackets1_13 extends nl.matsv.viabackwards.api.rewriters.It
             CompoundTag display = tag.get("display");
             if (display != null) {
                 StringTag name = display.get("Name");
-                if (name instanceof StringTag) {
+                if (name != null) {
                     display.put(new StringTag(extraNbtTag + "|Name", name.getValue()));
                     name.setValue(ChatRewriter.jsonToLegacyText(name.getValue()));
                 }
@@ -630,6 +630,7 @@ public class BlockItemPackets1_13 extends nl.matsv.viabackwards.api.rewriters.It
         tag.put(newCanPlaceOn);
     }
 
+    //TODO un-ugly all of this
     private void rewriteEnchantmentsToClient(CompoundTag tag, boolean storedEnch) {
         String key = storedEnch ? "StoredEnchantments" : "Enchantments";
         ListTag enchantments = tag.get(key);
@@ -775,7 +776,7 @@ public class BlockItemPackets1_13 extends nl.matsv.viabackwards.api.rewriters.It
             if (display instanceof CompoundTag) {
                 CompoundTag displayTag = (CompoundTag) display;
                 StringTag name = displayTag.get("Name");
-                if (name instanceof StringTag) {
+                if (name != null) {
                     StringTag via = displayTag.remove(extraNbtTag + "|Name");
                     name.setValue(via != null ? via.getValue() : ChatRewriter.legacyTextToJsonString(name.getValue()));
                 }
@@ -863,7 +864,8 @@ public class BlockItemPackets1_13 extends nl.matsv.viabackwards.api.rewriters.It
             for (Tag oldTag : blockTag) {
                 Object value = oldTag.getValue();
                 String oldId = value.toString().replace("minecraft:", "");
-                String numberConverted = BlockIdData.numberIdToString.get(Ints.tryParse(oldId));
+                int key = Ints.tryParse(oldId);
+                String numberConverted = BlockIdData.numberIdToString.get(key);
                 if (numberConverted != null) {
                     oldId = numberConverted;
                 }

common/src/main/java/nl/matsv/viabackwards/protocol/protocol1_13_2to1_14/packets/BlockItemPackets1_14.java

@@ -35,7 +35,6 @@ import us.myles.ViaVersion.protocols.protocol1_14to1_13_2.types.Chunk1_14Type;
 import us.myles.ViaVersion.protocols.protocol1_9_3to1_9_1_2.storage.ClientWorld;
 import us.myles.viaversion.libs.gson.JsonElement;
 import us.myles.viaversion.libs.gson.JsonObject;
-import us.myles.viaversion.libs.opennbt.conversion.ConverterRegistry;
 import us.myles.viaversion.libs.opennbt.tag.builtin.CompoundTag;
 import us.myles.viaversion.libs.opennbt.tag.builtin.ListTag;
 import us.myles.viaversion.libs.opennbt.tag.builtin.StringTag;
@@ -518,7 +517,7 @@ public class BlockItemPackets1_14 extends nl.matsv.viabackwards.api.rewriters.It
 
     @Override
     protected void registerRewrites() {
-        enchantmentRewriter = new EnchantmentRewriter(nbtTagName, false);
+        enchantmentRewriter = new EnchantmentRewriter(this, false);
         enchantmentRewriter.registerEnchantment("minecraft:multishot", "§7Multishot");
         enchantmentRewriter.registerEnchantment("minecraft:quick_charge", "§7Quick Charge");
         enchantmentRewriter.registerEnchantment("minecraft:piercing", "§7Piercing");
@@ -529,57 +528,54 @@ public class BlockItemPackets1_14 extends nl.matsv.viabackwards.api.rewriters.It
         if (item == null) return null;
         super.handleItemToClient(item);
 
-        CompoundTag tag = item.getTag();
-        if (tag != null) {
         // Lore now uses JSON
-            if (tag.get("display") instanceof CompoundTag) {
+        CompoundTag tag = item.getTag();
-                CompoundTag display = tag.get("display");
+        CompoundTag display;
-                if (((CompoundTag) tag.get("display")).get("Lore") instanceof ListTag) {
+        if (tag != null && (display = tag.get("display")) != null) {
             ListTag lore = display.get("Lore");
-                    ListTag via = display.remove(nbtTagName + "|Lore");
+            if (lore != null) {
-                    if (via != null) {
+                saveListTag(display, lore);
-                        display.put(ConverterRegistry.convertToTag("Lore", ConverterRegistry.convertToValue(via)));
+
-                    } else {
                 for (Tag loreEntry : lore) {
                     if (!(loreEntry instanceof StringTag)) continue;
 
-                            String value = ((StringTag) loreEntry).getValue();
+                    StringTag loreEntryTag = (StringTag) loreEntry;
+                    String value = loreEntryTag.getValue();
                     if (value != null && !value.isEmpty()) {
-                                ((StringTag) loreEntry).setValue(ChatRewriter.jsonToLegacyText(value));
+                        loreEntryTag.setValue(ChatRewriter.jsonToLegacyText(value));
-                            }
                     }
                 }
             }
         }
 
         enchantmentRewriter.handleToClient(item);
-        }
         return item;
     }
 
     @Override
     public Item handleItemToServer(Item item) {
         if (item == null) return null;
-        super.handleItemToServer(item);
 
+        // Lore now uses JSON
         CompoundTag tag = item.getTag();
-        if (tag != null) {
+        CompoundTag display;
-            // Display Lore now uses JSON
+        if (tag != null && (display = tag.get("display")) != null) {
-            if (tag.get("display") instanceof CompoundTag) {
+            // Transform to json if no backup tag is found (else process that in the super method)
-                CompoundTag display = tag.get("display");
-                if (display.get("Lore") instanceof ListTag) {
             ListTag lore = display.get("Lore");
-                    display.put(ConverterRegistry.convertToTag(nbtTagName + "|Lore", ConverterRegistry.convertToValue(lore)));
+            if (lore != null && !hasBackupTag(display, "Lore")) {
                 for (Tag loreEntry : lore) {
                     if (loreEntry instanceof StringTag) {
-                            ((StringTag) loreEntry).setValue(ChatRewriter.legacyTextToJson(((StringTag) loreEntry).getValue()).toString());
+                        StringTag loreEntryTag = (StringTag) loreEntry;
+                        loreEntryTag.setValue(ChatRewriter.legacyTextToJsonString(loreEntryTag.getValue()));
                     }
                 }
             }
         }
 
         enchantmentRewriter.handleToServer(item);
-        }
+
+        // Call this last to check for the backup lore above
+        super.handleItemToServer(item);
         return item;
     }
 }

common/src/main/java/nl/matsv/viabackwards/protocol/protocol1_15_2to1_16/packets/BlockItemPackets1_16.java

@@ -293,7 +293,7 @@ public class BlockItemPackets1_16 extends nl.matsv.viabackwards.api.rewriters.It
 
     @Override
     protected void registerRewrites() {
-        enchantmentRewriter = new EnchantmentRewriter(nbtTagName);
+        enchantmentRewriter = new EnchantmentRewriter(this);
         enchantmentRewriter.registerEnchantment("minecraft:soul_speed", "§7Soul Speed");
     }