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 scattered quit logic into a single GameLogic::quit(Bool toDesktop, Bool force) method and extends graceful exit to the loading screen and movie playback, enabling Alt+F4 and the window X button to work cleanly in all game states. The overall architecture is sound — the QuitGameException pattern for aborting loading mid-way, the m_quitToDesktopAfterMatch deferred-quit flag for multiplayer, and the isMovieAbortRequested() consolidation are all clean solutions to a previously fragmented problem.

Key findings:

• Bug (loading screen): The #if RTS_GENERALS min-spec fallback path in SinglePlayerLoadScreen::init() (the while(!isFrameReady()) Sleep(1) loop at lines 596–602 of LoadScreen.cpp) was not updated with an abort check or post-loop frame-ready guard, unlike the equivalent path in ChallengeLoadScreen which was correctly fixed. During a min-spec map load, the window cannot be closed until the last video frame finishes decoding.
• Style: startNewGame(Bool saveGame) in Generals/Code/GameEngine/Include/GameLogic/GameLogic.h — the rename from loadSaveGame to saveGame is ambiguous and reads as "save the game." The implementation file still uses loadingSaveGame; the declaration should match.
• Style: Trailing whitespace on a blank line inside isMovieAbortRequested() in both Generals/ and GeneralsMD/ GameClient.cpp implementations.
• The previously-flagged concerns (!isInSkirmishGame() guard, tryStartNewGame visibility, m_quitToDesktopAfterMatch encapsulation) are all correctly addressed in the final implementation.

Confidence Score: 3/5

• Safe to merge for Zero Hour; Generals min-spec loading abort is uninterruptible and should be fixed first.
• The core centralization is well-implemented and the main concerns from earlier review threads are resolved. One genuine regression exists: 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. Everything else — the QuitGameException mechanism, multiplayer self-destruct guard, force-flag semantics, and WM_CLOSE rework — is correct.
• Core/GameEngine/Source/GameClient/GUI/LoadScreen.cpp lines 596–602 (SinglePlayerLoadScreen min-spec path)

Important Files Changed

Filename	Overview
Core/GameEngine/Source/GameClient/GUI/LoadScreen.cpp	Refactored video skip to use isMovieAbortRequested() in both SinglePlayerLoadScreen and ChallengeLoadScreen loops; ChallengeLoadScreen correctly received the abort guard + post-loop frame-ready check, but the #if RTS_GENERALS min-spec fallback path in SinglePlayerLoadScreen (lines 596–602) was missed — the wait loop cannot be interrupted and frameDecompress() has no frame-ready guard.
Generals/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp	Added centralized quit(Bool toDesktop, Bool force) method and QuitGameException-based loading abort; tryStartNewGame wrapper and m_quitToDesktopAfterMatch flag handle clean exit paths correctly; !isInSkirmishGame() guard is correctly preserved in the self-destruct check.
GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp	Mirror of Generals GameLogic.cpp changes for Zero Hour; same quit() implementation with correct skirmish/sandbox guard and loading-state checks for script engine calls.
Generals/Code/Main/WinMain.cpp	Replaced hard-quit WM_CLOSE handler with graceful message-based exit; correctly detects Alt+F4 vs. X-button and passes force flag; falls back to setQuitting(TRUE) during early startup/late shutdown; null checks for TheGameEngine and TheMessageStream/ThePlayerList are properly guarded.
GeneralsMD/Code/Main/WinMain.cpp	Zero Hour WM_CLOSE and WM_QUERYENDSESSION both updated to use message-based graceful quit with null-checks and force-flag propagation; consistent with Generals version.
Generals/Code/GameEngine/Source/GameClient/GameClient.cpp	New static isMovieAbortRequested() correctly consolidates ESC check, serviceWindowsOS(), and quit-flag poll; one trailing whitespace on blank line between propagateMessages() call and the if block.
Generals/Code/GameEngine/Source/GameClient/MessageStream/CommandXlat.cpp	Removed legacy #ifdef ALLOW_ALT_F4 guard (and the old #define dont_ALLOW_ALT_F4), wired MSG_META_DEMO_INSTANT_QUIT to quit(TRUE, force), and registered it as a system message so it propagates during loading.
Generals/Code/GameEngine/Include/GameLogic/GameLogic.h	Properly added quit() declaration, private tryStartNewGame / m_quitToDesktopAfterMatch, and isQuitToDesktopRequested() getter; startNewGame parameter renamed from loadSaveGame to saveGame which is misleadingly ambiguous.
Generals/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/QuitMenu.cpp	Duplicate quit logic cleanly replaced with single TheGameLogic->quit() call in both exitQuitMenu and quitToDesktopQuitMenu; the quit-menu-visible guard in quit() correctly prevents double-toggling the menu.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["WM_CLOSE / Alt+F4"] -->|"WndProc"| B{"TheMessageStream\n& ThePlayerList\ninitialized?"}
    B -->|"Yes"| C["Append MSG_META_DEMO_INSTANT_QUIT\n(force = isAltF4)"]
    B -->|"No (early startup / late shutdown)"| D["TheGameEngine->setQuitting(TRUE)"]

    C --> E["CommandTranslator::translateGameMessage"]
    E --> F["TheGameLogic->quit(toDesktop=TRUE, force)"]

    F --> G{"isInGame()"}
    G -->|"No"| M
    G -->|"Yes"| H{"isInInteractiveGame()\n& force == FALSE\n& not loading?"}
    H -->|"Yes — quit menu not visible"| I["ToggleQuitMenu() → return\n(user confirms via UI)"]
    I -->|"User clicks Quit to Desktop"| F2["quit(TRUE, force=FALSE)\n(menu visible → skip toggle)"]
    F2 --> J

    H -->|"force==TRUE or loading"| J{"toDesktop?"}
    J -->|"TRUE, multiplayer"| K["m_quitToDesktopAfterMatch = TRUE\nexitGame() if not loading"]
    J -->|"TRUE, single-player"| L["clearGameData() if not loading\nsetQuitting(TRUE)"]
    J -->|"FALSE"| N["exitGame()"]

    K --> O["updateLoadProgress throws\nQuitGameException"]
    O --> P["startNewGame catch block\naborts load cleanly"]

    L --> M["setClientQuiet(TRUE)"]
    N --> M
    P --> M

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: Core/GameEngine/Source/GameClient/GUI/LoadScreen.cpp
Line: 599-602

Comment:
**Min-spec wait loop not interruptible — missing abort check and frame-ready guard**

`ChallengeLoadScreen::init()` received both an `isMovieAbortRequested()` check inside its equivalent `while(!isFrameReady())` loop **and** a post-loop early-return guard (see `LoadScreen.cpp:1101–1112`). The `SinglePlayerLoadScreen` min-spec path in the `#if RTS_GENERALS` block has neither: the loop spins until the last frame finishes decoding with no way to interrupt it, and `frameDecompress()`/`frameRender()` are always called unconditionally afterward.

If the user closes the window (or presses Alt+F4) while this loop is running, the shutdown is silently deferred until the frame decode completes. More importantly, if the loop were ever made interruptible, skipping the ready-guard would pass a non-ready frame to `frameDecompress()` — the same class of bug the `ChallengeLoadScreen` fix was specifically addressing.

Suggested fix to match the pattern already applied to `ChallengeLoadScreen`:

```suggestion
		while(!m_videoStream->isFrameReady())
		{
			if (GameClient::isMovieAbortRequested())
			{
				break;
			}
			Sleep(1);
		}
		if (!m_videoStream->isFrameReady())
		{
			return;
		}
		m_videoStream->frameDecompress();
		m_videoStream->frameRender(m_videoBuffer);
```

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/GameEngine/Include/GameLogic/GameLogic.h
Line: 155

Comment:
**Misleading parameter rename — `saveGame` reads as "save the game"**

The declaration was changed from `Bool loadSaveGame` to `Bool saveGame`. `saveGame` is ambiguous: a reader will naturally parse it as "should I save the game?" rather than "am I loading from a save?". The implementation in `GameLogic.cpp` still uses `loadingSaveGame`, which is clear. Keeping the declaration consistent with the implementation avoids the confusion:

```suggestion
	void startNewGame( Bool loadSaveGame );
```

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/GameEngine/Source/GameClient/GameClient.cpp
Line: 799

Comment:
**Trailing whitespace on blank line**

There is a stray tab/space on the blank line between `propagateMessages()` and the `if` check. The same trailing whitespace exists in the mirror implementation at `GeneralsMD/Code/GameEngine/Source/GameClient/GameClient.cpp` at the equivalent line.

```suggestion
	
```

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

_{Last reviewed commit: "fixes frameDecompres..."}

[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.

[Caball009]

I noticed an issue that should probably be addressed. If you launch the game and immediately close it (from the taskbar), the game crashes because ThePlayerList is not yet initialized. This forces the game into throwing an exception, catching it, and going through the crash dump generation process.

Call stack:

generalszh.exe!PlayerList::getLocalPlayer()
generalszh.exe!GameMessage::GameMessage(GameMessage::Type type)
generalszh.exe!MessageStream::appendMessage(GameMessage::Type type)
generalszh.exe!WndProc(HWND__ * hWnd, unsigned int message, unsigned int wParam, long lParam)

Maybe the way to fix this is to add a condition to the WM_CLOSE case, where it checks if the engine initialization is done before appending any message. If it's not, just call TheGameEngine->setQuitting(TRUE). I'm not sure if there's already a good way to tell if the engine initialization is finished, but I have an idea for that.

The WM_CLOSE case needs to be cleaned up, anyway.

[githubawn]

I've attempted to replicate the crash by repeatedly launching the game and closing it from the taskbar, using a script that increments the delay between launch and close by 10ms each iteration, I wasn't able to trigger it. Included the if (TheGameEngine->isActive() && !TheInGameUI->isQuitMenuVisible())

[Caball009]

I've attempted to replicate the crash by repeatedly launching the game and closing it from the taskbar, using a script that increments the delay between launch and close by 10ms each iteration, I wasn't able to trigger it. Included the if (TheGameEngine->isActive() && !TheInGameUI->isQuitMenuVisible())

Did you try with windowed mode? I'm not sure whether it works with full screen mode. I also used -quickstart and no EA logo movies, if it makes any difference.