From ba625b3217fdda691585471dcfb4e2b45e7bfc42 Mon Sep 17 00:00:00 2001
From: Carson McManus <dyc3@users.noreply.github.com>
Date: Fri, 24 Mar 2023 11:46:05 -0400
Subject: [PATCH] don't increment state_id when the server modifies cursor item
 (#306)

## Description

The client actually doesn't acknowledge the state id update when you
modify the cursor item, so don't increment it so we stay in sync.

fixes #304

## Test Plan

Explain the steps necessary to test your changes. If you used a
playground, include the code in the details below.

Steps:
1. Run the playground from #304 (not included here for brevity)
2. open inventory (not chest)
3. pick up one of the stacks
4. see that you don't get resynced, and the apples stay in your cursor
---
 crates/valence/src/inventory.rs | 37 ++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/crates/valence/src/inventory.rs b/crates/valence/src/inventory.rs
index 8fc7ee0..255ba95 100644
--- a/crates/valence/src/inventory.rs
+++ b/crates/valence/src/inventory.rs
@@ -530,7 +530,9 @@ fn update_player_inventories(
         }
 
         if cursor_item.is_changed() && !inv_state.client_updated_cursor_item {
-            inv_state.state_id += 1;
+            // Contrary to what you might think, we actually don't want to increment the
+            // state ID here because the client doesn't actually acknowledge the
+            // state_id change for this packet specifically. See #304.
 
             client.write_packet(&ScreenHandlerSlotUpdateS2c {
                 window_id: -1,
@@ -1501,6 +1503,39 @@ mod test {
         Ok(())
     }
 
+    #[test]
+    fn should_not_increment_state_id_on_cursor_item_change() -> anyhow::Result<()> {
+        let mut app = App::new();
+        let (client_ent, mut client_helper) = scenario_single_client(&mut app);
+
+        let inv_state = app
+            .world
+            .get::<PlayerInventoryState>(client_ent)
+            .expect("could not find client");
+        let expected_state_id = inv_state.state_id.0;
+
+        // Process a tick to get past the "on join" logic.
+        app.update();
+        client_helper.clear_sent();
+
+        let mut cursor_item = app.world.get_mut::<CursorItem>(client_ent).unwrap();
+        cursor_item.0 = Some(ItemStack::new(ItemKind::Diamond, 2, None));
+
+        app.update();
+
+        // Make assertions
+        let inv_state = app
+            .world
+            .get::<PlayerInventoryState>(client_ent)
+            .expect("could not find client");
+        assert_eq!(
+            inv_state.state_id.0, expected_state_id,
+            "state id should not have changed"
+        );
+
+        Ok(())
+    }
+
     mod dropping_items {
         use valence_protocol::block_pos::BlockPos;
         use valence_protocol::packet::c2s::play::click_slot::{ClickMode, Slot};