Rewrite Adventure Book calls to BookMeta

[TheosRee]

Fixes #14030 with re-routing the pages setter calls to the Book to the BookMeta and then returning the new asBook, rightfully returing an adventure Book instance to work with.

In the process of testing the result I discovered and fixed #14135, which also got broken in the adventure 5 update process. I decided to fix both in the same PR since both issues break the expected result/usage of the methods.

[TheosRee]

I would like to know if the version check should be dropped (the method with a different signature can't be compiled against 26.2 anyways) and if the added code should be at the start or end of the method. I tried to figure it from the changes but it did not lead me to a fixed result.

I also hope that the removal of the remnant method is ok and if I should also remove the methods which now just call super.

[masmc05]

Wouldn't this break if

1. it was used in a chain, so bookMeta.method1().method2() would have had a Book#method2() signature and not a BookMeta#method2(), method1's rewrite to return bookmeta instead of book will break method2
2. The result is passed into a non api method that accepts only an adventure book, and it would result in some error as book meta isn't a book anymore

[TheosRee]

1. Yes, any chained call after one of the pages setter methods will break with a java.lang.IncompatibleClassChangeError: Class org.bukkit.craftbukkit.inventory.CraftMetaBookSigned does not implement the requested interface net.kyori.adventure.inventory.Book. When I am correct that can't be fixed easily because that is an independent call to a different interface? Changing that would also rewrite all "legit" calls to adventure books. Having that method at the end of the chain works however since all other methods (author and title) return the BookMeta and not the Book.
2. That is nothing that I thought of but like my thoughts to 1.: With the openBook method on the player that is kinda legit, but you could also pass the item stack when you work with meta anyways - without you would just work with a Book directly?

Here is my test with the branch:

@Override
public boolean execute(final CommandSender sender, final String commandLabel, final String[] args) {
    final ItemStack book = ItemStack.of(Material.WRITTEN_BOOK);
    final BookMeta meta = (BookMeta) book.getItemMeta();
    eval("getter", () -> meta.pages());
    eval("workingChain", () -> meta.author(Component.empty()).title(Component.empty()).pages(List.of()));
    eval("setterL", () -> meta.pages(List.of()));
    eval("setterL2", () -> meta.pages(List.of()).pages());
    eval("setterA", () -> meta.pages(Component.empty()));
    eval("setterA2", () -> meta.pages(Component.empty()).author());
    eval("doubleSetter", () -> meta.pages(Component.empty()).pages(Component.empty()));
    eval("doubleSetter2", () -> meta.pages(List.of()).pages(List.of()));
    eval("title", () -> meta.title(Component.empty()).pages(List.of()));
    eval("title2", () -> meta.title(Component.empty()).pages(List.of()).title());
    eval("author", () -> meta.author(Component.empty()).pages(List.of()));
    eval("author2", () -> meta.author(Component.empty()).pages(List.of()).author());
    sender.sendMessage(Component.text("Executed book meta test"));
    return true;
}

private void eval(final String name, final Runnable runnable) {
    try {
        runnable.run();
        getLogger().info(name + " executed");
    } catch (final Error error) {
        getLogger().severe(name + " failed: " + error.getMessage());
    }
}

bookmetatest
getter executed
workingChain executed
setterL executed
setterL2 failed: Class org.bukkit.craftbukkit.inventory.CraftMetaBookSigned does not implement the requested interface net.kyori.adventure.inventory.Book
setterA executed
setterA2 failed: Class org.bukkit.craftbukkit.inventory.CraftMetaBookSigned does not implement the requested interface net.kyori.adventure.inventory.Book
doubleSetter failed: Class org.bukkit.craftbukkit.inventory.CraftMetaBookSigned does not implement the requested interface net.kyori.adventure.inventory.Book
doubleSetter2 failed: Class org.bukkit.craftbukkit.inventory.CraftMetaBookSigned does not implement the requested interface net.kyori.adventure.inventory.Book
title executed
title2 failed: Class org.bukkit.craftbukkit.inventory.CraftMetaBookSigned does not implement the requested interface net.kyori.adventure.inventory.Book
author executed
author2 failed: Class org.bukkit.craftbukkit.inventory.CraftMetaBookSigned does not implement the requested interface net.kyori.adventure.inventory.Book
Executed book meta test

[masmc05]

Yeah those are harder to fix, that's why in the original issue I mentioned redirecting instead the calls to an internal class with static methods that take the book meta + the arguments and returns an adventure book, this way nothing further should be affected as it's continuing to expect the Book instance

[TheosRee]

Your suggestion works like a charm!
All test calls now work, however any call after the first page just creates a new book and does not modify the Meta anymore, but who would do that anyway? Right?

[masmc05]

In my opinion that is fine, lots of deprecated methods become no-op in new version releases without a warning period (and this one had a bit of warning period), while the plugin at least loads. But better wait for a team member to decide

[TheosRee]

I would also like if I should put another fix for the WritableBookMeta storing the pages as legacy string and converting them leads to that components or if I should put all changes to the Craft classes into a separate PR:

Edit: Looking a bit longer into internals it looks on purpose that components don't work great in there.

Code

final Component deserialized = MiniMessage.miniMessage().deserialize("<rainbow><bold>Really");
for (final Material material : new Material[]{Material.WRITTEN_BOOK, Material.WRITABLE_BOOK}) {
    final ItemStack book = ItemStack.of(material);
    final BookMeta bookMeta = (BookMeta) book.getItemMeta();
    bookMeta.title(deserialized).author(deserialized).pages(deserialized);
    book.setItemMeta(bookMeta);
    player.give(book);
}
</details>

[electronicboy]

if that breakage isn't related to this then a separate PR is preferred, prevents one thing blocking the other needlessly

[kennytv]

The two Craft* changes don't belong here, but the commodore changes look reasonable

[TheosRee]

The two Craft* changes don't belong here, but the commodore changes look reasonable

As already wrote to mbax in the DC I made these changes to retain the behavior to before the adventure 5 change. After that the Writable Book no longer had colorful pages and the WrittenBook no longer had neither title nor author.

For that reason I see that as required part of the fix to maintain backwards compatibility/have consistent behavior.

Edit: Also, with that comment being an approve and not a change request I thought of it more of a "well, it should not be here but I guess I don't care enough".

[TheosRee]

On suggestion of @masmc05 I added a second issue with reasoning for the Craft changes done in this PR and edited the description.