feat: Implement ALT + F4 and Window X button to close game#2336
feat: Implement ALT + F4 and Window X button to close game#2336githubawn wants to merge 23 commits intoTheSuperHackers:mainfrom
Conversation
|
| Filename | Overview |
|---|---|
| Generals/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp | Adds canOpenQuitMenu(), quit(), and the QuitGameException/tryStartNewGame loading-abort mechanism; core logic is sound with correct loading guards and !isInSkirmishGame() restored. |
| GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp | Mirror of Generals GameLogic.cpp changes; same analysis applies. |
| Generals/Code/GameEngine/Source/GameClient/GameClient.cpp | New isMovieAbortRequested() centralises ESC/WM_CLOSE/Alt+F4 detection for all movie loops; correctly calls serviceWindowsOS() + propagateMessages() then checks both quit flags. |
| Core/GameEngine/Source/GameClient/GUI/LoadScreen.cpp | Movie loops refactored to use isMovieAbortRequested(); min-spec double-frame-ready guard added for ChallengeLoadScreen; LoadScreen::update() now also checks isQuitToDesktopRequested(). |
| Generals/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/QuitMenu.cpp | exitQuitMenu/quitToDesktopQuitMenu now delegate entirely to quit(); ToggleQuitMenu guard updated to canOpenQuitMenu() with correct isVisible short-circuit. |
| Generals/Code/Main/WinMain.cpp | WM_CLOSE handler replaced: old hard-exit (_exit) is gone; new path posts MSG_META_DEMO_INSTANT_QUIT or falls back to setQuitting(TRUE), guarded by !getQuitting(). |
| GeneralsMD/Code/Main/WinMain.cpp | WM_QUERYENDSESSION else-branch now guarded by TheGameEngine null check; WM_CLOSE uses canAddMessage() consistently with Generals version. |
| Generals/Code/GameEngine/Source/Common/MessageStream.cpp | New canAddMessage() delegates to ThePlayerList != nullptr, replacing the raw ThePlayerList pointer check at each WndProc call site. |
| Generals/Code/GameEngine/Source/GameClient/MessageStream/CommandXlat.cpp | MSG_META_DEMO_INSTANT_QUIT removed from debug guard, now always active; delegates to quit(TRUE, force) and registered as a system message for correct propagation during loading. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[WM_CLOSE / Alt+F4] --> B{TheGameEngine non-null & !getQuitting?}
B -- No --> Z[Ignore / already quitting]
B -- Yes --> C{canAddMessage? PlayerList != null}
C -- No --> D[setQuitting TRUE directly]
C -- Yes --> E[Append MSG_META_DEMO_INSTANT_QUIT force = isAltF4]
E --> F[CommandTranslator translateGameMessage]
F --> G[GameLogic::quit toDesktop=TRUE, force]
G --> H{isInGame?}
H -- No --> I[setQuitting TRUE setClientQuiet TRUE]
H -- Yes --> J{force==FALSE && canOpenQuitMenu?}
J -- Yes --> K[ToggleQuitMenu show confirmation]
J -- No --> L{isInMultiplayer && !isSkirmish && !sandbox?}
L -- Yes --> M[Append MSG_SELF_DESTRUCT]
L -- No --> N[stopRecording if active]
M --> N
N --> O[setGamePaused FALSE script unfreeze if !loading]
O --> P{toDesktop?}
P -- Yes, Multiplayer --> Q[m_quitToDesktopAfterMatch=TRUE exitGame if !loading]
P -- Yes, SinglePlayer --> R[clearGameData if !loading]
P -- No --> S[exitGame]
Q --> I
R --> I
S --> T[setClientQuiet TRUE]
D --> U{LoadScreen::update updateLoadProgress}
I --> U
U --> V{getQuitting OR m_quitToDesktopAfterMatch?}
V -- Yes --> W[throw QuitGameException caught in startNewGame cleanly aborts load]
V -- No --> X[continue loading]
Reviews (17): Last reviewed commit: "implemented review feedback" | Re-trigger Greptile
GeneralsMD/Code/GameEngine/Source/GameClient/GUI/LoadScreen.cpp
Outdated
Show resolved
Hide resolved
|
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. |
|
Will rebase soon. The multiplayer behavior was designed based on your comment from #1356 (comment). 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. |
|
Thank you for implementing this. I did a few quick tests and the current implementation appears to work well. |
843b370 to
4fda496
Compare
GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp
Outdated
Show resolved
Hide resolved
2d896d5 to
036ed84
Compare
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
xezon
left a comment
There was a problem hiding this comment.
It would have been easier to review if this change just contained Core+GeneralsMD edits for starters.
GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp
Outdated
Show resolved
Hide resolved
3a3c926 to
6f2b0b8
Compare
a2166e4 to
c6ee106
Compare
Additional Comments (1)
The original code called The identical implementation in Prompt To Fix With AIThis 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. |
GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngine/Source/GameClient/MessageStream/CommandXlat.cpp
Show resolved
Hide resolved
added comments moved QuitGameException
|
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." |
|
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: |
Summary of PR #2336: Alt+F4 and Window X Button SupportThis 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
Commit 2: Graceful Exit During Loading & Movies
Technical Details
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. |
|
@greptileai Include "Key findings:" and "Confidence Score:". |
|
Key findings:
Minor observations:
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. |
GeneralsMD/Code/GameEngine/Source/GameClient/MessageStream/CommandXlat.cpp
Outdated
Show resolved
Hide resolved
fixes accidentally revert
|
Loading a savegame in a match causes a quit, back to draft |
|
@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? |
|
Yes, remove 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, Removing it from LoadScreen.cpp prevents processing messages during normal loading while still allowing the quit detection to work during movies via |
Fix leftover quit state
xezon
left a comment
There was a problem hiding this comment.
It is astonishing how complex GameLogic::quit is.
Fixes #1356
Split up in 2 commits.
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).
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.