[WIP] Fixes to multiplayer

[FrozenAlex]

Summary by CodeRabbit

• Bug Fixes
  • Improved multiplayer synchronization by preventing unnecessary update notifications during per-player difficulty and modifier settings initialization and synchronization.

[coderabbitai]

Walkthrough

A guard flag skipUpdateHandler is introduced in MultiplayerCore::UI to suppress unwanted packet sends triggered by UI setter side effects during programmatic toggle initialization and network synchronization operations.

Changes

Cohort / File(s)	Summary
UI Toggle Guard Pattern src/UI/MpPerPlayerUI.cpp	Added shared guard flag skipUpdateHandler to prevent setter-triggered updates during initialization (DidActivate) and packet synchronization. Flag is toggled around programmatic value changes and checked in set_PerPlayerDifficulty and set_PerPlayerModifiers to early-return when guarded.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

🐰 A flag stands guard with whispered might,
Preventing echoes through the night,
When toggles dance in sync so true,
No phantom packets slip on through!
Guard and release in harmony,
Pure intent, at last, runs free. 🛡️✨

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title '[WIP] Fixes to multiplayer' is vague and overly generic, using non-descriptive language that fails to convey the specific technical change (guard flag for UI update synchronization).	Replace with a more specific title that describes the actual change, such as 'Add skipUpdateHandler guard to prevent UI update feedback loops' or 'Suppress setter side effects during multiplayer synchronization'.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (2)

src/UI/MpPerPlayerUI.cpp (2)

172-175: Use an RAII guard instead of manual true/false toggling.

The current pattern is fragile and easy to break in future edits. A scoped guard makes reset automatic and keeps these sections safe.

♻️ Proposed refactor (scoped guard)

 namespace MultiplayerCore::UI {
-    // Setting toggle triggers setters, guard every set_Value with this to prevent unwanted state resets
-    bool skipUpdateHandler = false;
+    // Setting toggle triggers setters, guard every set_Value with this to prevent unwanted state resets
+    bool skipUpdateHandler = false;
+
+    struct ScopedUpdateHandlerSkip {
+        bool& flag;
+        explicit ScopedUpdateHandlerSkip(bool& f) : flag(f) { flag = true; }
+        ~ScopedUpdateHandlerSkip() { flag = false; }
+    };

-                skipUpdateHandler = true; // set_Value triggers on update
+                ScopedUpdateHandlerSkip guard(skipUpdateHandler); // set_Value triggers on update
                 ppdt->set_Value(false);
                 ppmt->set_Value(false);
-                skipUpdateHandler = false;

-            skipUpdateHandler = true;
+            ScopedUpdateHandlerSkip guard(skipUpdateHandler);
             ppdt->set_Value(packet->PPDEnabled);
             ppmt->set_Value(packet->PPMEnabled);
-            skipUpdateHandler = false;

Also applies to: 233-236

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/UI/MpPerPlayerUI.cpp` around lines 172 - 175, Replace the manual toggling
of skipUpdateHandler around the two set_Value calls with a scoped RAII guard
that sets skipUpdateHandler = true on construction and resets it to false on
destruction (e.g., implement a small guard class like SkipUpdateGuard). Update
the code around ppdt->set_Value(false) and ppmt->set_Value(false) (and the
identical block at the other location) to instantiate that guard at the start of
the scope so the flag is automatically restored even on early
returns/exceptions.

---

49-50: Scope the update guard to the MpPerPlayerUI instance, not namespace state.

Using a namespace-level mutable flag risks cross-instance interference if more than one MpPerPlayerUI exists/re-initializes. This guard should be instance-owned (this->_skipUpdateHandler) to keep behavior isolated per UI controller.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/UI/MpPerPlayerUI.cpp` around lines 49 - 50, The namespace-level flag
skipUpdateHandler should be converted to an instance member on MpPerPlayerUI to
avoid cross-instance interference: add a private bool _skipUpdateHandler
(initialized in MpPerPlayerUI constructor), remove the namespace variable, and
update all guarded sites (where set_Value and other setters check
skipUpdateHandler) to use this->_skipUpdateHandler instead; ensure any code that
toggles the guard (set to true/false) is updated to operate on the instance
member so each MpPerPlayerUI controls its own update guard.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/UI/MpPerPlayerUI.cpp`:
- Around line 172-175: Replace the manual toggling of skipUpdateHandler around
the two set_Value calls with a scoped RAII guard that sets skipUpdateHandler =
true on construction and resets it to false on destruction (e.g., implement a
small guard class like SkipUpdateGuard). Update the code around
ppdt->set_Value(false) and ppmt->set_Value(false) (and the identical block at
the other location) to instantiate that guard at the start of the scope so the
flag is automatically restored even on early returns/exceptions.
- Around line 49-50: The namespace-level flag skipUpdateHandler should be
converted to an instance member on MpPerPlayerUI to avoid cross-instance
interference: add a private bool _skipUpdateHandler (initialized in
MpPerPlayerUI constructor), remove the namespace variable, and update all
guarded sites (where set_Value and other setters check skipUpdateHandler) to use
this->_skipUpdateHandler instead; ensure any code that toggles the guard (set to
true/false) is updated to operate on the instance member so each MpPerPlayerUI
controls its own update guard.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9aeee1c5-7fe0-483b-9963-b28db58e7cd4

📥 Commits

Reviewing files that changed from the base of the PR and between 62521f7 and 6f3fb40.

📒 Files selected for processing (1)

• src/UI/MpPerPlayerUI.cpp