feat: Implement ALT + F4 and Window X button to close game

[githubawn]

Fixes #1356

Split up in 2 commits.

1. Previously, the quit logic was duplicated and inconsistent across several places (the quit menu, the "quit to desktop" button, and the Alt+F4 handler). This commit centralizes all of that into a single GameLogic::quit(Bool toDesktop) method. It properly handles edge cases like stopping an active recording, sending a self-destruct message in multiplayer (using a new m_quitToDesktopAfterMatch flag), unpausing/unfreezing the game before exiting. All the old scattered quit code is replaced with a simple call to TheGameLogic->quit(true/false).

2. This follow-up extends the graceful exit to two more situations where the game previously couldn't be closed cleanly:
   During the loading screen: The load progress update now checks if a quit has been requested and throws a QuitGameException, which is caught in startNewGame() to cleanly abort the loading process mid-way.
   During movies: The inline ESC-key checking that was copy-pasted in multiple video playback loops is refactored into a new GameClient::isMovieAbortRequested() method. This method now also checks for window close / Alt+F4 events (not just ESC), so closing the window during a movie no longer gets stuck. The MSG_META_DEMO_INSTANT_QUIT message is also registered as a system message so it propagates correctly even during loading.

[greptile-apps]

Greptile Summary

This PR centralizes all quit logic into a single GameLogic::quit(Bool toDesktop, Bool force) method, replacing previously scattered and inconsistent quit code across QuitMenu.cpp, CommandXlat.cpp, and WinMain.cpp. It also extends graceful exit to two previously unhandled situations: the loading screen (via a new QuitGameException mechanism) and movie playback loops (via the new GameClient::isMovieAbortRequested() helper).

Key changes:

• New GameLogic::quit() centralizes recording stop, script engine unfreeze, multiplayer self-destruct, and game data teardown — with proper guards for the loading and skirmish cases.
• startNewGame() is split into a try/catch wrapper around tryStartNewGame(), catching QuitGameException thrown by updateLoadProgress() when a quit is requested mid-load.
• isMovieAbortRequested() consolidates ESC-key and window-close/Alt+F4 detection into one reusable static method, replacing copy-pasted keyboard polling across multiple video playback loops.
• MSG_META_DEMO_INSTANT_QUIT is promoted out of #if RTS_DEBUG and registered as a system message so it propagates correctly even during loading, and the WM_CLOSE handler now appends a force boolean argument to distinguish Alt+F4 from the X button.
• Previous review concerns (skirmish guard on MSG_SELF_DESTRUCT, TheScriptEngine null guard during loading, tryStartNewGame visibility, m_quitToDesktopAfterMatch getter) have all been addressed.

Remaining minor points:

• Trailing whitespace on a blank line in both Generals/ and GeneralsMD/ GameClient.cpp (inside isMovieAbortRequested()).
• The GetAsyncKeyState-based Alt+F4 detection in WM_CLOSE is reliable in the common case but could misfire in the uncommon scenario where a queued WM_CLOSE is processed while the user coincidentally holds Alt.

Confidence Score: 5/5

Safe to merge — all remaining findings are minor style or edge-case P2 concerns that do not affect correctness in normal usage.

All previously raised P1 concerns (skirmish guard, TheScriptEngine null check during load, tryStartNewGame visibility, m_quitToDesktopAfterMatch encapsulation) have been resolved. The two remaining findings are a trailing whitespace and a theoretical edge case with GetAsyncKeyState polling — neither blocks merge nor affects runtime correctness in normal gameplay scenarios.

No files require special attention; the WinMain.cpp Alt+F4 detection and GameClient.cpp trailing whitespace are the only open points.

Important Files Changed

Filename	Overview
Core/GameEngine/Source/GameClient/GUI/LoadScreen.cpp	Replaced inline ESC-checking loops with GameClient::isMovieAbortRequested() calls; added guard for non-ready frame before frameDecompress()/frameRender() on early abort; added isMovieAbortRequested() check to the loading-progress loop.
Generals/Code/GameEngine/Include/GameLogic/GameLogic.h	Adds quit(), isQuitToDesktopRequested(), tryStartNewGame(), and m_quitToDesktopAfterMatch member — all appropriately scoped (getter is public, member is private, tryStartNewGame is private).
Generals/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp	Adds centralized quit(), QuitGameException mechanism for loading abort, and tryStartNewGame() wrapper; correctly guards script-engine and loading-state checks; properly initializes m_quitToDesktopAfterMatch in constructor.
Generals/Code/GameEngine/Source/GameClient/GameClient.cpp	New isMovieAbortRequested() consolidates ESC + window-close detection cleanly; trailing whitespace on line 777; TheMessageStream dereferenced without null check (consistent with rest of codebase but diverges from the TheKeyboard pattern in the same function).
Generals/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/QuitMenu.cpp	Greatly simplified by delegating to TheGameLogic->quit(); destroyQuitMenu() is now called after quit() (reversed from original), but safe because exitGame() only queues MSG_CLEAR_GAME_DATA — no synchronous window destruction occurs.
Generals/Code/GameEngine/Source/GameClient/MessageStream/CommandXlat.cpp	Moved MSG_META_DEMO_INSTANT_QUIT handler outside the #if RTS_DEBUG guard; now reads force boolean argument and delegates to quit(); registered as a system message so it propagates during loading.
Generals/Code/GameEngine/Source/GameLogic/System/GameLogicDispatch.cpp	Adds m_quitToDesktopAfterMatch check in clearGameData() to trigger setQuitting(TRUE) and resets the flag after use — correct handling of the deferred desktop-quit path.
Generals/Code/Main/WinMain.cpp	WM_CLOSE now appends MSG_META_DEMO_INSTANT_QUIT with a boolean force flag instead of hard-quitting; adds TheGameEngine null guard and ThePlayerList initialization check; Alt+F4 detection via GetAsyncKeyState has a minor reliability concern documented in review comment.
Generals/Code/GameEngine/Include/Common/MessageStream.h	Moves MSG_META_DEMO_INSTANT_QUIT before the debug-only block, making it a regular non-debug message; enum shift is safe per developer confirmation that MSG_META_* messages are never serialized.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["WM_CLOSE / WM_QUERYENDSESSION\n(WinMain.cpp)"] --> B{TheMessageStream\n& ThePlayerList?}
    B -- Yes --> C["Append MSG_META_DEMO_INSTANT_QUIT\n+ appendBooleanArgument(isAltF4)"]
    B -- No --> D["TheGameEngine->setQuitting(TRUE)"]
    C --> E["CommandXlat: MSG_META_DEMO_INSTANT_QUIT\n→ GameLogic::quit(TRUE, force)"]
    F["QuitMenu: exitQuitMenu()"] --> G["GameLogic::quit(FALSE)"]
    H["QuitMenu: quitToDesktopQuitMenu()"] --> I["GameLogic::quit(TRUE)"]
    G --> J{isInGame?}
    I --> J
    E --> J
    J -- No --> K["setQuitting if toDesktop\nsetClientQuiet"]
    J -- Yes --> L{isInInteractiveGame?}
    L -- Yes, force=FALSE\nnot loading --> M["ToggleQuitMenu()\n(show quit dialog)"]
    L -- Yes --> N{Multiplayer &\nnot skirmish &\nnot sandbox?}
    N -- Yes --> O["MSG_SELF_DESTRUCT"]
    N -- No --> P
    O --> P["stopRecording\nsetGamePaused(FALSE)\nunfreezeTime"]
    P --> Q{toDesktop?}
    Q -- Yes, Multiplayer --> R["m_quitToDesktopAfterMatch=TRUE\nupdateLoadProgress → QuitGameException\nclearGameData → setQuitting(TRUE)"]
    Q -- Yes, Single-player --> S["clearGameData()\nsetQuitting(TRUE)"]
    Q -- No --> T["exitGame()\n→ MSG_CLEAR_GAME_DATA (deferred)"]
    U["isMovieAbortRequested()\n(GameClient.cpp)"] --> V{ESC key?}
    V -- Yes --> W["return TRUE\n(abort movie)"]
    V -- No --> X["serviceWindowsOS()\npropagateMessages()"]
    X --> Y{getQuitting or\nm_quitToDesktopAfterMatch?}
    Y -- Yes --> W
    Y -- No --> Z["return FALSE\n(continue)"]

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: 777

Comment:
**Trailing whitespace on blank line**

Line 777 (and the corresponding line 814 in `GeneralsMD/Code/GameEngine/Source/GameClient/GameClient.cpp`) contains trailing whitespace. This is a minor inconsistency with the surrounding code style.

```suggestion
	
```

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

---

This is a comment left during a code review.
Path: Generals/Code/Main/WinMain.cpp
Line: 376-378

Comment:
**`GetAsyncKeyState` polling for Alt+F4 detection may be unreliable in queued-close scenarios**

`GetAsyncKeyState` returns the physical key state at the moment the call executes — not the key state at the moment `WM_CLOSE` was originally generated. In the normal case (user presses Alt+F4), Windows dispatches `WM_CLOSE` synchronously while the keys are still held, so this works correctly.

However, if `WM_CLOSE` arrives from a `PostMessage` / `SendMessage` call (e.g., a queued close from another context) while the user happens to be holding Alt for unrelated reasons, `isAltF4` will be `TRUE` even though the close was not initiated by Alt+F4. This would trigger a forced quit (no quit menu shown) instead of the normal graceful quit-menu path.

The same pattern appears at `GeneralsMD/Code/Main/WinMain.cpp:393-395`.

Consider using the `wParam`/`lParam` passed to `WM_SYSCOMMAND` or checking `SC_CLOSE` before `WM_CLOSE` as a more reliable signal, or at minimum documenting the known limitation with a comment.

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

_{Reviews (16): Last reviewed commit: "Remove TheMessageStream->propagateMessag..." | 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]

Greptile is still complaining about "the SinglePlayerLoadScreen min-spec wait loop in LoadScreen.cpp was not given the same isMovieAbortRequested() + frame-ready guard that ChallengeLoadScreen received, leaving a small hang window during Generals-only min-spec map loads."

[githubawn]

Greptile didn't update the first comment and I don't think I can tag it as it doesn't give me the tag pop-up.

Let me try:
@greptileai can you update the first summary at #2336 (comment) ? or if you can't generate a new one.

[greptile-apps]

Summary of PR #2336: Alt+F4 and Window X Button Support

This PR implements graceful exit handling for Alt+F4 and the window close button, addressing issue #1356. The changes are split across two commits:

Commit 1: Centralized Quit Logic

• New method: GameLogic::quit(Bool toDesktop, Bool force) consolidates all quit handling
• Replaces scattered code in quit menu callbacks, Alt+F4 handler, and "quit to desktop" button
• Handles edge cases:
  • Stops active recordings via TheRecorder->stopRecording()
  • Sends self-destruct message in multiplayer (using new m_quitToDesktopAfterMatch flag)
  • Unpauses/unfreezes game before exit via setGamePaused(FALSE) and TheScriptEngine->forceUnfreezeTime()
  • Distinguishes Alt+F4 (force=TRUE) from window X button (force=FALSE, shows quit menu)

Commit 2: Graceful Exit During Loading & Movies

• Loading screen: LoadScreen::update() now checks quit status and throws QuitGameException, caught in startNewGame() to cleanly abort mid-load
• Movies: Refactored duplicate ESC-key checking into GameClient::isMovieAbortRequested()
  • Checks ESC key, window close, and Alt+F4 events
  • Applied to video loops in SinglePlayerLoadScreen and ChallengeLoadScreen
  • Window close during movies no longer hangs the game
• Message system: MSG_META_DEMO_INSTANT_QUIT registered as system message for proper propagation during loading

Technical Details

• Files changed: 19 files across Generals and GeneralsMD codebases
• Changes: +373 additions, -139 deletions
• Key files: GameLogic.cpp, GameClient.cpp, LoadScreen.cpp, WinMain.cpp, QuitMenu.cpp, CommandXlat.cpp
• Window message handling: WinMain.cpp now detects Alt+F4 vs window X and passes appropriate force flag

The implementation ensures clean shutdown in all game states (main menu, loading, in-game, during movies) while properly handling multiplayer, recording, and script engine cleanup.

[githubawn]

@greptileai Include "Key findings:" and "Confidence Score:".

[greptile-apps]

Key findings:

1. Excellent refactoring - The new GameLogic::quit(Bool toDesktop, Bool force) method successfully centralizes previously scattered quit logic from QuitMenu, Alt+F4 handler, and quit buttons. This eliminates code duplication and creates a single source of truth.

2. Smart force parameter design - The distinction between window X-button (force=FALSE, shows quit menu) vs Alt+F4 (force=TRUE, immediate quit) is well-designed and matches user expectations.

3. Clean loading abort - The QuitGameException approach elegantly handles mid-loading exits without complex state checking. The try-catch in startNewGame() ensures clean cleanup.

4. Robust movie abort - Refactoring inline ESC checks into GameClient::isMovieAbortRequested() is cleaner and now properly handles window close/Alt+F4 during videos (previously would hang).

5. Good defensive programming - Proper null checks on TheInGameUI, TheRecorder, TheScriptEngine, ThePlayerList prevent early-startup/late-shutdown crashes.

6. System message registration - Moving MSG_META_DEMO_INSTANT_QUIT out of #ifdef RTS_DEBUG and registering it as a system message ensures it propagates correctly during loading.

Minor observations:

• The m_quitToDesktopAfterMatch flag correctly defers desktop quit in multiplayer until match end
• Unpause/unfreeze logic before exit prevents game from getting stuck in paused state
• Both Generals and GeneralsMD codebases updated consistently

Confidence Score: 90/100

This is high-quality work. The architecture is sound, edge cases are handled, and the refactoring meaningfully improves maintainability. The 10% deduction is purely because extensive quit scenarios (loading screens, movies, multiplayer, recording, etc.) require thorough testing to ensure no regressions.

[xezon]

What the comment describes is unintuitive. Where/how have you seen this requirement?

[githubawn]

#2336 (comment)

this was added after this comment

[xezon]

This one is also testing ThePlayerList. I still do not understand it. It looks like a "code smell".

[githubawn]

This is for the exact same reason as above. It prevents the engine from crashing during an early exit since ThePlayerList isn't ready.

[githubawn]

Loading a savegame in a match causes a quit, back to draft

[githubawn]

@greptileai when i load another savegame from an already loaded match the game now quits the game

Should i remove "TheMessageStream->propagateMessages();" from LoadScreen.cpp and or Gameclient.cpp?

[greptile-apps]

Yes, remove TheMessageStream->propagateMessages(); from LoadScreen.cpp only.

Keep it in GameClient.cpp - that one is specifically for detecting quit requests during movie playback (the intended feature).

The issue is that during a savegame load, LoadScreen::update() calls propagateMessages(), which processes any pending messages or checks quit flags. If there's leftover state from the previous game (like a quit message or the m_quitToDesktopAfterMatch flag), it triggers an unwanted quit.

Removing it from LoadScreen.cpp prevents processing messages during normal loading while still allowing the quit detection to work during movies via isMovieAbortRequested().