Avoid relying on package name to parse CraftBukkit version.

[portlek]

Fix #294

the pr might be a bit confusing. here are the things done:

• revision names from wrapper class names removed.
• searches the version wrapper starting from the server versions' patch version to 0.
• move the special version classes to their own modules.
• implemented a simple version parser using Bukkit#getVersion().

tested on 1.8.6, 1.8.8, 1.10.2, 1.17.1.

[mastercake10]

What's the point of still having the R-Version in the module name while removing them in the class names? This is inconsistent.

Also, do we really need all these new classes in the API module? Afaik you copied them partially from the PaperLib, why can't they be shaded in? Its not ideal to have unattributed code from others in the repo. As we discussed in #294, we don't even need a lib for this. Getting the minor, major and patch version is really just parsing the version string.

[portlek]

What's the point of still having the R-Version in the module name while removing them in the class names? This is inconsistent.

Although I agree with what you are saying, I think it should stay the same because if we change the module names, every single artifactId in the maven repository will change and that might not be what @WesJD wants. It's up to him tho.

Also, do we really need all these new classes in the API module? Afaik you copied them partially from the PaperLib, why can't they be shaded in? It's not ideal to have unattributed code from others in the repo. As we discussed in #294, we don't even need a lib for this. Getting the minor, major and patch version is really just parsing the version string.

The reason why I didn't shade the paperlib in is because it's containing too many unnecessary features in it. I just copied the necessary part which parses the Minecraft version using Bukkit#getVersion() method, which is basically what you are saying here:

Getting the minor, major and patch version is really just parsing the version string.

Thanks for the feedback.

[mastercake10]

every single artifactId in the maven repository will change

Why is this a problem? The modules get shaded in the final jar anyway, I don't think people use the Wrapper classes explicitly in their plugins. Even then they can still rename the module.

I think if we're going to ditch the R-Version, we should do it right, which means renaming the packages too.

The reason why I didn't shade the paperlib in is because it's containing too many unnecessary features in it. I just copied the necessary part which parses the Minecraft version using Bukkit#getVersion() method, which is basically what you are saying here:

Do you think we can use their code without attribution? I am not an expert when it comes to licenses, but it doesn't feel right. I think this can be done without using PaperLib at all. We don't even use the methods Environment#isPaper etc.

[portlek]

Why is this a problem? The modules get shaded in the final jar anyway, I don't think people use the Wrapper classes explicitly in their plugins. Even then they can still rename the module.

I think if we're going to ditch the R-Version, we should do it right, which means renaming the packages too.

idk I didn't say it's a problem, I just don't know what WesJD thinks about that. I'm willing to change the module names but it's not up to me.

Do you think we can use their code without attribution? I am not an expert when it comes to licenses, but it doesn't feel right. I think this can be done without using PaperLib at all.

It's an MIT license, which basically means that you do whatever you want with it. And plus, I've put the license file from the paperlib repository as well.

We don't even use the methods Environment#isPaper etc.

Yea, I agree, I can remove the isPaper/isSpigot parts and other unused methods as well.

[portlek]

remove almost every class and end up with a single version parser which makes things a lot simpler.

[mastercake10]

Looking good.

It's an MIT license, which basically means that you do whatever you want with it. And plus, I've put the license file from the paperlib repository as well.

Yeah, I know. But it still needs attribution, which is basically what I was saying. The LICENSE file is empty btw. Mabye put it in the header of the file(s) so others know which parts are exactly taken from paper / licensed under MIT? I was thinking we could just avoid using PaperLib since its really not that hard to parse a simple string.

[portlek]

Yeah, I know. But it still needs attribution, which is basically what I was saying. The LICENSE file is empty btw. Mabye put it in the header of the file(s) so others know which parts are exactly taken from paper / licensed under MIT? I was thinking we could just avoid using PaperLib since its really not that hard to parse a simple string.

didn't notice it was empty. i've removed the license and everything related to paperlib so we should be good now.

[mastercake10]

didn't notice it was empty. i've removed the license and everything related to paperlib so we should be good now.

Parts of the code are still from PaperLib, so it needs the attribution + license.

[portlek]

we don't need to add them, but I still add it to make you feel ok. it's not a required thing to do if it's a mit license. you just don't need to do that.

[0dinD]

In regards to the renaming of the packages and Maven artifacts: I think it's fine, since most (if not all) users of AnvilGUI probably just depend on the main AnvilGUI artifact, not the individual adapter modules. So the rename will not affect them.

If we make the decision to keep the old "R-version" module/package names, I think we should also keep the old class names, for consistency. Then we would need to have a hybrid approach where we use the new naming scheme for 1.20.5+ and fall back to the old "R-version" naming scheme for older modules. But for what it's worth, if I had to choose, I would ditch the "R-versions" entirely and rename the packages and Maven artifacts.

---

In regards to the version parsing, I am quite convinced that we could just use Bukkit.getServer().getBukkitVersion().split("-")[0] instead of trying to parse Bukkit.getVersion(), as discussed in https://github.com/WesJD/AnvilGUI/issues/294#issuecomment-2087862734. The reason why PaperLib does that is because it gives a few more bits of information, such as information about whether the Minecraft version is a pre-release or release candidate. But I don't see how this information is important to us. And if we use Bukkit.getServer().getBukkitVersion().split("-")[0], we can avoid copying from PaperLib at all and simplify the VersionProvider class.

But maybe there is some advantage to parsing Bukkit.getVersion() that I have missed?

[portlek]

In regards to the version parsing, I am quite convinced that we could just use Bukkit.getServer().getBukkitVersion().split("-")[0] instead of trying to parse Bukkit.getVersion(), as discussed in #294 (comment). The reason why PaperLib does that is because it gives a few more bits of information, such as information about whether the Minecraft version is a pre-release or release candidate. But I don't see how this information is important to us. And if we use Bukkit.getServer().getBukkitVersion().split("-")[0], we can avoid copying from PaperLib at all and simplify the VersionProvider class.

But maybe there is some advantage to parsing Bukkit.getVersion() that I have missed?

Bukkit#getVersion() is much more simple to work with in terms of the knowledge about the way Bukkit works. The main reason why I think that is because Bukkit#getBukkitVersion() defined by Bukkit, but Bukkit#getVersion() method is using the thing that represents the Mojang's implementation. So if MinecraftServer#getServerVersion() implementation changes, which is highly unlikely, then we will know that immediately and change the version pattern in the version provider. But let's say if some random fork decided to change CraftServer#getBukkitVersion(), we cannot catch that change easily since we cannot know every single spigot/paper fork on the planet.

[0dinD]

Bukkit#getVersion() is much more simple to work with in terms of the knowledge about the way Bukkit works. The main reason why I think that is because Bukkit#getBukkitVersion() defined by Bukkit, but Bukkit#getVersion() method is using the thing that represents the Mojang's implementation. So if MinecraftServer#getServerVersion() implementation changes, which is highly unlikely, then we will know that immediately and change the version pattern in the version provider. But let's say if some random fork decided to change CraftServer#getBukkitVersion(), we cannot catch that change easily since we cannot know every single spigot/paper fork on the planet.

Okay, that is a fair point. Although I honestly find it hard to believe that a fork would change the getBukkitVersion implementation that drastically. At that point, what's to say that they don't also change getVersion to return something in a different format as well? Like, neither of those methods actually make any guarantees about the return string format, as far as I know. But then again, I would agree that getBukkitVersion is more likely to change than getVersion, so honestly I'm fine with your solution.

[0dinD]

Personally I would have liked to see either sticking entirely with the old "R-version" names for both the modules and the class names (which would lead to a very simple PR with minimal changes) or the complete opposite where we remove all traces of "R-versions", even from the module names. Currently, we have an inconsistent middle ground.

But for what it's worth, I still approve of the current solution, because it will work, and at the end of the day that's what really matters.

[0dinD]

Looks like these dependencies have not been updated to use the new module names:

https://github.com/forkstuffs/AnvilGUI/blob/90fcd18557b98ac2a1a45efca509f2a4296a1bdd/api/pom.xml#L28-L195

[WesJD]

@portlek Can you confirm which versions you tested the latest commit on?

[portlek]

@portlek Can you confirm which versions you tested the latest commit on?

i didn't test the latest commits. whenever i get time, i will test on different versions.

[WesJD]

@mastercake10 @0dinD @portlek could someone test the latest commit so we can get this out?

[mastercake10]

@mastercake10 @0dinD @portlek could someone test the latest commit so we can get this out?

I think this should be fine, I've tested it on a few MC versions. Shouldn't break anything.

But I have my gradle fork that already has support for 1.20.5/6. If we decide to use Gradle instead of maven later on, all the changes of this PR would be useless since I haven't renamed the NMS modules.

The reason why I haven't created this PR is that I am not sure if the PaperWrapper will stay the same for the next few minecraft versions, its just speculations at this point.

This is why we proposed not to rename the modules in #294 and take a simple approach instead (something like just to check if the craft bukkit version contains an R, if not, get the minecraft version and find the appropriate "R-Module" through a look up table).

I don't want to void all the work @portlek put into this PR, I really appreciate it. Maybe we should find a middle ground and create a new PR without changing that much to make it work for 1.20.5/6? We can still merge this or my gradle fork when necessary later on.

[WesJD]

I'm good for you to take the lead and make a decision there. If you think it's better to do a simpler PR, just whip one up. I just want to make sure we are on track to support the new versions, as I've been getting a lot of requests for it.

[BySwiizen]

I'm good for you to take the lead and make a decision there. If you think it's better to do a simpler PR, just whip one up. I just want to make sure we are on track to support the new versions, as I've been getting a lot of requests for it.

I think it would be more favorable firstly to make a simple commit which adds the new versions and secondly, to create a branch with all the more technical modifications to be made.

[WesJD]

I'm closing this since we just merged #326 . Let's make these technical changes in another PR if still desired.