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 Skript MinecraftServer.isRunning reflection on 1.20(.1) #6352

Merged
merged 3 commits into from
Feb 1, 2024
Merged

Fix Skript MinecraftServer.isRunning reflection on 1.20(.1) #6352

merged 3 commits into from
Feb 1, 2024

Conversation

C0D3-M4513R
Copy link
Contributor

@C0D3-M4513R C0D3-M4513R commented Jan 20, 2024

Related #6351

Adds the obfuscated name for Server#isRunning for 1.20 versions.

Please note that whilst this change is untested, it should be correct.

@sovdeeth sovdeeth added bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. 2.8 Targeting a 2.8.X version release labels Jan 20, 2024
@sovdeeth
Copy link
Member

Does it fix your issue when you test?

@C0D3-M4513R
Copy link
Contributor Author

C0D3-M4513R commented Jan 21, 2024

I have made other changes to Ketting (adding the bukkit method back in). So even if I did compile this (which I didn't) it wouldn't test the relavent code paths.

I don't even have this code locally, and haven't even looked, if this builds with maven or gradle.

Copy link
Member

@sovdeeth sovdeeth left a comment

Choose a reason for hiding this comment

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

Can replicate with Spigot servers, this does fix it.

@skmedix
Copy link

skmedix commented Jan 28, 2024

What if the mappings was saved in a json file (or any other format) in the jar, so you don't have to change the code every major version?

@sovdeeth
Copy link
Member

What if the mappings was saved in a json file (or any other format) in the jar, so you don't have to change the code every major version?

Then we'd just have to change the mappings every version, no? It seems like the same outcome to me.
A better solution would be figuring out how to remove this dependency on NMS entirely.

@skmedix
Copy link

skmedix commented Jan 28, 2024

Then we'd just have to change the mappings every version, no? It seems like the same outcome to me. A better solution would be figuring out how to remove this dependency on NMS entirely.

Yeah, fair enough. My idea was to just have a list of mappings, instead of adding another round of if statements. Kinda cleaner, but you still need to bump the list when needed, but I guess it's fine for now

@sovdeeth
Copy link
Member

Then we'd just have to change the mappings every version, no? It seems like the same outcome to me. A better solution would be figuring out how to remove this dependency on NMS entirely.

Yeah, fair enough. My idea was to just have a list of mappings, instead of adding another round of if statements. Kinda cleaner, but you still need to bump the list when needed, but I guess it's fine for now

Ah I see what you mean. Sure, I think if we need to touch this again we should swap to a switch or a map like your suggestion, but I think we should aim to get rid of it by then if possible.

@sovdeeth sovdeeth changed the base branch from master to dev/patch January 29, 2024 11:16
src/main/java/ch/njol/skript/Skript.java Outdated Show resolved Hide resolved
@APickledWalrus APickledWalrus merged commit 906c5a2 into SkriptLang:dev/patch Feb 1, 2024
4 checks passed
@C0D3-M4513R C0D3-M4513R deleted the 1-20-Minecraft.isRunning-fix branch February 1, 2024 18:27
ShaneBeee pushed a commit to ShaneBeee/Skript that referenced this pull request Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.8 Targeting a 2.8.X version release bug An issue that needs to be fixed. Alternatively, a PR fixing an issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants