Transfer API improvements 3

- **(Slightly source-breaking change)** Change the return type of `Storage#iterator` and `Storage#iterable` from `Iterator<StorageView<T>>` to `Iterator<? extends StorageView<T>>` to allow returning a list directly. Most modders shouldn't be affected by this (this only broke one call site in the whole module).
- Precise that using the iterator or a view after the transaction is closed is "undefined behavior". Also specify that calling remove on the iterator is not allowed.
- Add `StorageView#getUnderlyingView` to be able to tell if some views are equal. This is useful to **compute the contents of multiple storage views without duplicates** (see testmod).
- Expose the lifecycle of the transaction manager cleanly with an enum.
- Definalize some methods in `SingleStackStorage` to allow custom implementations of some of them if needed.
- Add a note to `BlockApiLookup` to fix #1998.
- Play the composter empty sound when it is emptied through the transfer API, as a comment in the source code suggests.

Co-authored-by: Juuxel <6596629+Juuxel@users.noreply.github.com>

fabric-api-lookup-api-v1/src/main/java/net/fabricmc/fabric/api/lookup/v1/block/BlockApiLookup.java

@@ -207,6 +207,10 @@ public interface BlockApiLookup<A, C> {
 	 * The mapping from the parameters of the query to the API is handled by the passed {@code provider}.
 	 * This overload allows using the correct block entity class directly.
 	 *
+	 * <p>Note: The type is not used directly for detecting the supported blocks and block entities in the world, but it is converted to
+	 * its {@linkplain BlockEntityType#blocks} when this method is called.
+	 * If the {@code blocks} field is empty, {@link IllegalArgumentException} is thrown.
+	 *
 	 * @param <T> The block entity class for which an API is exposed.
 	 * @param provider The provider: returns an API if available in the passed block entity with the passed context,
 	 *                 or {@code null} if no API is available.
@@ -223,6 +227,10 @@ public interface BlockApiLookup<A, C> {
 	 * This overload allows registering multiple block entity types at once,
 	 * but due to how generics work in java, the provider has to cast to the correct block entity class if necessary.
 	 *
+	 * <p>Note: The type is not used directly for detecting the supported blocks and block entities in the world, but it is converted to
+	 * its {@linkplain BlockEntityType#blocks} when this method is called.
+	 * If the {@code blocks} field is empty, {@link IllegalArgumentException} is thrown.
+	 *
 	 * @param provider The provider.
 	 * @param blockEntityTypes The block entity types.
 	 */

fabric-transfer-api-v1/src/main/java/net/fabricmc/fabric/api/transfer/v1/item/base/SingleStackStorage.java

@@ -84,27 +84,27 @@ public abstract class SingleStackStorage extends SnapshotParticipant<ItemStack>
 	}
 
 	@Override
-	public final boolean isResourceBlank() {
+	public boolean isResourceBlank() {
 		return getResource().isBlank();
 	}
 
 	@Override
-	public final ItemVariant getResource() {
+	public ItemVariant getResource() {
 		return ItemVariant.of(getStack());
 	}
 
 	@Override
-	public final long getAmount() {
+	public long getAmount() {
 		return getStack().getCount();
 	}
 
 	@Override
-	public final long getCapacity() {
+	public long getCapacity() {
 		return getCapacity(getResource());
 	}
 
 	@Override
-	public final long insert(ItemVariant insertedVariant, long maxAmount, TransactionContext transaction) {
+	public long insert(ItemVariant insertedVariant, long maxAmount, TransactionContext transaction) {
 		StoragePreconditions.notBlankNotNegative(insertedVariant, maxAmount);
 
 		ItemStack currentStack = getStack();
@@ -132,7 +132,7 @@ public abstract class SingleStackStorage extends SnapshotParticipant<ItemStack>
 	}
 
 	@Override
-	public final long extract(ItemVariant variant, long maxAmount, TransactionContext transaction) {
+	public long extract(ItemVariant variant, long maxAmount, TransactionContext transaction) {
 		StoragePreconditions.notBlankNotNegative(variant, maxAmount);
 
 		ItemStack currentStack = getStack();
@@ -154,14 +154,14 @@ public abstract class SingleStackStorage extends SnapshotParticipant<ItemStack>
 	}
 
 	@Override
-	protected final ItemStack createSnapshot() {
+	protected ItemStack createSnapshot() {
 		ItemStack original = getStack();
 		setStack(original.copy());
 		return original;
 	}
 
 	@Override
-	protected final void readSnapshot(ItemStack snapshot) {
+	protected void readSnapshot(ItemStack snapshot) {
 		setStack(snapshot);
 	}
 }

fabric-transfer-api-v1/src/main/java/net/fabricmc/fabric/api/transfer/v1/storage/Storage.java

@@ -17,15 +17,14 @@
 package net.fabricmc.fabric.api.transfer.v1.storage;
 
 import java.util.Iterator;
-import java.util.NoSuchElementException;
 
 import org.jetbrains.annotations.ApiStatus;
 import org.jetbrains.annotations.Nullable;
 
-import net.fabricmc.fabric.api.transfer.v1.fluid.base.SingleFluidStorage;
 import net.fabricmc.fabric.api.transfer.v1.storage.base.CombinedStorage;
 import net.fabricmc.fabric.api.transfer.v1.storage.base.ExtractionOnlyStorage;
 import net.fabricmc.fabric.api.transfer.v1.storage.base.InsertionOnlyStorage;
+import net.fabricmc.fabric.api.transfer.v1.storage.base.SingleVariantStorage;
 import net.fabricmc.fabric.api.transfer.v1.storage.base.SingleViewIterator;
 import net.fabricmc.fabric.api.transfer.v1.transaction.Transaction;
 import net.fabricmc.fabric.api.transfer.v1.transaction.TransactionContext;
@@ -48,7 +47,7 @@ import net.fabricmc.fabric.impl.transfer.TransferApiImpl;
  *     <li>{@link ExtractionOnlyStorage} and {@link InsertionOnlyStorage} can be used when only extraction or insertion is needed.</li>
  *     <li>{@link SingleViewIterator} can be used to wrap a single view for use with {@link #iterator}.</li>
  *     <li>Resource-specific base implementations may also be available.
- *     For example, Fabric API provides {@link SingleFluidStorage} to accelerate implementations of {@code Storage<FluidVariant>}.</li>
+ *     For example, Fabric API provides {@link SingleVariantStorage} to accelerate implementations of transfer variant storages.</li>
  * </ul>
  *
  * <p><b>Important note:</b> Unless otherwise specified, all transfer functions take a non-blank resource
@@ -141,13 +140,11 @@ public interface Storage<T> {
 	 * Iterate through the contents of this storage, for the scope of the passed transaction.
 	 * Every visited {@link StorageView} represents a stored resource and an amount.
 	 * The iterator doesn't guarantee that a single resource only occurs once during an iteration.
+	 * Calling {@linkplain Iterator#remove remove} on the iterator is not allowed.
 	 *
 	 * <p>The returned iterator and any view it returns are only valid for the scope of to the passed transaction.
 	 * They should not be used once that transaction is closed.
-	 *
-	 * <p>More precisely, as soon as the transaction is closed,
-	 * {@link Iterator#hasNext hasNext()} must return {@code false},
-	 * and any call to {@link Iterator#next next()} must throw a {@link NoSuchElementException}.
+	 * Using the iterator or any view once the transaction is closed is undefined behavior.
 	 *
 	 * <p>{@link #insert} and {@link #extract} may be called safely during iteration.
 	 * Extractions should be visible to an open iterator, but insertions are not required to.
@@ -156,9 +153,9 @@ public interface Storage<T> {
 	 * the iteration.
 	 *
 	 * @param transaction The transaction to which the scope of the returned iterator is tied.
-	 * @return An iterator over the contents of this storage.
+	 * @return An iterator over the contents of this storage. Calling remove on the iterator is not allowed.
 	 */
-	Iterator<StorageView<T>> iterator(TransactionContext transaction);
+	Iterator<? extends StorageView<T>> iterator(TransactionContext transaction);
 
 	/**
 	 * Iterate through the contents of this storage, for the scope of the passed transaction.
@@ -168,8 +165,9 @@ public interface Storage<T> {
 	 * @return An iterable over the contents of this storage.
 	 * @see #iterator
 	 */
-	default Iterable<StorageView<T>> iterable(TransactionContext transaction) {
-		return () -> iterator(transaction);
+	@SuppressWarnings({"rawtypes", "unchecked"})
+	default Iterable<? extends StorageView<T>> iterable(TransactionContext transaction) {
+		return () -> (Iterator) iterator(transaction);
 	}
 
 	/**

fabric-transfer-api-v1/src/main/java/net/fabricmc/fabric/api/transfer/v1/storage/StorageView.java

@@ -62,4 +62,16 @@ public interface StorageView<T> {
 	 * or an estimate of the number of resources that could be stored if this view has a blank resource.
 	 */
 	long getCapacity();
+
+	/**
+	 * If this is view is a delegate around another storage view, return the underlying view.
+	 * This can be used to check if two views refer to the same inventory "slot".
+	 * <b>Do not try to extract from the underlying view, or you risk bypassing some checks.</b>
+	 *
+	 * <p>It is expected that two storage views with the same underlying view ({@code a.getUnderlyingView() == b.getUnderlyingView()})
+	 * share the same content, and mutating one should mutate the other. However, one of them may allow extraction, and the other may not.
+	 */
+	default StorageView<T> getUnderlyingView() {
+		return this;
+	}
 }

fabric-transfer-api-v1/src/main/java/net/fabricmc/fabric/api/transfer/v1/storage/base/CombinedStorage.java

@@ -108,7 +108,7 @@ public class CombinedStorage<T, S extends Storage<T>> implements Storage<T> {
 		final TransactionContext transaction;
 		final Iterator<S> partIterator = parts.iterator();
 		// Always holds the next StorageView<T>, except during next() while the iterator is being advanced.
-		Iterator<StorageView<T>> currentPartIterator = null;
+		Iterator<? extends StorageView<T>> currentPartIterator = null;
 
 		CombinedIterator(TransactionContext transaction) {
 			this.transaction = transaction;

fabric-transfer-api-v1/src/main/java/net/fabricmc/fabric/api/transfer/v1/storage/base/FilteringStorage.java

@@ -220,5 +220,10 @@ public abstract class FilteringStorage<T> implements Storage<T> {
 		public long getCapacity() {
 			return backingView.getCapacity();
 		}
+
+		@Override
+		public StorageView<T> getUnderlyingView() {
+			return backingView.getUnderlyingView();
+		}
 	}
 }

fabric-transfer-api-v1/src/main/java/net/fabricmc/fabric/api/transfer/v1/transaction/Transaction.java

@@ -90,10 +90,17 @@ public interface Transaction extends AutoCloseable, TransactionContext {
 	}
 
 	/**
-	 * @return True if a transaction is open on the current thread, and false otherwise.
+	 * @return True if a transaction is open or closing on the current thread, and false otherwise.
 	 */
 	static boolean isOpen() {
-		return TransactionManagerImpl.MANAGERS.get().isOpen();
+		return getLifecycle() != Lifecycle.NONE;
+	}
+
+	/**
+	 * @return The current lifecycle of the transaction stack on this thread.
+	 */
+	static Lifecycle getLifecycle() {
+		return TransactionManagerImpl.MANAGERS.get().getLifecycle();
 	}
 
 	/**
@@ -148,4 +155,23 @@ public interface Transaction extends AutoCloseable, TransactionContext {
 	 */
 	@Override
 	void close();
+
+	enum Lifecycle {
+		/**
+		 * No transaction is currently open or closing.
+		 */
+		NONE,
+		/**
+		 * A transaction is currently open.
+		 */
+		OPEN,
+		/**
+		 * The current transaction is invoking its close callbacks.
+		 */
+		CLOSING,
+		/**
+		 * The current transaction is invoking its outer close callbacks.
+		 */
+		OUTER_CLOSING
+	}
 }

fabric-transfer-api-v1/src/main/java/net/fabricmc/fabric/impl/transfer/item/ComposterWrapper.java

@@ -29,6 +29,8 @@ import org.jetbrains.annotations.Nullable;
 import net.minecraft.block.BlockState;
 import net.minecraft.block.ComposterBlock;
 import net.minecraft.item.Items;
+import net.minecraft.sound.SoundCategory;
+import net.minecraft.sound.SoundEvents;
 import net.minecraft.util.math.BlockPos;
 import net.minecraft.util.math.Direction;
 import net.minecraft.world.World;
@@ -107,6 +109,7 @@ public class ComposterWrapper extends SnapshotParticipant<Float> {
 			// Mimic ComposterBlock#emptyComposter logic.
 			location.setBlockState(location.getBlockState().with(ComposterBlock.LEVEL, 0));
 			// Play the sound
+			location.world.playSound(null, location.pos, SoundEvents.BLOCK_COMPOSTER_EMPTY, SoundCategory.BLOCKS, 1.0F, 1.0F);
 		} else if (increaseProbability > 0) {
 			boolean increaseSuccessful = location.world.getRandom().nextDouble() < increaseProbability;
 

fabric-transfer-api-v1/src/main/java/net/fabricmc/fabric/impl/transfer/item/SidedInventorySlotWrapper.java

@@ -20,6 +20,7 @@ import net.minecraft.inventory.SidedInventory;
 import net.minecraft.util.math.Direction;
 
 import net.fabricmc.fabric.api.transfer.v1.item.ItemVariant;
+import net.fabricmc.fabric.api.transfer.v1.storage.StorageView;
 import net.fabricmc.fabric.api.transfer.v1.storage.base.SingleSlotStorage;
 import net.fabricmc.fabric.api.transfer.v1.transaction.TransactionContext;
 
@@ -74,4 +75,9 @@ class SidedInventorySlotWrapper implements SingleSlotStorage<ItemVariant> {
 	public long getCapacity() {
 		return slotWrapper.getCapacity();
 	}
+
+	@Override
+	public StorageView<ItemVariant> getUnderlyingView() {
+		return slotWrapper.getUnderlyingView();
+	}
 }

fabric-transfer-api-v1/src/main/java/net/fabricmc/fabric/impl/transfer/transaction/TransactionManagerImpl.java

@@ -47,7 +47,7 @@ public class TransactionManagerImpl {
 	public TransactionContext getCurrentUnsafe() {
 		if (currentDepth == -1) {
 			return null;
-		} else if (stack.get(currentDepth).isOpen) {
+		} else if (stack.get(currentDepth).lifecycle == Transaction.Lifecycle.OPEN) {
 			return stack.get(currentDepth);
 		} else {
 			throw new IllegalStateException("May not call getCurrentUnsafe() from a close callback.");
@@ -65,7 +65,7 @@ public class TransactionManagerImpl {
 		}
 
 		TransactionImpl current = stack.get(currentDepth);
-		current.isOpen = true;
+		current.lifecycle = Transaction.Lifecycle.OPEN;
 		return current;
 	}
 
@@ -79,12 +79,18 @@ public class TransactionManagerImpl {
 		}
 	}
 
+	public Transaction.Lifecycle getLifecycle() {
+		if (currentDepth == -1) {
+			return Transaction.Lifecycle.NONE;
+		} else {
+			return stack.get(currentDepth).lifecycle;
+		}
+	}
+
 	private class TransactionImpl implements Transaction {
 		final int nestingDepth;
 		final ArrayList<CloseCallback> closeCallbacks = new ArrayList<>();
-		// This may be false even when the transaction is not fully closed, to prevent callbacks calling other functions in an invalid state.
-		// It is reset to true in TransactionManagerImpl#open.
-		boolean isOpen = false;
+		Lifecycle lifecycle = Lifecycle.NONE;
 
 		TransactionImpl(int nestingDepth) {
 			this.nestingDepth = nestingDepth;
@@ -104,7 +110,7 @@ public class TransactionManagerImpl {
 
 		// Validate that this transaction is open.
 		private void validateOpen() {
-			if (!isOpen) {
+			if (lifecycle != Lifecycle.OPEN) {
 				throw new IllegalStateException("Transaction operation cannot be applied to a closed transaction.");
 			}
 		}
@@ -120,7 +126,7 @@ public class TransactionManagerImpl {
 			validateCurrentTransaction();
 			validateOpen();
 			// Block transaction operations
-			isOpen = false;
+			lifecycle = Lifecycle.CLOSING;
 
 			// Note: it is important that we don't let exceptions corrupt the global state of the transaction manager.
 			// That is why any callback has to run inside a try block.
@@ -142,6 +148,8 @@ public class TransactionManagerImpl {
 			closeCallbacks.clear();
 
 			if (currentDepth == 0) {
+				lifecycle = Lifecycle.OUTER_CLOSING;
+
 				// Invoke outer close callbacks in reverse order
 				for (int i = outerCloseCallbacks.size() - 1; i >= 0; i--) {
 					try {
@@ -160,6 +168,7 @@ public class TransactionManagerImpl {
 
 			// Only this check will allow openOuter operations.
 			currentDepth--;
+			lifecycle = Lifecycle.NONE;
 
 			// Throw exception if necessary
 			if (closeException != null) {
@@ -179,7 +188,7 @@ public class TransactionManagerImpl {
 
 		@Override
 		public void close() {
-			if (isOpen() && isOpen) { // check that a transaction is open on this thread and that this transaction is open.
+			if (isOpen() && lifecycle == Lifecycle.OPEN) { // check that a transaction is open on this thread and that this transaction is open.
 				abort();
 			}
 		}

fabric-transfer-api-v1/src/testmod/java/net/fabricmc/fabric/test/transfer/unittests/TransactionStateTests.java

@@ -18,9 +18,10 @@ package net.fabricmc.fabric.test.transfer.unittests;
 
 import net.fabricmc.fabric.api.transfer.v1.transaction.Transaction;
 
-class TransactionExceptionsTests {
+class TransactionStateTests {
 	public static void run() {
 		testTransactionExceptions();
+		testTransactionLifecycle();
 	}
 
 	private static int callbacksInvoked = 0;
@@ -94,4 +95,22 @@ class TransactionExceptionsTests {
 			throw new AssertionError(message);
 		}
 	}
+
+	private static void testTransactionLifecycle() {
+		TestUtil.assertEquals(Transaction.Lifecycle.NONE, Transaction.getLifecycle());
+
+		try (Transaction transaction = Transaction.openOuter()) {
+			TestUtil.assertEquals(Transaction.Lifecycle.OPEN, Transaction.getLifecycle());
+
+			transaction.addCloseCallback((tx, result) -> {
+				TestUtil.assertEquals(Transaction.Lifecycle.CLOSING, Transaction.getLifecycle());
+			});
+
+			transaction.addOuterCloseCallback(result -> {
+				TestUtil.assertEquals(Transaction.Lifecycle.OUTER_CLOSING, Transaction.getLifecycle());
+			});
+		}
+
+		TestUtil.assertEquals(Transaction.Lifecycle.NONE, Transaction.getLifecycle());
+	}
 }

fabric-transfer-api-v1/src/testmod/java/net/fabricmc/fabric/test/transfer/unittests/UnderlyingViewTests.java

@@ -0,0 +1,63 @@
+/*
+ * Copyright (c) 2016, 2017, 2018, 2019 FabricMC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package net.fabricmc.fabric.test.transfer.unittests;
+
+import java.util.Collections;
+import java.util.Set;
+
+import it.unimi.dsi.fastutil.Hash;
+import it.unimi.dsi.fastutil.objects.Reference2ReferenceOpenCustomHashMap;
+
+import net.minecraft.block.Blocks;
+import net.minecraft.block.entity.FurnaceBlockEntity;
+import net.minecraft.util.math.BlockPos;
+import net.minecraft.util.math.Direction;
+
+import net.fabricmc.fabric.api.transfer.v1.item.InventoryStorage;
+import net.fabricmc.fabric.api.transfer.v1.item.ItemVariant;
+import net.fabricmc.fabric.api.transfer.v1.storage.StorageView;
+
+public class UnderlyingViewTests {
+	public static void run() {
+		testFurnaceSides();
+	}
+
+	/**
+	 * Ensure that only 3 slots with different underlying view exist on all sides of a furnace combined.
+	 */
+	private static void testFurnaceSides() {
+		FurnaceBlockEntity furnace = new FurnaceBlockEntity(BlockPos.ORIGIN, Blocks.FURNACE.getDefaultState());
+
+		Set<StorageView<ItemVariant>> viewSet = Collections.newSetFromMap(new Reference2ReferenceOpenCustomHashMap<>(new Hash.Strategy<>() {
+			@Override
+			public int hashCode(StorageView<ItemVariant> o) {
+				return o == null ? 0 : System.identityHashCode(o.getUnderlyingView());
+			}
+
+			@Override
+			public boolean equals(StorageView<ItemVariant> a, StorageView<ItemVariant> b) {
+				return a == null || b == null ? a == b : a.getUnderlyingView() == b.getUnderlyingView();
+			}
+		}));
+
+		for (Direction direction : Direction.values()) {
+			viewSet.addAll(InventoryStorage.of(furnace, direction).getSlots());
+		}
+
+		TestUtil.assertEquals(3, viewSet.size());
+	}
+}

fabric-transfer-api-v1/src/testmod/java/net/fabricmc/fabric/test/transfer/unittests/UnitTestsInitializer.java

@@ -30,7 +30,8 @@ public class UnitTestsInitializer implements ModInitializer {
 		ItemTests.run();
 		PlayerInventoryStorageTests.run();
 		SingleVariantItemStorageTests.run();
-		TransactionExceptionsTests.run();
+		TransactionStateTests.run();
+		UnderlyingViewTests.run();
 
 		LoggerFactory.getLogger("fabric-transfer-api-v1 testmod").info("Transfer API unit tests successful.");
 	}