Yarn: Slot.onCrafted is wrong

[Juuxel]

See https://github.com/FabricMC/yarn/issues/1656#issuecomment-812829091

[Runemoro]

I think either onSizeChanged or onItemCountChanged would be better. It's the stack size, or item count, being changed, not the stack count (number of stacks).

[liach]

is this still applicable after mojang's recent revamp on screen handlers in like 21w11a?

[Juuxel]

Okay, the original analysis might not be 100% correct. Current behaviour:

• Slot.onStackChanged is still not a general stack change handler.
  
  
  
  • It's called from transferSlot implementations for output slots of various crafting blocks, but also for brewing stand inputs.
  
  
  • The parameters are seemingly newStack, originalStack (swapped from how they're mapped currently).
  
  
  • What it does is checks if there have been items taken out of the slot and if so, calls onCrafted(ItemStack, int) with the originalStack (called newItem in current mappings) and the difference between the stack sizes.
  
  
  • I think this is more of a "on output slot item count changed".
  
  
• Slot.onCrafted(ItemStack, int) is only called by onStackChanged.
  
  
  
  • Default impl: no-op
  
  
  • In CraftingResultSlot, FurnaceOutputSlot and TradeOutputSlot the implementation is always the same: it increments a private amount field by the int parameter, then calls onCrafted(ItemStack) with the item stack parameter.
  
  
• Slot.onCrafted(ItemStack) is only called by onCrafted(ItemStack, int) and onTakeItem implementations in those three classes.
  
  
  
  • Default impl: no-op
  
  
  • Implementations in those three classes: resets the private amount field to 0 and runs finalising tasks (furnace dropping XP etc)
  
  

Conclusion (needs to be confirmed with a debugger or a closer look):

• All these methods relate to the player taking items out of output slots (crafting or trading).
• onStackChanged is called when the item count changes in the output slot, and onCrafted(ItemStack, int) when that count decreases (= items are taken out).
• onCrafted(ItemStack) finalises the output process when there's either a partial decrease (onStackChanged) or a full stack taken out (onTakeItem).
• These methods still need renames.

[Reoseah]

Conclusion (needs to be confirmed with a debugger or a closer look):

• All these methods relate to the player taking items out of output slots (crafting or trading).
• onStackChanged is called when the item count changes in the output slot, and onCrafted(ItemStack, int) when that count decreases (= items are taken out).

The method that calls them, transferSlot, is only used for quick transfer a.k.a. shift clicking. So, onQuickTransfer seems the most accurate to me. Parameter names were indeed switched.

[Juuxel]

This still applies to onCrafted, so reopening.