feat: Implement game exit on ALT+F4 and window close

[githubawn]

Fixes #1356

This PR modernizes the application's lifecycle management by unifying scattered quit logic into a single, robust pipeline. It ensures the game can be closed cleanly at any time, regardless of whether the exit was triggered by the in-game menu, Alt+F4, or a Windows shutdown event.

Consolidated fragmented shutdown paths into GameLogic::quit(Bool toDesktop). This handles essential cleanup like stopping recordings, unpausing the engine, and managing the multiplayer self-destruct sequence.
Message-Driven Shutdown: Intercepted WM_CLOSE and WM_QUERYENDSESSION to inject MSG_META_DEMO_INSTANT_QUIT into the message stream. This ensures the engine follows a consistent, synchronized teardown process.

Integrated MSG_SELF_DESTRUCT handling into the exit sequence. This ensures that when a player quits, a message is broadcast to the network first, preventing synchronization errors or "stuck" players in multiplayer matches.

Updated WM_CLOSE to intelligently intercept exit requests. The first attempt to close the window during a match now brings up the in-game Quit Menu; repeating the command (or using Alt+F4) triggers the full sequenced quit and self-destruct.

Applied the 0xFFF0 mask to WM_SYSCOMMAND per Win32 documentation, which was previously absent, to ensure reliable handling of system commands like SC_KEYMENU.

[greptile-apps]

Greptile Summary

This PR unifies fragmented quit paths across Alt+F4, the window close button, and Windows shutdown events into a single GameLogic::quit(Bool toDesktop) pipeline. The key mechanisms are: a QuitGameException to abort loading in-progress, isReadyForMessages() to distinguish pre-game from in-game shutdown, and a two-press design where the first close attempt shows the in-game quit menu and the second triggers the actual teardown with network self-destruct. Most issues from prior review rounds are addressed in this revision.

Confidence Score: 5/5

Safe to merge; only P2 style findings remain after this revision round

All previously-flagged P1 issues (skirmish guard, WM_QUERYENDSESSION null dereference, tryStartNewGame visibility, MSG_META_DEMO_INSTANT_QUIT debug crash, etc.) are addressed. The only new finding is a P2 naming/documentation concern on isMovieAbortRequested(). No P0 or P1 issues were identified in this pass.

GameLogic.cpp (both Generals and GeneralsMD) contains the most complex new logic and warrants careful QA of the multiplayer loading-abort path

Important Files Changed

Filename	Overview
Generals/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp	Adds quit(Bool toDesktop) centralizing shutdown logic, QuitGameException mechanism for loading-abort, and tryStartNewGame wrapper; complex branching logic handles loading, MP, and desktop-quit paths with m_quitToDesktopAfterMatch
GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp	Mirror of Generals changes: same quit(), QuitGameException, tryStartNewGame additions; equivalent logic and edge-case coverage
Generals/Code/Main/WinMain.cpp	Adds WM_QUERYENDSESSION and replaces the hard _exit() WM_CLOSE path with a message-driven shutdown; adds WM_SYSCOMMAND 0xFFF0 mask fix; null guards are present
GeneralsMD/Code/Main/WinMain.cpp	Refines existing WM_QUERYENDSESSION/WM_CLOSE handlers with null+quitting guards and isReadyForMessages() check; previously raw appendMessage calls are now safely guarded
Generals/Code/GameEngine/Source/GameClient/GameClient.cpp	Introduces isMovieAbortRequested() static helper consolidating ESC/quit detection; has undocumented side-effects (message pump, keyboard state mutation) inconsistent with predicate naming
Core/GameEngine/Source/GameClient/GUI/LoadScreen.cpp	Replaces inline ESC-key polling with isMovieAbortRequested() calls; adds isQuitToDesktopRequested() check in LoadScreen::update(); ChallengeLoadScreen min-spec loop now returns early before frameDecompress() on abort
Generals/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/QuitMenu.cpp	Delegates shutdown to GameLogic::quit(); adds canOpenQuitMenu() helper; ToggleQuitMenu guard simplified; self-destruct and cleanup logic removed from menu callbacks
Generals/Code/GameEngine/Source/GameLogic/System/GameLogicDispatch.cpp	Adds m_quitToDesktopAfterMatch check to clearGameData() to trigger setQuitting(TRUE) on deferred desktop-quit; resets flag after use
Generals/Code/GameEngine/Include/GameLogic/GameLogic.h	Adds quit(), isQuitToDesktopRequested(), tryStartNewGame(), and m_quitToDesktopAfterMatch; encapsulation is good with tryStartNewGame private and member exposed via const getter
Generals/Code/GameEngine/Source/Common/MessageStream.cpp	Adds isReadyForMessages() implementation checking ThePlayerList != nullptr; simple and correctly gating the message path

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[WM_CLOSE / Alt+F4 / WM_QUERYENDSESSION] --> B{isReadyForMessages?}
    B -- Yes --> C[appendMessage MSG_META_DEMO_INSTANT_QUIT]
    B -- No --> D[TheGameEngine->setQuitting TRUE]
    C --> E[CommandXlat processes message]
    E --> F[GameLogic::quit TRUE]
    F --> G{isInGame?}
    G -- No --> H[setQuitting TRUE setClientQuiet TRUE]
    G -- Yes --> I{isInInteractiveGame?}
    I -- Yes --> J{canOpenQuitMenu?}
    J -- Yes --> K[ToggleQuitMenu return - first press]
    J -- No --> L{isInMultiplayerGame && !isSkirmish && !sandbox?}
    L -- Yes --> M[appendMessage MSG_SELF_DESTRUCT]
    L -- No --> N[continue]
    M --> N
    N --> O{toDesktop?}
    O -- Yes MP --> P[m_quitToDesktopAfterMatch=TRUE exitGame if not loading]
    O -- Yes SP --> Q{isNotLoading?}
    Q -- Yes --> R[clearGameData setQuitting TRUE]
    Q -- No --> S[setQuitting TRUE directly]
    O -- No --> T[exitGame MSG_CLEAR_GAME_DATA]
    P --> U[updateLoadProgress throws QuitGameException setQuitting TRUE]
    I -- No --> O

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: Generals/Code/GameEngine/Source/GameClient/GameClient.cpp
Line: 333-368

Comment:
**`isMovieAbortRequested()` has undocumented write side-effects**

The function is named and called like a boolean predicate, but it modifies shared state on every call: it invokes `TheGameEngine->serviceWindowsOS()` (which processes the Windows message queue), calls `TheKeyboard->UPDATE()`, and marks the ESC key as "used" via `io->setUsed()`. Callers in `LoadScreen.cpp` invoke this in tight per-frame loops where multiple calls per frame are possible. The keyboard state mutation means only the first call detects an ESC press, which is intentional but invisible at call-sites.

A comment on the function documenting these side-effects (or a name like `checkAndServiceMovieAbort`) would prevent accidental misuse by future callers who add a redundant `isMovieAbortRequested()` check without expecting the message-pump and keyboard side-effects.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: GeneralsMD/Code/GameEngine/Source/GameClient/GameClient.cpp
Line: 836-871

Comment:
**`isMovieAbortRequested()` has undocumented write side-effects**

Same as `Generals/Code/GameEngine/Source/GameClient/GameClient.cpp`: the function modifies the Windows message queue (via `serviceWindowsOS()`), updates keyboard state, and marks the ESC key used on every call, but its predicate-style name and `static` signature don't signal this to callers. A descriptive comment or a name that reflects the pump-and-check nature would prevent future misuse.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (32): Last reviewed commit: "Merge branch 'main' into fix/graceful-ex..." | Re-trigger Greptile}

[greptile-apps]

_{8 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[xezon]

Needs rebase.

My initial thought on this, without looking at code, is that it will make it easier for players to disconnect from a Multiplayer match - by accident or intentional - so this feature must not work during a Multiplayer Match.

[githubawn]

Will rebase soon.

The multiplayer behavior was designed based on your comment from #1356 (comment).
Rather than blocking Alt+F4 entirely, it intercepts the OS close command to ensure a MSG_SELF_DESTRUCT (Surrender) message is sent over the network before the game quits, so other players aren't left hanging. Happy to adjust if your thinking has changed since then!

It also adds safe abort logic so Alt+F4 works consistently everywhere addressing helmutbuhler's #1356 (comment) concern about the button only working in some situations.

[Caball009]

Thank you for implementing this. I did a few quick tests and the current implementation appears to work well.

[greptile-apps]

_{16 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[xezon]

It would have been easier to review if this change just contained Core+GeneralsMD edits for starters.

[greptile-apps]

Additional Comments (1)

Generals/Code/GameEngine/Source/GameClient/GameClient.cpp
TheKeyboard->UPDATE() called on every invocation of isMovieAbortRequested()

isMovieAbortRequested() is called in tight video playback loops (e.g. spinning on isFrameReady() with Sleep(1) between iterations). Each call unconditionally invokes TheKeyboard->UPDATE(), which may accumulate keyboard state incorrectly or cause events to be double-processed when the function is called at high frequency within a single logical frame.

The original code called UPDATE() once per outer loop iteration for a specific keyboard check. The refactored helper should either guard against repeated calls (e.g. a static frame counter check) or the callers should call it once per outer iteration rather than inside the innermost spin loop.

The identical implementation in GeneralsMD/Code/GameEngine/Source/GameClient/GameClient.cpp has the same concern.

Prompt To Fix With AI

This is a comment left during a code review.
Path: Generals/Code/GameEngine/Source/GameClient/GameClient.cpp
Line: 803-806

Comment:
**`TheKeyboard->UPDATE()` called on every invocation of `isMovieAbortRequested()`**

`isMovieAbortRequested()` is called in tight video playback loops (e.g. spinning on `isFrameReady()` with `Sleep(1)` between iterations). Each call unconditionally invokes `TheKeyboard->UPDATE()`, which may accumulate keyboard state incorrectly or cause events to be double-processed when the function is called at high frequency within a single logical frame.

The original code called `UPDATE()` once per outer loop iteration for a specific keyboard check. The refactored helper should either guard against repeated calls (e.g. a static frame counter check) or the callers should call it once per outer iteration rather than inside the innermost spin loop.

The identical implementation in `GeneralsMD/Code/GameEngine/Source/GameClient/GameClient.cpp` has the same concern.

How can I resolve this? If you propose a fix, please make it concise.

[xezon]

Looking good

[xezon]

Greptile also says:

Comment:
GetAsyncKeyState timing is unreliable for Alt+F4 detection

GetAsyncKeyState(VK_MENU) and GetAsyncKeyState(VK_F4) sample the current physical key state at the moment WM_CLOSE is dispatched. Because WM_CLOSE is posted (not sent) by the OS after the WM_SYSKEYDOWN event, a fast key release or message-queue latency can result in both keys reading as up even when Alt+F4 was the trigger — causing isAltF4 to be FALSE when it should be TRUE. The same code is duplicated in GeneralsMD/Code/Main/WinMain.cpp. Since force does not currently alter quit()'s behaviour the impact is cosmetic, but if force is ever used to skip the confirmation dialog this will silently mis-classify Alt+F4 presses as X-button clicks.

[xezon]

Greptile has 2 more comments.

[githubawn]

@greptile
Your last scan appears to be broken.
I have fixed these listed issues in earlier commits.
Do a fresh scan then list all remaining issues if any.

[githubawn]

Is the last P2 a concern? isMovieAbortRequested is quite localized. Happy to hear.

[helmutbuhler]

Looks like this PR broke Replay Checks in CI. @githubawn can you take a look?