Improve handling of empty item groups in the creative menu (#3600)

* Better handling of empty item groups in pagination

Empty/non-displayable item groups are now put after all other groups and only then are they split into pages.

* Serendipitious bugfix

Fixed an issue where empty item groups could get selected when switching pages

* checkstyle

(cherry picked from commit f9b333cc5c)

fabric-item-group-api-v1/src/client/java/net/fabricmc/fabric/mixin/itemgroup/client/CreativeInventoryScreenMixin.java

@@ -93,13 +93,10 @@ public abstract class CreativeInventoryScreenMixin<T extends ScreenHandler> exte
 
 	private void fabric_updateSelection() {
 		if (!fabric_isGroupVisible(selectedTab)) {
-			ItemGroups.getGroups().stream()
+			ItemGroups.getGroups()
+					.stream()
 					.filter(this::fabric_isGroupVisible)
-					.min((a, b) -> {
-						if (a.isSpecial() && !b.isSpecial()) return 1;
-						if (!a.isSpecial() && b.isSpecial()) return -1;
-						return 0;
-					})
+					.min((a, b) -> Boolean.compare(a.isSpecial(), b.isSpecial()))
 					.ifPresent(this::setSelectedTab);
 		}
 	}
@@ -144,7 +141,7 @@ public abstract class CreativeInventoryScreenMixin<T extends ScreenHandler> exte
 	}
 
 	private boolean fabric_isGroupVisible(ItemGroup itemGroup) {
-		return fabric_currentPage == fabric_getPage(itemGroup);
+		return itemGroup.shouldDisplay() && fabric_currentPage == fabric_getPage(itemGroup);
 	}
 
 	private static int fabric_getPage(ItemGroup itemGroup) {

fabric-item-group-api-v1/src/main/java/net/fabricmc/fabric/mixin/itemgroup/ItemGroupsMixin.java

@@ -45,6 +45,7 @@ import net.minecraft.item.ItemGroup;
 import net.minecraft.item.ItemGroups;
 import net.minecraft.registry.Registries;
 import net.minecraft.registry.RegistryKey;
+import net.minecraft.registry.entry.RegistryEntry;
 
 import net.fabricmc.fabric.impl.itemgroup.FabricItemGroup;
 
@@ -54,21 +55,40 @@ public class ItemGroupsMixin {
 	private static final int TABS_PER_PAGE = FabricItemGroup.TABS_PER_PAGE;
 
 	@Inject(method = "collect", at = @At("HEAD"), cancellable = true)
-	private static void collect(CallbackInfo ci) {
+	private static void deferDuplicateCheck(CallbackInfo ci) {
+		/*
+		 * Defer the duplication checks to when fabric performs them (see mixin below).
+		 * It is preserved just in case, but fabric's pagination logic should prevent any from happening anyway.
+		 */
+		ci.cancel();
+	}
+
+	@Inject(method = "updateEntries", at = @At("TAIL"))
+	private static void paginateGroups(CallbackInfo ci) {
 		final List<RegistryKey<ItemGroup>> vanillaGroups = List.of(BUILDING_BLOCKS, COLORED_BLOCKS, NATURAL, FUNCTIONAL, REDSTONE, HOTBAR, SEARCH, TOOLS, COMBAT, FOOD_AND_DRINK, INGREDIENTS, SPAWN_EGGS, OPERATOR, INVENTORY);
 
 		int count = 0;
 
-		// Sort the item groups to ensure they are in a deterministic order.
-		final List<RegistryKey<ItemGroup>> sortedItemGroups = Registries.ITEM_GROUP.getKeys().stream()
-				.sorted(Comparator.comparing(RegistryKey::getValue))
+		Comparator<RegistryEntry.Reference<ItemGroup>> entryComparator = (e1, e2) -> {
+			// Non-displayable groups should come last for proper pagination
+			int displayCompare = Boolean.compare(e1.value().shouldDisplay(), e2.value().shouldDisplay());
+
+			if (displayCompare != 0) {
+				return -displayCompare;
+			} else {
+				// Ensure a deterministic order
+				return e1.registryKey().getValue().compareTo(e2.registryKey().getValue());
+			}
+		};
+		final List<RegistryEntry.Reference<ItemGroup>> sortedItemGroups = Registries.ITEM_GROUP.streamEntries()
+				.sorted(entryComparator)
 				.toList();
 
-		for (RegistryKey<ItemGroup> registryKey : sortedItemGroups) {
-			final ItemGroup itemGroup = Registries.ITEM_GROUP.getOrThrow(registryKey);
+		for (RegistryEntry.Reference<ItemGroup> reference : sortedItemGroups) {
+			final ItemGroup itemGroup = reference.value();
 			final FabricItemGroup fabricItemGroup = (FabricItemGroup) itemGroup;
 
-			if (vanillaGroups.contains(registryKey)) {
+			if (vanillaGroups.contains(reference.registryKey())) {
 				// Vanilla group goes on the first page.
 				fabricItemGroup.setPage(0);
 				continue;
@@ -99,7 +119,5 @@ public class ItemGroupsMixin {
 				throw new IllegalArgumentException("Duplicate position: (%s) for item groups %s vs %s".formatted(position, displayName, existingName));
 			}
 		}
-
-		ci.cancel();
 	}
 }

fabric-item-group-api-v1/src/testmod/java/net/fabricmc/fabric/test/item/group/ItemGroupTest.java

@@ -69,6 +69,10 @@ public class ItemGroupTest implements ModInitializer {
 
 		// Add a differently damaged pickaxe to all groups
 		ItemGroupEvents.MODIFY_ENTRIES_ALL.register((group, content) -> {
+			if (group.getIcon() == ItemStack.EMPTY) { // Leave the empty groups empty
+				return;
+			}
+
 			ItemStack minDmgPickaxe = new ItemStack(Items.DIAMOND_PICKAXE);
 			minDmgPickaxe.setDamage(1);
 			content.prepend(minDmgPickaxe);
@@ -78,6 +82,17 @@ public class ItemGroupTest implements ModInitializer {
 			content.add(maxDmgPickaxe);
 		});
 
+		// Regression test for #3566
+		for (int j = 0; j < 20; j++) {
+			Registry.register(
+					Registries.ITEM_GROUP,
+					new Identifier(MOD_ID, "empty_group_" + j),
+					FabricItemGroup.builder()
+							.displayName(Text.literal("Empty Item Group: " + j))
+							.build()
+			);
+		}
+
 		for (int i = 0; i < 100; i++) {
 			final int index = i;