Update, refactor, bug fixes (hard to summarize)

[Fourmisain]

Changes

• fix NullPointerException in method17869Tail due to improperly initialized (read: uninitialized) ThreadLocal

This partially fixes https://github.com/Bernasss12/BetterEnchantedBooks/issues/20.

Before, Enchanted Books would just show "Enchanted Book" without any tooltips in REI. Now it'll show the tooltips like in vanilla.

The reason why it won't show the BEBooks tooltips is because REI uses the "deepest" renderOrderedTooltip method (taking a list of OrderedText), whereas BEBooks uses a "higher" renderTooltip taking an ItemStack, so it can read the enchantments off of it.

To fix this, the whole tooltip and render logic needs to be rewritten.

• update Minecraft version, dependencies, gradle version, build and run gradlew wrapper

Should still be compatible with 1.16.4 (as if anyone still uses that). I wish Github would allow to PR to new branches, but oh well.

• refactor NBTUtils: use streams instead of new ArrayLists, sort() should never throw an Exception

This should be a lot more efficient and easier to read.

• use WeakHashMap for ItemStack caches

New ItemStacks are constantly created and thus the HashMap cache would fill up more and more, which is effectively a (very slow) memory leak. In contrast, WeakHashMap entries are prone to be garbage collected once the objects have no (hard) references to them anymore.

• rename Mixin methods

Fixes https://github.com/Bernasss12/BetterEnchantedBooks/issues/29, as I was running in the same issue.

• add a proper Logger

Log4J2 loggers are named (in this case "BEBooks"), they have log levels, they know where they are called from (class name, line number) and from whom (thread name), making them invaluable for debugging.

• update README

Do not run gradlew idea! It is known to screw up a few things like the dev env.

Comments

The ThreadLocals were initialized using .set(...), but that would only set their value on the main thread, not on the threads they are actually used on, thus later causing NullPointerExceptions. The proper way to initialize ThreadLocals is to create them via ThreadLocal.withInitial(...).

I'm still not sure why ThreadLocals were used in the first place, so I'm not gonna question it.

• instanceof does a null check, so no need for x != null && x instanceof X.

• The clear() collections method does nothing when the collection is empty, so no need for if (!c.isEmpty()) c.clear();.

Sorry this became such a big PR, I'm a "see it, fix it" kinda guy. There's lots of smaller stuff, so if anything isn't clear, do ask!

[Bernasss12]

Thanks for this I have a lot to learn from it! To answer as to why I did stuff the way I did it, to put it broadly, it was because I didn't know any better. I am still learning and I only do after seeing things work and I do feel I have learned a lot since making this mod hence why I mentioned in an issue that I would eventually refactor it all. I guess I won't need to for now though, and again, thank you for that!

[Bernasss12]

There is no special reason to use ThreadLocals to be honest so if you see fit feel free to replace that with something more fitting

[Fourmisain]

There is no special reason to use ThreadLocals to be honest so if you see fit feel free to replace that with something more fitting

Oh, it should be fine as-is. My first instinct would have been to either, if possible, add them as (boolean, ItemStack) fields to one of the classes via mixin, so the variables are more local - or make the variables thread safe (e.g. using volatile boolean or AtomicBoolean instead of ThreadLocal<Boolean>).

The latter seems overkill since the variables are only used on one thread, I think - and the first may need an @Accessor.

In the end, it shouldn't really matter. If BEBooks is to be rewritten so it works with REI too, those variables would probably end up being removed.