Rewrite book handling, Fixes SPIGOT-182 and part of SPIGOT-164

7 files changed, 190 insertions, 72 deletions

diff --git a/src/main/java/org/bukkit/craftbukkit/event/CraftEventFactory.java b/src/main/java/org/bukkit/craftbukkit/event/CraftEventFactory.java
index cc1d3718..391e5b92 100644
--- a/src/main/java/org/bukkit/craftbukkit/event/CraftEventFactory.java
+++ b/src/main/java/org/bukkit/craftbukkit/event/CraftEventFactory.java
@@ -826,10 +826,10 @@ public class CraftEventFactory {
         // If they've got the same item in their hand, it'll need to be updated.
         if (itemInHand != null && itemInHand.getItem() == Items.WRITABLE_BOOK) {
             if (!editBookEvent.isCancelled()) {
-                CraftItemStack.setItemMeta(itemInHand, editBookEvent.getNewBookMeta());
                 if (editBookEvent.isSigning()) {
                     itemInHand.setItem(Items.WRITTEN_BOOK);
                 }
+                CraftItemStack.setItemMeta(itemInHand, editBookEvent.getNewBookMeta());
             }
 
             // Client will have updated its idea of the book item; we need to overwrite that
diff --git a/src/main/java/org/bukkit/craftbukkit/inventory/CraftItemFactory.java b/src/main/java/org/bukkit/craftbukkit/inventory/CraftItemFactory.java
index 3e483e56..b46d3b5c 100644
--- a/src/main/java/org/bukkit/craftbukkit/inventory/CraftItemFactory.java
+++ b/src/main/java/org/bukkit/craftbukkit/inventory/CraftItemFactory.java
@@ -62,8 +62,9 @@ public final class CraftItemFactory implements ItemFactory {
         case AIR:
             return null;
         case WRITTEN_BOOK:
+            return meta instanceof CraftMetaBookSigned ? meta : new CraftMetaBookSigned(meta);
         case BOOK_AND_QUILL:
-            return meta instanceof CraftMetaBook ? meta : new CraftMetaBook(meta);
+            return meta != null && meta.getClass().equals(CraftMetaBook.class) ? meta : new CraftMetaBook(meta);
         case SKULL_ITEM:
             return meta instanceof CraftMetaSkull ? meta : new CraftMetaSkull(meta);
         case LEATHER_HELMET:
diff --git a/src/main/java/org/bukkit/craftbukkit/inventory/CraftItemStack.java b/src/main/java/org/bukkit/craftbukkit/inventory/CraftItemStack.java
index 36cf8d0b..53682097 100644
--- a/src/main/java/org/bukkit/craftbukkit/inventory/CraftItemStack.java
+++ b/src/main/java/org/bukkit/craftbukkit/inventory/CraftItemStack.java
@@ -326,6 +326,7 @@ public final class CraftItemStack extends ItemStack {
         }
         switch (getType(item)) {
             case WRITTEN_BOOK:
+                return new CraftMetaBookSigned(item.getTag());
             case BOOK_AND_QUILL:
                 return new CraftMetaBook(item.getTag());
             case SKULL_ITEM:
@@ -394,46 +395,14 @@ public final class CraftItemStack extends ItemStack {
             return false;
         }
 
+        itemMeta = CraftItemFactory.instance().asMetaFor(itemMeta, getType(item));
+        if (itemMeta == null) return true;
+
         NBTTagCompound tag = new NBTTagCompound();
         item.setTag(tag);
 
         ((CraftMetaItem) itemMeta).applyToItem(tag);
 
-        // Hacky fix for books
-        // TODO: Not use a hacky fix for books
-        if (tag.getBoolean(CraftMetaBook.RESOLVED.NBT) && item.getItem() == Items.WRITABLE_BOOK) {
-            if (tag.hasKey(CraftMetaBook.BOOK_PAGES.NBT)) {
-                NBTTagList pages = tag.getList(CraftMetaBook.BOOK_PAGES.NBT, 8);
-
-                for (int i = 0; i < pages.size(); i++) {
-                    String page = pages.getString(i);
-                    page = CraftChatMessage.fromComponent(ChatSerializer.a(page));
-                    pages.a(i, new NBTTagString(page));
-                }
-                tag.set(CraftMetaBook.BOOK_PAGES.NBT, pages);
-            }
-            tag.setBoolean(CraftMetaBook.RESOLVED.NBT, false);
-        } else if (!tag.getBoolean(CraftMetaBook.RESOLVED.NBT) && item.getItem() == Items.WRITTEN_BOOK) {
-            if (tag.hasKey(CraftMetaBook.BOOK_PAGES.NBT)) {
-                NBTTagList pages = tag.getList(CraftMetaBook.BOOK_PAGES.NBT, 8);
-
-                for (int i = 0; i < pages.size(); i++) {
-                    String page = pages.getString(i);
-                    page = ChatSerializer.a(CraftChatMessage.fromString(page, true)[0]);
-                    pages.a(i, new NBTTagString(page));
-                }
-                tag.set(CraftMetaBook.BOOK_PAGES.NBT, pages);
-            }
-
-            tag.setBoolean(CraftMetaBook.RESOLVED.NBT, true);
-            if (!tag.hasKey(CraftMetaBook.BOOK_TITLE.NBT)) {
-                tag.setString(CraftMetaBook.BOOK_TITLE.NBT, "");
-            }
-            if (!tag.hasKey(CraftMetaBook.BOOK_AUTHOR.NBT)) {
-                tag.setString(CraftMetaBook.BOOK_AUTHOR.NBT, "");
-            }
-        }
-
         return true;
     }
 
diff --git a/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaBook.java b/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaBook.java
index 9a24d2e7..debca310 100644
--- a/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaBook.java
+++ b/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaBook.java
@@ -30,27 +30,28 @@ class CraftMetaBook extends CraftMetaItem implements BookMeta {
     static final int MAX_PAGE_LENGTH = 256;
     static final int MAX_TITLE_LENGTH = 0xffff;
 
-    private String title;
-    private String author;
-    private List<String> pages = new ArrayList<String>();
-    private Boolean resolved;
-    private Integer generation;
+    protected String title;
+    protected String author;
+    protected List<String> pages = new ArrayList<String>();
+    protected Integer generation;
 
     CraftMetaBook(CraftMetaItem meta) {
         super(meta);
 
-        if (!(meta instanceof CraftMetaBook)) {
-            return;
+        if (meta instanceof CraftMetaBook) {
+            CraftMetaBook bookMeta = (CraftMetaBook) meta;
+            this.title = bookMeta.title;
+            this.author = bookMeta.author;
+            pages.addAll(bookMeta.pages);
+            this.generation = bookMeta.generation;
         }
-        CraftMetaBook bookMeta = (CraftMetaBook) meta;
-        this.title = bookMeta.title;
-        this.author = bookMeta.author;
-        pages.addAll(bookMeta.pages);
-        this.resolved = bookMeta.resolved;
-        this.generation = bookMeta.generation;
     }
 
     CraftMetaBook(NBTTagCompound tag) {
+        this(tag, true);
+    }
+
+    CraftMetaBook(NBTTagCompound tag, boolean handlePages) {
         super(tag);
 
         if (tag.hasKey(BOOK_TITLE.NBT)) {
@@ -60,7 +61,8 @@ class CraftMetaBook extends CraftMetaItem implements BookMeta {
         if (tag.hasKey(BOOK_AUTHOR.NBT)) {
             this.author = tag.getString(BOOK_AUTHOR.NBT);
         }
-        
+
+        boolean resolved = false;
         if (tag.hasKey(RESOLVED.NBT)) {
             resolved = tag.getBoolean(RESOLVED.NBT);
         }
@@ -69,13 +71,13 @@ class CraftMetaBook extends CraftMetaItem implements BookMeta {
             generation = tag.getInt(GENERATION.NBT);
         }
 
-        if (tag.hasKey(BOOK_PAGES.NBT)) {
+        if (tag.hasKey(BOOK_PAGES.NBT) && handlePages) {
             NBTTagList pages = tag.getList(BOOK_PAGES.NBT, 8);
             String[] pageArray = new String[pages.size()];
 
             for (int i = 0; i < pages.size(); i++) {
                 String page = pages.getString(i);
-                if (resolved != null && resolved) {
+                if (resolved) {
                     page = CraftChatMessage.fromComponent(ChatSerializer.a(page));
                 }
                 pageArray[i] = page;
@@ -95,12 +97,15 @@ class CraftMetaBook extends CraftMetaItem implements BookMeta {
         Iterable<?> pages = SerializableMeta.getObject(Iterable.class, map, BOOK_PAGES.BUKKIT, true);
         CraftMetaItem.safelyAdd(pages, this.pages, MAX_PAGE_LENGTH);
         
-        resolved = SerializableMeta.getObject(Boolean.class, map, RESOLVED.BUKKIT, true);
         generation = SerializableMeta.getObject(Integer.class, map, GENERATION.BUKKIT, true);
     }
 
     @Override
     void applyToItem(NBTTagCompound itemData) {
+        applyToItem(itemData, true);
+    }
+
+    void applyToItem(NBTTagCompound itemData, boolean handlePages) {
         super.applyToItem(itemData);
 
         if (hasTitle()) {
@@ -111,24 +116,18 @@ class CraftMetaBook extends CraftMetaItem implements BookMeta {
             itemData.setString(BOOK_AUTHOR.NBT, this.author);
         }
 
-        if (hasPages()) {
-            NBTTagList list = new NBTTagList();
-            for (String page : pages) {
-                if (resolved != null && resolved) {                    
-                    list.add(new NBTTagString(
-                        ChatSerializer.a(CraftChatMessage.fromString(page, true)[0])
-                    ));
-                } else {
+        if (handlePages) {
+            if (hasPages()) {
+                NBTTagList list = new NBTTagList();
+                for (String page : pages) {
                     list.add(new NBTTagString(page));
                 }
+                itemData.set(BOOK_PAGES.NBT, list);
             }
-            itemData.set(BOOK_PAGES.NBT, list);
-        }
-        
-        if (resolved != null) {
-            itemData.setBoolean(RESOLVED.NBT, resolved);
+
+            itemData.remove(RESOLVED.NBT);
         }
-        
+
         if (generation != null) {
             itemData.setInt(GENERATION.NBT, generation);
         }
@@ -297,10 +296,6 @@ class CraftMetaBook extends CraftMetaItem implements BookMeta {
             builder.put(BOOK_PAGES.BUKKIT, pages);
         }
         
-        if (resolved != null) {
-            builder.put(RESOLVED.BUKKIT, resolved);
-        }
-        
         if (generation != null) {
             builder.put(GENERATION.BUKKIT, generation);
         }
diff --git a/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaBookSigned.java b/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaBookSigned.java
new file mode 100644
index 00000000..fd8ba56d
--- /dev/null
+++ b/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaBookSigned.java
@@ -0,0 +1,142 @@
+package org.bukkit.craftbukkit.inventory;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+
+import net.minecraft.server.NBTTagCompound;
+import net.minecraft.server.NBTTagList;
+
+import org.bukkit.Material;
+import org.bukkit.configuration.serialization.DelegateDeserialization;
+import org.bukkit.craftbukkit.inventory.CraftMetaItem.SerializableMeta;
+import org.bukkit.inventory.meta.BookMeta;
+
+import com.google.common.collect.ImmutableMap.Builder;
+import net.minecraft.server.ChatSerializer;
+import net.minecraft.server.NBTTagString;
+import org.bukkit.craftbukkit.util.CraftChatMessage;
+
+@DelegateDeserialization(SerializableMeta.class)
+class CraftMetaBookSigned extends CraftMetaBook implements BookMeta {
+
+    CraftMetaBookSigned(CraftMetaItem meta) {
+        super(meta);
+    }
+
+    CraftMetaBookSigned(NBTTagCompound tag) {
+        super(tag, false);
+
+        boolean resolved = true;
+        if (tag.hasKey(RESOLVED.NBT)) {
+            resolved = tag.getBoolean(RESOLVED.NBT);
+        }
+
+        if (tag.hasKey(BOOK_PAGES.NBT)) {
+            NBTTagList pages = tag.getList(BOOK_PAGES.NBT, 8);
+            String[] pageArray = new String[pages.size()];
+
+            for (int i = 0; i < pages.size(); i++) {
+                String page = pages.getString(i);
+                if (resolved) {
+                    page = CraftChatMessage.fromComponent(ChatSerializer.a(page));
+                }
+                pageArray[i] = page;
+            }
+
+            addPage(pageArray);
+        }
+    }
+
+    CraftMetaBookSigned(Map<String, Object> map) {
+        super(map);
+
+        setAuthor(SerializableMeta.getString(map, BOOK_AUTHOR.BUKKIT, true));
+
+        setTitle(SerializableMeta.getString(map, BOOK_TITLE.BUKKIT, true));
+
+        Iterable<?> pages = SerializableMeta.getObject(Iterable.class, map, BOOK_PAGES.BUKKIT, true);
+        CraftMetaItem.safelyAdd(pages, this.pages, MAX_PAGE_LENGTH);
+        
+        generation = SerializableMeta.getObject(Integer.class, map, GENERATION.BUKKIT, true);
+    }
+
+    @Override
+    void applyToItem(NBTTagCompound itemData) {
+        super.applyToItem(itemData, false);
+
+        if (hasTitle()) {
+            itemData.setString(BOOK_TITLE.NBT, this.title);
+        } else {
+            itemData.setString(BOOK_TITLE.NBT, " ");
+        }
+
+        if (hasAuthor()) {
+            itemData.setString(BOOK_AUTHOR.NBT, this.author);
+        } else {
+            itemData.setString(BOOK_AUTHOR.NBT, " ");
+        }
+
+        if (hasPages()) {
+            NBTTagList list = new NBTTagList();
+            for (String page : pages) {                   
+                list.add(new NBTTagString(
+                    ChatSerializer.a(CraftChatMessage.fromString(page, true)[0])
+                ));
+            }
+            itemData.set(BOOK_PAGES.NBT, list);
+        }        
+        itemData.setBoolean(RESOLVED.NBT, true);
+
+        if (generation != null) {
+            itemData.setInt(GENERATION.NBT, generation);
+        } else {
+            itemData.setInt(GENERATION.NBT, 0);
+        }
+    }
+
+    @Override
+    boolean isEmpty() {
+        return super.isEmpty();
+    }
+
+    @Override
+    boolean applicableTo(Material type) {
+        switch (type) {
+        case WRITTEN_BOOK:
+        case BOOK_AND_QUILL:
+            return true;
+        default:
+            return false;
+        }
+    }
+
+    @Override
+    public CraftMetaBookSigned clone() {
+        CraftMetaBookSigned meta = (CraftMetaBookSigned) super.clone();
+        return meta;
+    }
+
+    @Override
+    int applyHash() {
+        final int original;
+        int hash = original = super.applyHash();
+        return original != hash ? CraftMetaBookSigned.class.hashCode() ^ hash : hash;
+    }
+
+    @Override
+    boolean equalsCommon(CraftMetaItem meta) {
+        return super.equalsCommon(meta);
+    }
+
+    @Override
+    boolean notUncommon(CraftMetaItem meta) {
+        return super.notUncommon(meta) && (meta instanceof CraftMetaBookSigned || isBookEmpty());
+    }
+
+    @Override
+    Builder<String, Object> serialize(Builder<String, Object> builder) {
+        super.serialize(builder);
+        return builder;
+    }
+}
diff --git a/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaItem.java b/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaItem.java
index 661fbae8..78a0ab49 100644
--- a/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaItem.java
+++ b/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaItem.java
@@ -99,6 +99,7 @@ class CraftMetaItem implements ItemMeta, Repairable {
                     .put(CraftMetaBanner.class, "BANNER")
                     .put(CraftMetaTileEntity.class, "TILE_ENTITY")
                     .put(CraftMetaBook.class, "BOOK")
+                    .put(CraftMetaBookSigned.class, "BOOK_SIGNED")
                     .put(CraftMetaSkull.class, "SKULL")
                     .put(CraftMetaLeatherArmor.class, "LEATHER_ARMOR")
                     .put(CraftMetaMap.class, "MAP")
diff --git a/src/test/java/org/bukkit/craftbukkit/inventory/ItemMetaTest.java b/src/test/java/org/bukkit/craftbukkit/inventory/ItemMetaTest.java
index 10682e76..71a34576 100644
--- a/src/test/java/org/bukkit/craftbukkit/inventory/ItemMetaTest.java
+++ b/src/test/java/org/bukkit/craftbukkit/inventory/ItemMetaTest.java
@@ -138,6 +138,16 @@ public class ItemMetaTest extends AbstractTestingBase {
                     return cleanStack;
                 }
             },
+            new StackProvider(Material.WRITTEN_BOOK) {
+                @Override ItemStack operate(final ItemStack cleanStack) {
+                    final BookMeta meta = (BookMeta) cleanStack.getItemMeta();
+                    meta.setAuthor("Some author");
+                    meta.setPages("Page 1", "Page 2");
+                    meta.setTitle("A title");
+                    cleanStack.setItemMeta(meta);
+                    return cleanStack;
+                }
+            },
             /* Skulls rely on a running server instance
             new StackProvider(Material.SKULL_ITEM) {
                 @Override ItemStack operate(final ItemStack cleanStack) {