bugfix(shell): Prevent menu crashing by avoiding initializing layouts during teardown

[CryoTheRenegade]

Fixes #2777. Shell teardown now suppresses initialization of newly uncovered layouts while retaining each layout’s shutdown callback. This prevents the full-screen save/load menu from accessing GameState after it has been destroyed when closing the game after loading a save. I reproduced the issue with a Zero Hour skirmish save, where the process exited with code 2816 before the change and 0 afterward.

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Fix Shell teardown by suppressing layout init on immediate pop

🐞 Bug fix 🕐 10-20 Minutes

AI Description

• Add option to suppress init when popping layouts immediately.
• Prevent uncovered layouts from initializing during Shell teardown.
• Avoid post-destroy GameState access when exiting after loading a save.

Diagram

graph TD
  A["Shell::deconstruct()"] --> B["popImmediate(TRUE)"] --> C["doPop(suppressInit)"]
  B --> F["WindowLayout::runShutdown()"]
  C -- "if !suppressInit" --> D["Init uncovered layout"] --> E[("GameState")]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Internal teardown flag (no API parameter)

• ➕ Avoids expanding the public Shell API surface
• ➕ Prevents misuse by callers passing the wrong suppressInit value
• ➕ Keeps lifecycle policy centralized within Shell
• ➖ Introduces implicit state that can be harder to reason about in debugging
• ➖ Requires careful reset semantics if Shell can be reused/reconstructed

2. Split doPop into pop + explicit re-init step

• ➕ Makes init behavior explicit and easier to audit at call sites
• ➕ Reduces chance of hidden side effects during pop
• ➖ Larger refactor with more call sites impacted
• ➖ Higher risk of regressions in UI navigation flows

Recommendation: The chosen approach (a defaulted suppressInit parameter on popImmediate forwarded into doPop) is a pragmatic, low-risk fix that targets the teardown path without broad refactoring. If future call sites need similar behavior, consider evolving this into an internal "isDestroying" flag or a clearly named API (e.g., popImmediateWithoutInit) to reduce accidental misuse.

Files changed (2) +5 / -4

Bug fix (2) +5 / -4

Shell.hAdd suppressInit option to Shell::popImmediate() +1/-1

Add suppressInit option to Shell::popImmediate()

• Extends the popImmediate() declaration to accept an optional suppressInit flag (default FALSE). Updates the comment to clarify that init of the uncovered layout can be suppressed.

Core/GameEngine/Include/GameClient/Shell.h

Shell.cppSuppress uncovered-layout init during Shell teardown +4/-3

Suppress uncovered-layout init during Shell teardown

• Updates Shell::deconstruct() to call popImmediate(TRUE) so uncovered layouts do not initialize while destroying the shell. Threads the new suppressInit parameter through popImmediate() into doPop(), preserving shutdown behavior while avoiding teardown-time init side effects.

Core/GameEngine/Source/GameClient/GUI/Shell/Shell.cpp

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Flag semantic mismatch ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

Shell::popImmediate(Bool suppressInit) forwards suppressInit into doPop(Bool impendingPush), where
the parameter name/meaning differs (it controls whether newTop->runInit runs). This semantic
mismatch is introduced by the PR and makes the public API intent harder to follow at the call site.

Code

Core/GameEngine/Source/GameClient/GUI/Shell/Shell.cpp[452]

+	doPop( suppressInit );

Evidence

The new API parameter is named suppressInit, but it is passed into doPop's impendingPush parameter;
doPop uses that boolean to gate whether it calls runInit on the newly exposed layout. This works
today but the mismatch is introduced by this PR and reduces readability/maintainability.

Core/GameEngine/Source/GameClient/GUI/Shell/Shell.cpp[428-456]
Core/GameEngine/Source/GameClient/GUI/Shell/Shell.cpp[689-714]
Core/GameEngine/Include/GameClient/Shell.h[131-170]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`popImmediate(Bool suppressInit)` passes its new flag directly into `doPop(Bool impendingPush)`. Although the polarity currently matches the desired behavior (TRUE => skip `newTop->runInit`), the different naming/abstraction (`suppressInit` vs `impendingPush`) is confusing and brittle.
### Issue Context
`doPop` currently uses its boolean to decide whether to run init on the newly exposed top layout. The PR repurposes this parameter by forwarding `suppressInit` positionally, which obscures intent.
### Fix Focus Areas
- Core/GameEngine/Source/GameClient/GUI/Shell/Shell.cpp[428-714]
- Core/GameEngine/Include/GameClient/Shell.h[131-170]
### Suggested approach
Pick one:
1) Rename the `doPop` parameter (declaration + definition) to reflect what it actually controls (e.g., `suppressInit` or `runInit`), and update call sites accordingly.
2) Keep `doPop(impendingPush)` but make the mapping explicit in `popImmediate`, e.g.:
- `doPop(/*impendingPush=*/suppressInit);` (or)
- `const Bool impendingPush = suppressInit; doPop(impendingPush);`
This preserves behavior while making intent unambiguous.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can tweak Display preferences with a live preview to see your comment before it ships

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[xezon]

Code looks logical.