Properly Handle Blockdata in Click Event

[erenkarakal]

Problem

If you use the on right click on %blockdata% pattern and click on an entity, it always assumes the object is either an entity or an item type, but not block data, which can cause the server to crash.

Solution

Moved the blockdata check a bit earlier to fix the issue and added an instanceof ItemType check to prevent possible crashes in the future.

Testing Completed

Tested on right click on pig, on right click on stone, on right click on stone_button[] and on right click on stone_button[powered=false] to ensure nothing was broken with this fix.

---

Completes: #7885
Related: none

[Absolutionism]

imo, kinda weird having the conditions go from
object instanceof to entity != null then to object instanceof

[Absolutionism]

Also, maybe this can be a good opportunity for a JUnit test.

[erenkarakal]

imo, kinda weird having the conditions go from object instanceof to entity != null then to object instanceof

it's either that or I check instanceof BlockData twice, unless I'm missing something

[Absolutionism]

it's either that or I check instanceof BlockData twice, unless I'm missing something

Why would you need to check it twice? I believe the only real fix was just adding the object instanceof ItemType itemType inside the entity != null block

[erenkarakal]

it's either that or I check instanceof BlockData twice, unless I'm missing something

Why would you need to check it twice? I believe the only real fix was just adding the object instanceof ItemType itemType inside the entity != null block

there would be one inside there and another outside (the one I moved)

[Absolutionism]

I just tested with this

return type.check(event, (Predicate<Object>) object -> {
	if (entity != null) {
		if (object instanceof EntityData<?> entityData) {
			return entityData.isInstance(entity);
		} else if (object instanceof ItemType itemType) {
			Relation compare = DefaultComparators.entityItemComparator.compare(EntityData.fromEntity(entity), itemType);
			return Relation.EQUAL.isImpliedBy(compare);
		}
	} else if (object instanceof ItemType itemType) {
		return itemType.isOfType(block);
	} else if (blockDataCheck != null && object instanceof BlockData blockData)  {
		return blockDataCheck.matches(blockData);
	}
	return false;
});

You wouldnt need a instanceof BlockData check inside.
The reason why the ItemType has a check inside is because some items and their respective entities share the same name/codename. So if we're given an ItemType we check to see if that item has a matching entity, which is stored in the aliases.

BlockData are specific, the user has to put [] at the end. In which case we shouldn't need to check for a matching entity

[APickledWalrus]

It might not hurt to have a comment there to clarify why a BlockData check isn't necessary (even though an ItemType check is present)

[erenkarakal]

It might not hurt to have a comment there to clarify why a BlockData check isn't necessary (even though an ItemType check is present)

its for comparing like a boat item to an entity boat if that's what you are asking

[Absolutionism]

its for comparing like a boat item to an entity boat if that's what you are asking

He was saying to put a comment in the code

[erenkarakal]

language barrier :(