Fix Item Comparison Issues

src/main/java/ch/njol/skript/aliases/ItemData.java

@@ -32,6 +32,7 @@
 
 import org.bukkit.Bukkit;
 import org.bukkit.Material;
+import org.bukkit.OfflinePlayer;
 import org.bukkit.block.Block;
 import org.bukkit.block.BlockState;
 import org.bukkit.enchantments.Enchantment;
@@ -41,6 +42,7 @@
 import org.bukkit.inventory.ItemStack;
 import org.bukkit.inventory.meta.ItemMeta;
 import org.bukkit.inventory.meta.PotionMeta;
+import org.bukkit.inventory.meta.SkullMeta;
 import org.bukkit.inventory.meta.SpawnEggMeta;
 import org.bukkit.potion.PotionData;
 import org.eclipse.jdt.annotation.Nullable;
@@ -79,6 +81,7 @@ public static class OldItemData {
 	static final MaterialRegistry materialRegistry;
 
 	private static final boolean SPAWN_EGG_META_EXISTS = Skript.classExists("org.bukkit.inventory.meta.SpawnEggMeta");
+	private static final boolean HAS_NEW_SKULL_META_METHODS = Skript.methodExists(SkullMeta.class, "getOwningPlayer");
 
 	// Load or create material registry
 	static {
@@ -404,28 +407,28 @@ private static MatchQuality compareItemMetas(ItemMeta first, ItemMeta second) {
 		String ourName = first.hasDisplayName() ? first.getDisplayName() : null;
 		String theirName = second.hasDisplayName() ? second.getDisplayName() : null;
 		if (!Objects.equals(ourName, theirName)) {
-			quality = ourName != null ? MatchQuality.SAME_MATERIAL : quality;
+			quality = ourName != null ? MatchQuality.SAME_MATERIAL : theirName != null ? MatchQuality.SAME_MATERIAL : quality;
 		}
 
 		// Lore
 		List<String> ourLore = first.hasLore() ? first.getLore() : null;
 		List<String> theirLore = second.hasLore() ? second.getLore() : null;
 		if (!Objects.equals(ourLore, theirLore)) {
-			quality = ourLore != null ? MatchQuality.SAME_MATERIAL : quality;
+			quality = ourLore != null ? MatchQuality.SAME_MATERIAL : theirLore != null ? MatchQuality.SAME_MATERIAL : quality;
 		}
 
 		// Enchantments
 		Map<Enchantment, Integer> ourEnchants = first.getEnchants();
 		Map<Enchantment, Integer> theirEnchants = second.getEnchants();
 		if (!Objects.equals(ourEnchants, theirEnchants)) {
-			quality = !ourEnchants.isEmpty() ? MatchQuality.SAME_MATERIAL : quality;
+			quality = !ourEnchants.isEmpty() ? MatchQuality.SAME_MATERIAL : !theirEnchants.isEmpty() ? MatchQuality.SAME_MATERIAL : quality;
 		}
 
 		// Item flags
 		Set<ItemFlag> ourFlags = first.getItemFlags();
 		Set<ItemFlag> theirFlags = second.getItemFlags();
 		if (!Objects.equals(ourFlags, theirFlags)) {
-			quality = !ourFlags.isEmpty() ? MatchQuality.SAME_MATERIAL : quality;
+			quality = !ourFlags.isEmpty() ? MatchQuality.SAME_MATERIAL : !theirFlags.isEmpty() ? MatchQuality.SAME_MATERIAL : quality;
 		}
 
 		// Potion data
@@ -450,6 +453,25 @@ private static MatchQuality compareItemMetas(ItemMeta first, ItemMeta second) {
 			return !Objects.equals(ourSpawnedType, theirSpawnedType) ? MatchQuality.SAME_MATERIAL : quality;
 		}
 
+		// Skull owner
+		if (second instanceof SkullMeta) {
+			if (!(first instanceof SkullMeta)) {
+				return MatchQuality.DIFFERENT; // Second is a skull, first is clearly not
+			}
+			// Compare skull owners
+			if (HAS_NEW_SKULL_META_METHODS) {
+				OfflinePlayer ourOwner = ((SkullMeta) first).getOwningPlayer();
+				OfflinePlayer theirOwner = ((SkullMeta) second).getOwningPlayer();
+				return !Objects.equals(ourOwner, theirOwner) ? MatchQuality.SAME_MATERIAL : quality;
+			} else { // Use old methods
+				@SuppressWarnings("deprecation")
+				String ourOwner = ((SkullMeta) first).getOwner();
+				@SuppressWarnings("deprecation")
+				String theirOwner = ((SkullMeta) second).getOwner();
+				return !Objects.equals(ourOwner, theirOwner) ? MatchQuality.SAME_MATERIAL : quality;
+			}
+		}
+
 		return quality;
 	}
 
@@ -460,7 +482,7 @@ private static MatchQuality compareItemMetas(ItemMeta first, ItemMeta second) {
 	 * @return If this item can be considered the default item of its type.
 	 */
 	public boolean isDefault() {
-		return itemFlags == 0 && blockValues == null;
+		return isAlias || (itemFlags == 0 && blockValues == null);
 	}
 
 	/**

src/main/java/ch/njol/skript/aliases/ItemType.java

@@ -742,12 +742,14 @@ public final boolean removeFrom(final List<ItemStack>... lists) {
 					 * it to return true for two "same items", even if their
 					 * item meta is completely different.
 					 */
-					if (is != null && d.matchAlias(new ItemData(is)).isAtLeast(MatchQuality.EXACT)) {
+					ItemData other = is != null ? new ItemData(is) : null;
+					if (other != null && other.matchAlias(d).isAtLeast((d.isDefault() && !other.isDefault()) ? MatchQuality.SAME_MATERIAL : MatchQuality.EXACT)) {
 						if (all && amount == -1) {
 							list.set(i, null);
 							removed = 1;
 							continue;
 						}
+						assert is != null;
 						final int toRemove = Math.min(is.getAmount(), getAmount() - removed);
 						removed += toRemove;
 						if (toRemove == is.getAmount()) {

src/main/java/ch/njol/skript/classes/data/DefaultComparators.java

@@ -20,6 +20,7 @@
 
 import java.util.Objects;
 
+import ch.njol.skript.aliases.MatchQuality;
 import ch.njol.skript.util.GameruleValue;
 import ch.njol.skript.util.EnchantmentType;
 import ch.njol.skript.util.Experience;
@@ -193,7 +194,19 @@ public boolean supportsOrdering() {
 		Comparators.registerComparator(ItemType.class, ItemType.class, new Comparator<ItemType, ItemType>() {
 			@Override
 			public Relation compare(final ItemType i1, final ItemType i2) {
-				return Relation.get(i2.isSupertypeOf(i1));
+				if (i1.isAll() != i2.isAll())
+					return Relation.NOT_EQUAL;
+				if (i1.getAmount() != i2.getAmount())
+					return Relation.NOT_EQUAL;
+				for (ItemData myType : i1.getTypes()) {
+					for (ItemData otherType : i2.getTypes()) {
+						// Don't require an EXACT match if the other ItemData is an alias. They only need to share a material.
+						if (myType.matchAlias(otherType).isAtLeast((otherType.isDefault() && !myType.isDefault()) ? MatchQuality.SAME_ITEM : MatchQuality.EXACT)) {
+							return Relation.EQUAL;
+						}
+					}
+				}
+				return Relation.NOT_EQUAL;
 			}
 
 			@Override

src/test/skript/tests/regressions/2581-all potions are water.sk

@@ -1,5 +1,5 @@
 test "potion aliases" when minecraft version is not "1.9.4":
-	# Disabled on 1.9 due to #2586
+	# Disabled on 1.9 - see #2587
 	assert speed potion is not water bottle with "potion alias: speed"
 	assert strength potion is not water bottle with "potion alias: strength"
-	assert long strength potion is not water bottle with "potion alias: long strength"
+	assert long strength potion is not water bottle with "potion alias: long strength"

src/test/skript/tests/regressions/3419-item comparisons.sk

@@ -0,0 +1,126 @@
+test "item comparisons":
+
+	# Basic Material Comparisons
+
+	set {_a} to a dirt block
+	set {_b} to a diamond block
+
+	assert {_a} is a dirt block with "{_a} is not a dirt block (it should be)"
+	assert {_a} is not a diamond block with "{_a} is a diamond block (it shouldn't be)"
+
+	assert {_b} is not a dirt block with "{_b} is a dirt block (it shouldn't be)"
+	assert {_b} is a diamond block with "{_b} is not a diamond block (it should be)"
+
+	assert {_a} is not {_b} with "{_a} is {_b} (it shouldn't be) (Basic Material Comparisons)"
+	assert {_b} is not {_a} with "{_b} is {_a} (it shouldn't be) (Basic Material Comparisons)"
+
+	# Different Name Comparisons
+
+	set {_a} to a diamond
+	set {_b} to a diamond named "Fake Diamond"
+
+	assert {_a} is a diamond with "{_a} is not a diamond (it should be)"
+	assert {_a} is a diamond named "" with "{_a} is not an unnamed diamond (it should be)"
+	assert {_a} is not a diamond named "Fake Diamond" with "{_a} is a diamond named ""Fake Diamond"" (it shouldn't be)"
+
+	assert {_b} is a diamond with "{_b} is not a diamond (it should be)"
+	assert {_b} is not a diamond named "" with "{_b} is an unnamed diamond (it shouldn't be)"
+	assert {_b} is a diamond named "Fake Diamond" with "{_b} is not a diamond named ""Fake Diamond"" (it should be)"
+
+	assert {_a} is not {_b} with "{_a} is {_b} (it shouldn't be) (Different Name Comparisons)"
+	assert {_b} is {_a} with "{_b} is not {_a} (it should be) (Different Name Comparisons)"
+
+	# Different Enchantment Comparisons
+
+	set {_a} to a dirt block of sharpness 1
+	set {_b} to a dirt block of sharpness 2
+
+	assert {_a} is a dirt block with "{_a} is not a dirt block (it should be)"
+	assert {_a} is a dirt block of sharpness 1 with "{_a} is not a dirt block of sharpness 1 (it should be)"
+	assert {_a} is not a dirt block of sharpness 2 with "{_a} is a dirt block of sharpness 2 (it shouldn't be)"
+	assert a dirt block of sharpness 1 is {_a} with "a dirt block of sharpness 1 is not {_a} (it should be)"
+
+	assert {_b} is a dirt block with "{_b} is not a dirt block (it should be)"
+	assert {_b} is not a dirt block of sharpness 1 with "{_b} is a dirt block of sharpness 1 (it shouldn't be)"
+	assert {_b} is a dirt block of sharpness 2 with "{_b} is not a dirt block of sharpness 2 (it should be)"
+	assert a dirt block of sharpness 1 is not {_b} with "a dirt block of sharpness 1 is {_b} (it shouldn't be)"
+
+	assert {_a} is not {_b} with "{_a} is {_b} (it shouldn't be) (Different Enchantment Comparisons"
+	assert {_b} is not {_a} with "{_b} is {_a} (it shouldn't be) (Different Enchantment Comparisons)"
+
+	# Same Material and Same Lore BUT Different Enchantment
+
+	set {_a} to a diamond of sharpness 1 with lore "Lore 1" and "Lore 2"
+	set {_b} to a diamond of sharpness 2 with lore "Lore 1" and "Lore 2"
+
+	assert {_a} is a diamond of sharpness 1 with lore "Lore 1" and "Lore 2" with "{_a} is not a diamond of sharpness 1 with lore ""Lore 1"" and ""Lore 2"" (it should be)"
+	assert {_a} is not a diamond of sharpness 2 with lore "Lore 1" and "Lore 2" with "{_a} is a diamond of sharpness 2 with lore ""Lore 1"" and ""Lore 2"" (it shouldn't be)"
+
+	assert {_b} is a diamond of sharpness 2 with lore "Lore 1" and "Lore 2" with "{_b} is not a diamond of sharpness 2 with lore ""Lore 1"" and ""Lore 2"" (it should be)"
+	assert {_b} is not a diamond of sharpness 1 with lore "Lore 1" and "Lore 2" with "{_b} is a diamond of sharpness 1 with lore ""Lore 1"" and ""Lore 2"" (it shouldn't be)"
+
+	assert {_a} is not {_b} with "{_a} is {_b} (it shouldn't be) (Same Material and Same Lore BUT Different Enchantment)"
+	assert {_b} is not {_a} with "{_b} is {_a} (it shouldn't be) (Same Material and Same Lore BUT Different Enchantment)"
+
+	# Skull Comparisons
+
+	set {_a} to a skeleton skull named "Skulliest Skull"
+	set {_b} to an ender dragon skull named "Skully Skull"
+
+	assert {_a} is a skeleton skull with "{_a} is not a skeleton skull (it should be)"
+	assert {_a} is not a skeleton skull named "" with "{_a} is an unnamed skeleton skull (it shouldn't be)"
+	assert {_a} is a skeleton skull named "Skulliest Skull" with "{_a} is not a skeleton skull named ""Skulliest Skull"" (it should be)"
+
+	assert {_a} is not an ender dragon skull with "{_a} is an ender dragon skull (it should't be)"
+	assert {_a} is not an ender dragon skull named "Skulliest Skull" with "{_a} is an ender dragon skull named ""Skulliest Skull"" (it shouldn't be)"
+
+	assert {_b} is an ender dragon skull with "{_b} is not an ender dragon skull (it should be)"
+	assert {_b} is not an ender dragon skull named "" with "{_b} is an unnamed ender dragon skull (it shouldn't be)"
+	assert {_b} is an ender dragon skull named "Skully Skull" with "{_b} is not an ender dragon skull named ""Skully Skull"" (it should be)"
+
+	assert {_b} is not a skeleton skull with "{_b} is a skeleton skull (it shouldn't be)"
+	assert {_b} is not a skeleton skull named "Skully Skull" with "{_b} is a skeleton skull named ""Skully Skull"" (it shouldn't be)"
+
+	assert {_a} is not {_b} with "{_a} is {_b} (it shouldn't be) (Skull Comparisons)"
+	assert {_b} is not {_a} with "{_b} is {_a} (it shouldn't be) (Skull Comparisons)"
+
+	# Player Skull Comparisons
+
+	set {_p1} to "APickledWalrus" parsed as an offlineplayer
+	set {_p2} to "ShaneBee" parsed as an offlineplayer
+	set {_a} to skull of {_p1}
+	set {_b} to skull of {_p2}
+
+	assert {_a} is a player skull with "{_a} is not a player skull (it should be)"
+	assert {_a} is the skull of {_p1} with "{_a} is not the skull of {_p1} (it should be)"
+	assert {_a} is not the skull of {_p2} with "{_a} is the skull of {_p2} (it shouldn't be)"
+
+	assert {_b} is a player skull with "{_b} is not a player skull (it should be)"
+	assert {_b} is the skull of {_p2} with "{_b} is not the skull of {_p2} (it should be)"
+	assert {_b} is not the skull of {_p1} with "{_b} is the skull of {_p1} (it shouldn't be)"
+
+	assert {_a} is not {_b} with "{_a} is {_b} (it shouldn't be) (Player Skull Comparisons)"
+	assert {_b} is not {_a} with "{_b} is {_a} (it shouldn't be) (Player Skull Comparisons)"
+
+	# Remove / Remove All Tests
+
+	set block at spawn of world "world" to air
+	set block at spawn of world "world" to a chest
+
+	set {_inventory} to inventory of block at spawn of world "world"
+	add 64 diamonds and 64 diamonds named "Fake Diamond" to {_inventory}
+
+	remove 4 diamonds named "" from {_inventory}
+	assert {_inventory} contains 60 diamonds and 64 diamonds named "Fake Diamond" with "4 unnamed diamonds were not properly removed."
+
+	remove 4 diamonds named "Fake Diamond" from {_inventory}
+	assert {_inventory} contains 60 diamonds and 60 diamonds named "Fake Diamond" with "4 named diamonds were not properly removed."
+
+	set {_size} to number of diamonds in {_inventory}
+	assert {_size} = 120 with "{_inventory} should have 120 diamonds in it, but it doesn't. Number of diamonds: '%{_size}%'"
+
+	assert {_inventory} is not empty with "{_inventory} is empty, but it shouldn't be."
+	remove all diamonds from {_inventory}
+	assert {_inventory} is empty with "{_inventory} still has diamonds, but it shouldn't."
+
+	set block at spawn of world "world" to air