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: minecraft vanilla top level description text and mixed cases #599

Merged
merged 3 commits into from
Aug 11, 2024

Conversation

CosminPerRam
Copy link
Member

Closes #598

@xCausxn
Copy link
Contributor

xCausxn commented Aug 9, 2024

What if description.text is a value but also has description.extra?

@CosminPerRam
Copy link
Member Author

Yep, maybe we should get rid of those two conditions (last 2 ifs), and just name = description?.text || ' ' + {processExtras(description.extra)}.
I'm not really sure of it as this would be a breaking change (whereas the fix I've done is not unless a server's name is JUST empty spaces (this is also partial as the default name is a empty string)), cause we would be suddenly changing some queries name output.

@CosminPerRam
Copy link
Member Author

The best solution here is to just do what Minecraft is doing internally, but I don't think we have access to that at all nor will they provide that, I'll have to look into it.

@xCausxn
Copy link
Contributor

xCausxn commented Aug 9, 2024

Well we can just use the second if condition for both cases as we just need to set name to description.text and if description.extra does exist continue to append the text.

I think that would satisfy both cases without being a breaking change.

I did have a working impl for the velocity fix but you got the PR in sooner 😅

@xCausxn
Copy link
Contributor

xCausxn commented Aug 9, 2024

The best solution here is to just do what Minecraft is doing internally, but I don't think we have access to that at all nor will they provide that, I'll have to look into it.

Apologies about the formatting, but as a reference I just did an example in this branch based on the current code.

xCausxn@8e8f411

@CosminPerRam
Copy link
Member Author

Hey, I reviewed this again and I ended up doing what I mentioned in my first reply.
I don't think .trim is needed at the end, I queried a bunch of random servers to make sure stuff looks alright, have you found one that has at the end a bunch of whitespaces that shouldn't be there?

@CosminPerRam CosminPerRam changed the title fix: minecraft vanilla top level description text fix: minecraft vanilla top level description text and mixed cases Aug 11, 2024
@CosminPerRam CosminPerRam force-pushed the fix/minecraft_empty_top_desc_text branch from 3d637c4 to 9fdb1f4 Compare August 11, 2024 11:36
@CosminPerRam CosminPerRam merged commit b6ac839 into master Aug 11, 2024
8 checks passed
@joshhsoj1902
Copy link

Something about this change appears to not work as expected.

I use gamedig as part of some automated tests to confirm that gameservers start properly. Currently I've been using the "Name" field to test against.

It has started duplicating the server name in the name field (the name should just be "LinuxGSM"):

{"name":"LinuxGSMLinuxGSM","map":"","password":false,"raw":{"vanilla":{"name":"","map":"","password":false,"raw":{"version":{"name":"Paper 1.20.2","protocol":764},"enforcesSecureChat":true,"description":{"text":"LinuxGSM"},"players":{"max":20,"online":0},"bcc":{}},"version":"","maxplayers":20,"numplayers":0,"players":[],"bots":[],"queryPort":25565,"connect":"172.30.0.1:25565","ping":4}},"version":"Paper 1.20.2","maxplayers":20,"numplayers":0,"players":[],"bots":[],"queryPort":25565,"connect":"172.30.0.1:25565","ping":4}

I'll likely refactor my tests so that the server name doesn't matter, but I thought I would let you know just in case this is unintended.

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.

bug: Minecraft name property is empty - motd with hex colors
3 participants