Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix MinecraftComponentSerializer on 1.20.3 #144

Merged
merged 2 commits into from
Dec 20, 2023

Conversation

56738
Copy link
Contributor

@56738 56738 commented Dec 6, 2023

ChatSerializer no longer implements JsonDeserializer, it uses codecs instead.
MinecraftComponentSerializer expects the serialization methods to be in a subclass which implements JsonDeserializer.
This change makes it look for these methods in all subclasses instead of just that one.

Additionally, it makes it look for serialization methods which use JsonElement instead of String.
The previous approach of using MC_TEXT_GSON to avoid converting via String doesn't work in 1.20.3, ChatSerializer no longer provides a Gson object with the type adapter.

.filter(m -> Modifier.isStatic(m.getModifiers()))
.filter(m -> CLASS_CHAT_COMPONENT.isAssignableFrom(m.getReturnType()))
.filter(m -> m.getParameterCount() == 1 && m.getParameterTypes()[0].equals(String.class))
.min(Comparator.comparing(Method::getName)) // prefer the #a method
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorting like this seems a little unstable, also it might not work on a Mojang mapped server

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, this is existing code...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#a is for strict parsing and #b is for lenient parsing on most versions. I don't think this makes a difference, the input is always valid JSON anyways (since it comes from a serializer).
Also, I believe the String serialization methods are never actually used. 1.13+ has JsonElement serialization methods, and all versions except 1.20.3 seem to provide a Gson object anyways.

By the way, 1.20.3 provides multiple methods which match the serializeTree/deserializeTree lookup, but the only difference is a null check in one of them so that doesn't matter either.

(Just pushed a change where I replaced min with findFirst in deserializeTree because that was a copy-paste error. The line you commented on is still the same.)

@bergerkiller
Copy link

bergerkiller commented Dec 14, 2023

These fixes work for me on 1.20.3/4 and Im already using it, as without it the library degrades to minimal functionality

Tested with cloud help menu on Minecraft 1.8, 1.13, 1.16.5, 1.19.4, 1.20.2 and 1.20.4.

@zml2008
Copy link
Member

zml2008 commented Dec 16, 2023

Has this been tested to even work back on 1.7.10 when CB shipped relocated gson?

@Machine-Maker
Copy link
Member

Has this been tested to even work back on 1.7.10 when CB shipped relocated gson?

This wouldn't be a new issue caused by these changes tho? It used JsonElement and types from gson before

Copy link
Member

@Machine-Maker Machine-Maker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes all the incorrect facets being selected on 1.20.4, chat, title, action bar, etc.

@56738
Copy link
Contributor Author

56738 commented Dec 19, 2023

Has this been tested to even work back on 1.7.10 when CB shipped relocated gson?

This wouldn't be a new issue caused by these changes tho? It used JsonElement and types from gson before

Seems like there is a small oversight that breaks 1.7.10. This PR only checks subclasses, but on 1.7.10, ChatSerializer itself contains the needed methods. I'll push a fix tomorrow.

@kashike kashike self-assigned this Dec 20, 2023
@kashike kashike merged commit 86a2347 into KyoriPowered:main Dec 20, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants