CryptoMorin
Add XItemBuilder
When loading properties from an ItemStack, the from() method of every registered Property is executed and the property is stored in the ItemBuilder, regardless of whether that property is actually present or needed in the original ItemStack.
For example, if we input a simple ItemStack of two diamonds, all properties in the registry (e.g., BookAuthor, Durability, etc.) will still have their from() method executed, and each property will be added to the builder. Even if they are irrelevant to the item.
To illustrate the current behavior:
for (Map.Entry<Class<? extends Property>, Supplier<? extends Property>> entry : PROPERTIES_REGISTRY.entrySet()) {
Property currentProperty = entry.getValue().get();
currentProperty.from(item, meta);
builder.property(currentProperty); // TODO: Only add if really present in item
}
To solve this, we need a way to determine after calling from() whether a property was actually present on the item. Even then, we still need to consider whether some properties should still be added, even if not present, so that calling to() later can reset the property to their default values on an existing ItemStack.
My suggestion:
public interface Property {
...
boolean isDefault();
...
}
public static final class DisplayName implements ItemMetaProperty {
...
@Override public boolean isDefault() { return displayName == null; }
...
}
public void from(ItemStack item, boolean resetProps) {
...
for (Map.Entry<Class<? extends Property>, Supplier<? extends Property>> entry : PROPERTIES_REGISTRY.entrySet()) {
Property currentProperty = entry.getValue().get();
currentProperty.from(item, meta);
if (resetProps || !currentProperty.isDefault()) {
property(currentProperty);
}
}
}
This way, each property can determine whether it is in a "default" state. After calling from(), we can check if the property represents meaningful data. If not, we can skip it - unless resetProps is explicitly true, in which case we store it anyway to allow a reset via to().
The only problem with this could be for more complex properties if, for example, the default value changes over several versions or has some other funky conditions.
Minor Things
We shouldn't have two classes like LegacyDurability and NewDurability if the intended purpose of the developer is to literally set the durability of the item, we should merge this to a single class and choose the appropriate way to handle this based on the version.
I’ve already removed this functionality, but I still want to clarify that it was never intended for developers to manually choose which durability property to use.
Both properties were wrapped in a third class (PropertyCollection) that internally selected the appropriate implementation based on #isSupported() (See lines 141–177). I thought this would be a good idea because the legacy durability doesn't use an ItemMeta and thus I didn't have to merge ItemMeta / ItemStack behaviour in one property.
You should make every method a single line when the method body is only a single line. [...] Also don't use final for the parameters, it's pointless in most cases. [...]
I agree with most of these design suggestions. However, at this early stage, I don't want to have to deal with formatting the code myself in case major changes are made to the properties. Once I begin implementing all properties, I’ll clean up the style accordingly.
As for final parameters - I like the safety they provide, but I’ll happily skip them for you ;)
metaClass.isInstance() should throw an exception instead of simply returning. This is error-prone and the responsibility of the developer to know which meta is for what item.
I strongly disagree. Throwing here would make the XItemBuilder#from() implementation unnecessarily cumbersome, as we’d only be able to call the from() method of a Property when we're 100% sure the ItemMeta matches the material's meta. I can't think of a pretty way to implement this for the global XItembuilder#from(), which loops through the whole property registry.
At most, I’d consider adding a toggleable warning (if we ever add a logger), but no exceptions.
I agree, we should add a separate static from() from method, but keep this one.
How should the non-static from() behave when called multiple times? Should it simply add the new properties each time?
Merging/copying of XItemBuilder/Property is something I plan to do soon, but it's not a high priority for me right now.
I was thinking of changing the signature to:
class Property {
boolean from(ItemStack item);
}
Which is more cleaner without needing a separate method. Check the result and add it depending on if the item had the property or not. I think we shouldn't keep properties that literally don't exist. We could simply remove these properties if someone wants to do that with a global to(ItemStack item, boolean removeNonExisting) but practically this would be rarely used. For example, if you have a strict ItemStack reference that you can only change, otherwise you could simply build the item from scratch using build()
As for metaClass.isInstance() now that I think about it, you're right. it's fine the way it is.
How should the non-static from() behave when called multiple times? Should it simply add the new properties each time?
It should have an option to determine whether only new properties should be added or it should override.
public static void from(ItemStack item, boolean override)
Just another small thing, we could further simplify register() by just passing the constructor:
private static <T extends Property> void register(Supplier<T> creator) {
PROPERTIES_REGISTRY.put(creator.get().getClass(), creator);
}
I know this is a weird code. Obviously this is not as efficient, but it's a static initialization and this small performance difference is irrelevant. This however has another advantage. This simply makes sure static initialization and constructor of these properties do not throw errors. If any errors do occur, we can see them right away instead of waiting for them to happen later once the properties are actually used. These types of errors will rarely happen, but let's assume for example a new ItemMeta was added in a new Minecraft version, the isSupported() will use a cached variable inside the property, if the cached variable does not properly perform the check, it should throw an error. We could also add a try-catch to the register() method to make this more lenient to make the class usable for other properties until the issue is fixed.
What do you think?
That being said, make sure properties that rely on existence of a class actually perform a Class.forName check not supports() from XReflection or XMaterial. You can make a specialized method for this just like XItemStack's onlyIf(). isSupported() is only if the developer manually used a specific property in an older version.
I think we shouldn't keep properties that literally don't exist. We could simply remove these properties if someone wants to do that with a global to(ItemStack item, boolean removeNonExisting) [...]
I agree, in the context of the from() method, we shouldn't store properties that don't actually exist. It makes much more sense to implement that kind of logic in the to() method instead. For that, I would simply use an XItemBuilder initialized with all properties from the registry (see #deleteAll()).
That said, I believe the behaviour of an empty property always deleting a property from an ItemStack is useful. I was thinking of a "template ItemBuilder" that could for example be used to modify player inventory items directly. For example:
XItemBuilder unenchant = new XItemBuilder().property(Enchantments()).property(Name("Unenchanted due to ..."));
unenchant.to(player.getPlayerInventory().get(0));
If we want to support something like that, having an isDefault() method would make sense. But at that point, we should probably also consider adding getters to property classes, which would complete the whole feature set: creating, editing, reading and removing ItemStack properties in a version-independent way.
I know this is a weird code. Obviously this is not as efficient, but it's a static initialization and this small performance difference is irrelevant. This however has another advantage. This simply makes sure static initialization and constructor of these properties do not throw errors. If any errors do occur, we can see them right away instead of waiting for them to happen later once the properties are actually used. These types of errors will rarely happen, but let's assume for example a new ItemMeta was added in a new Minecraft version, the isSupported() will use a cached variable inside the property, if the cached variable does not properly perform the check, it should throw an error. We could also add a try-catch to the register() method to make this more lenient to make the class usable for other properties until the issue is fixed. What do you think?
I get your point, and I’ve implemented it that way.
However, I wouldn’t add a try-catch around everything. Catching all exceptions feels a bit off, and as you mentioned, the likelihood of such an error occurring is pretty low anyway. I don’t think we should be afraid of using our own code, otherwise we would end up wrapping everything in try-catch blocks. That said, I don’t have a strong opinion here, so if you still think it’s a good idea, I’m happy to add it.
That being said, make sure properties that rely on existence of a class actually perform a Class.forName check not supports() from XReflection or XMaterial. You can make a specialized method for this just like XItemStack's onlyIf().
In what cases would XMaterial#supports() return true while Class.forName() would fail?
Should I replace all uses of XMaterial#supports() with onlyIf() then?
isSupported() is only if the developer manually used a specific property in an older version.
I didn’t quite understand what you mean by that. Could you explain again?
I agree there should be a way to manually remove properties. In that case, we can keep isDefault() but for manually removing, we should have:
class XItemBuilder {
public XItemStack remove(Class<? extends Property> property) {
properties.put(property, PROPERTIES_REGISTRY.get(property).get())
}
}
(Don't forget to throw an exception if the Material or Amount properties are being removed)
That also means we don't need to change void from(...); signature to return a bool.
In what cases would XMaterial#supports() return true while Class.forName() would fail?
A class existing is sometimes implementation-specific. Of course, most of them try to follow Spigot as their upstream, but that's not always the case, specially with Paper's hardfork announcement and specially some hybrid server software can cause issues too. Also, the fact that we actually have to keep track of what thing was added when, we can simply check if the class exists and be done with it. For example, there were times where Minecraft pushed an update with new features, but Spigot didn't have an API ready for them in their early builds yet, so the minor and patch version numbers match, but the build number doesn't. So we do this to prevent issues on outdated builds.
I didn’t quite understand what you mean by that. Could you explain again?
I was basically saying if you look at XItemStack's onlyIf() usage you will see that the properties are only registered if they exist in that version. So that means the global from(ItemStack...) method will not even have these properties registered to check isSupported() on them, and isSupported() should only be effectively called when a developer manually adds a property that was introduced in later versions. This saves memory and CPU usage. But this also means that in the remove(Class) method I mentioned above, the PROPERTIES_REGISTRY.get() might return null, so make sure to check that.
I guess you have the green flag to add the rest of the properties? Btw, thank you for putting your time and effort into this.
The LambdaMetaProperty originally accepted a Class<? extends ItemMeta> in its constructor. Obviously, that doesn’t work if the meta class doesn’t exist, it throws an exception. I’ve addressed this by wrapping the class reference in a Supplier, which is only called after #isSupported. I believe this solves the issue, though I haven’t had the chance to fully test it yet.
So at this point, it’s the responsibility of #isSupported to ensure the class actually exists and prevent a ClassNotFoundException. That said, I’m wondering if it might make more sense to combine the Class.forName check in #isSupported with the #getMetaClass method, instead of keeping them separate.
(Don't forget to throw an exception if the Material or Amount properties are being removed)
I had assumed that "removing" the amount property would simply set the ItemStack amount to 1, as that’s the default.
The material property is a bit special right now. It only implements the #from method. The #to method is effectively handled inside the #build method, which just uses XMaterial#parseItem.
I considered this acceptable since calling ItemStack#setType would clear all of the item’s existing properties anyway, so we would need to add futher logic to apply it first and so on.
This also makes #resetAll less complicated. It blindly applies all properties from the registry, but since material does nothing in that context, it avoids a special check.
What do you think? Should we keep this special behavior, or would it be better to allow changing the material of an already existing ItemStack? I'm open to both approaches.
I guess you have the green flag to add the rest of the properties?
I’ve gone ahead and done a cleanup, everything is formatted as you requested (hopefully). Before I continue adding the remaining properties, it might be worth doing a full code review. In particular, it would be helpful to take a close look at the current property implementations to make sure they’re solid and this is the way to go for other properties.
Btw, thank you for putting your time and effort into this.
And likewise, thank you for creating such a fantastic project :)
You should cache checkMetaAvailable calls. E.g. it's done for Durability but not BookAuthor
I'm not really a fan of the isSupportedCached() design. Just save them all as a simple boolean variable just like the Durability property.
I’m wondering if it might make more sense to combine the Class.forName check in #isSupported with the #getMetaClass method, instead of keeping them separate.
Catching NoClassDefFoundError from metaClass.get() could be a valid solution, but we still need to cache it using a private static final boolean not one that requires a map or a non-constant variable.
As for the formatting, the lambda classes can have their methods packed in a single line, except that long constructor of course. Same with SimpleProperty and the with... methods. If you think the line will get too long, you can keep @Override in a separate line.
Also, Property class doesn't need new lines between methods (isSupported() should be in a single line too)
As for removing Amount and Material properties, I don't think a developer should be allowed to manually "remove" the amount property. Simply because "removing" that property wouldn't make sense. If someone wants to set an item's amount to 1, they should use withAmount(). Same with materials. It doesn't make sense to "remove" the material of an item.
I don't see the resetAll() method you're talking about so I don't know what that method is supposed to do in detail, but we should be able to change the material of a XItemBuilder but of course it'd be better to check if a certain property is supported by the specific material we're using. That however needs a lot of more coding for each property. It's up to you if you want to do it. We could postpone this specific feature for future versions.
Btw, you should write some tests too. A separate test class instead of having them inside XSeriesTests class. You can check here to see how you can run these tests.
You should cache checkMetaAvailable calls. E.g. it's done for Durability but not BookAuthor I'm not really a fan of the isSupportedCached() design. Just save them all as a simple boolean variable just like the Durability property.
That adds one line per property. Plus, it's not a strictly enforced pattern, so it could easily be forgotten. With isSupportedCached(), I've abstracted away the responsibility for caching the result of #isSupported. But I get the performance benefit of doing it your way and thus I implemented it like this now.
Catching NoClassDefFoundError from metaClass.get() could be a valid solution, but we still need to cache it using a private static final boolean not one that requires a map or a non-constant variable.
I'll leave this as-is for now. Maybe this is something to revisit in a future update.
As for the formatting, the lambda classes can have their methods packed in a single line, except that long constructor of course. Same with SimpleProperty and the with... methods. If you think the line will get too long, you can keep
@Overridein a separate line. Also, Property class doesn't need new lines between methods (isSupported() should be in a single line too
I think this formatting style should only apply to concrete property implementations. For base/abstract classes, I prefer conventional formatting for better maintainability. The goal here is mostly to reduce file size, which matters more in the case of the many property classes.
I agree with your point about the with... methods. I’ve updated that accordingly.
As for removing Amount and Material properties, I don't think a developer should be allowed to manually "remove" the amount property. Simply because "removing" that property wouldn't make sense. If someone wants to set an item's amount to 1, they should use withAmount(). Same with materials. It doesn't make sense to "remove" the material of an item.
I agree it doesn’t make much sense from this perspective. Maybe instead of thinking of it as "removing", we treat it more like "resetting". I wanted to rename the methods to "reset" anyway. For materials though I fully agree.
I don't see the resetAll() method you're talking about so I don't know what that method is supposed to do in detail,
Ah, my bad. I meant #deleteAll(). The naming is still a bit WIP right now.
I currently use "remove" for removing properties from the ItemBuilder itself, and "delete" for resetting/deleting a property from an actual already existing ItemStack. I might rename "delete" to "reset" for clarity.
but we should be able to change the material of a XItemBuilder
That is currently possible. Doing something like property(new Material(...)) will simply override the existing one, since each property can only exist once in the IdentityHashMap.
I was actually referring to changing the material of an already existing ItemStack.
but of course it'd be better to check if a certain property is supported by the specific material we're using. That however needs a lot of more coding for each property. It's up to you if you want to do it. We could postpone this specific feature for future versions.
Yes, I think we should revisit this in the future.
Btw, you should write some tests too.
Sure thing! I'll get into that soon.
Unfortunately, I was in the exam phase until recently and therefore didn't have much time to work on it. I'll be working on it more actively again soon. I just pushed some old commits, but that's basically all I did until now.
Quality Gate passed
Issues
6 New issues
0 Accepted issues
Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code
See analysis details on SonarQube Cloud
![sonarqubecloud[bot] avatar](https://avatars.githubusercontent.com/in/12526?v=4 "Published by a pub.dev verified publisher"){kind=small}
I haven’t been able to work on this PR for some time, and I currently don’t have a timeline to continue it. I may reopen it in the future once I find enough time.