Shift + double click only works while holding an item with the mouse

As the title says. This is a very long-standing bug, and it’s hard for me to believe that no one has reported it before. Its existence has likely made the shift + double click feature harder to discover and thus less well known, and perhaps the players who do know about it have been satisfied with that information and not questioned why it is this way. However code analysis confirms that this is indeed a bug.

To reproduce:

1. Open a chest inventory. (Player inventories are affected by MC-19232 which is a separate bug.)

2. Place items of the same type in multiple slots of the chest. Ensure that there is room in the player inventory.

3. Pick up any item with the mouse (from a slot other than the ones filled in step 2).

4. While holding shift, double click on one of the slots from step 2.

5. All stacks of the type in question are moved to the player inventory.

6. Repeat step 2.

7. Repeat step 4, this time without holding an item.

8. Only one stack is moved.

Code analysis:

The shift + double click behavior is triggered in AbstractContainerScreen.mouseReleased under the following conditions:

		if (this.doubleclick && $$1 != null && $$0.button() == 0 && this.menu.canTakeItemForPickAll(ItemStack.EMPTY, $$1)) {
			if ($$0.hasShiftDown()) {
				if (!this.lastQuickMoved.isEmpty()) { // (this is the important bit)

Depending on whether an item is being held with the mouse, the regular shift-click behavior, which always happens first as part of the shift + double click sequence, is triggered either in mouseClicked or mouseReleased. In both cases lastQuickMoved is set to the item stack contained in the targeted slot, regardless of if it’s empty:

					boolean $$9 = $$5 != -999 && $$0.hasShiftDown();
					if ($$9) {
						this.lastQuickMoved = $$1 != null && $$1.hasItem() ? $$1.getItem().copy() : ItemStack.EMPTY;
					}

These are the only places where lastQuickMoved is assigned apart from the constructor. There is no separate code to reset the value. The reasoning behind this, if any, was likely that the combination of this.doubleclick and hasShiftDown() should already imply that lastQuickMoved was set by the shift-click code earlier during the same double-click. TOCTOU issues aside, the bug occurs because when the mouseClicked shift-click path is used (i.e. when not holding an item), the second click is treated as another shift click. At that point the targeted slot will likely be empty as a result of the first shift click, and hence by the time the second click has been released to trigger the shift + double click code, lastQuickMoved will have been reassigned as the empty stack, causing the code to be skipped.

Patch:

The simplest fix i can think of is the following:

--- a/net/minecraft/client/gui/screens/inventory/AbstractContainerScreen.java
+++ b/net/minecraft/client/gui/screens/inventory/AbstractContainerScreen.java
@@ -317,2 +317,5 @@
 			this.skipNextRelease = false;
+			if (!this.doubleclick) {
+				this.lastQuickMoved = ItemStack.EMPTY;
+			}
 			if ($$0.button() != 0 && $$0.button() != 1 && !$$2) {
@@ -354,3 +357,5 @@
 								if ($$8) {
-									this.lastQuickMoved = $$3 != null && $$3.hasItem() ? $$3.getItem().copy() : ItemStack.EMPTY;
+									if ($$3 != null && $$3.hasItem()) {
+										this.lastQuickMoved = $$3.getItem().copy();
+									}
 									$$9 = ClickType.QUICK_MOVE;
@@ -533,3 +538,5 @@
 					if ($$9) {
-						this.lastQuickMoved = $$1 != null && $$1.hasItem() ? $$1.getItem().copy() : ItemStack.EMPTY;
+						if ($$1 != null && $$1.hasItem()) {
+							this.lastQuickMoved = $$1.getItem().copy();
+						}
 					}

which skips assigning lastQuickMoved if the slot is empty, and instead explicitly ensures that it is reset at the start of a click (that isn’t the second click of a double-click sequence). However the click handling code in AbstractContainerScreen contains many subtle logic bugs and should probably be completely rewritten.