Skip to content

feat(headless): stub gogoMessageBox in GameWindowManagerDummy#2894

Closed
bobtista wants to merge 2 commits into
TheSuperHackers:mainfrom
bobtista:bobtista/feat/headless-messagebox-dummy
Closed

feat(headless): stub gogoMessageBox in GameWindowManagerDummy#2894
bobtista wants to merge 2 commits into
TheSuperHackers:mainfrom
bobtista:bobtista/feat/headless-messagebox-dummy

Conversation

@bobtista

Copy link
Copy Markdown

GameWindowManagerDummy inherits the base gogoMessageBox implementations, so paths that raise a message box in headless mode, eg GameState::saveGame reporting a save error via MessageBoxOk still try to build a message box layout without a display.

Fix:
GameWindowManagerDummy returns no window for both gogoMessageBox overloads, consistent with its other no-op window functions. The message box call sites reachable in headless mode ignore the returned window - the callers that keep the pointer are all interactive menu code.

Todo:

  • Verify the save error path in headless mode returns cleanly instead of crashing
  • Replicate to Generals

@bobtista bobtista self-assigned this Jul 19, 2026
@greptile-apps

greptile-apps Bot commented Jul 19, 2026

Copy link
Copy Markdown

Greptile Summary

Stubs out both gogoMessageBox overloads in GameWindowManagerDummy to return nullptr immediately, preventing the base-class implementation from attempting to build a GUI window layout when no display is present in headless mode. The fix is applied identically to both the Generals and Zero Hour builds.

  • Both overloads (with and without useLogo) are overridden, covering all call sites that reach gogoMessageBox in headless mode.
  • The stubs are consistent with the existing no-op pattern already used for every other window-factory and draw-func method in GameWindowManagerDummy.
  • Note: registered callbacks (okCallback, cancelCallback, etc.) will not be invoked when the stub returns nullptr. The PR description asserts the headless-reachable call sites ignore the returned window and don't rely on callbacks, but the open TODO item ("Verify the save error path in headless mode returns cleanly") should be closed before this is considered fully validated.

Confidence Score: 5/5

Safe to merge — the change is a minimal, targeted stub that prevents a crash in headless mode and is fully consistent with the surrounding no-op pattern.

Both overloads are correctly overridden with signatures that match the base class exactly, nullptr is used appropriately, and the inline one-liner style matches every other stub already present in GameWindowManagerDummy. The one open thread — whether the callbacks need to fire on the save-error path — is explicitly called out in the PR's own TODO, so the author is already tracking it.

No files require special attention. The only follow-up is the open TODO to validate the save-error code path in headless mode, which lives outside these header changes.

Important Files Changed

Filename Overview
Generals/Code/GameEngine/Include/GameClient/GameWindowManager.h Adds two gogoMessageBox no-op overrides to GameWindowManagerDummy returning nullptr, preventing headless-mode crashes when the base class tries to build a window layout without a display.
GeneralsMD/Code/GameEngine/Include/GameClient/GameWindowManager.h Identical change to the Zero Hour counterpart — mirrors the same two gogoMessageBox stubs in the Zero Hour GameWindowManagerDummy, keeping the two builds in sync.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant GS as GameState::saveGame
    participant WM as TheWindowManager
    participant B as GameWindowManager (base)
    participant D as GameWindowManagerDummy

    Note over WM: Headless mode uses Dummy

    GS->>WM: gogoMessageBox(...)

    alt Before this PR (base class)
        WM->>B: gogoMessageBox(...)
        B-->>B: Build window layout (no display!)
        B--xGS: crash / undefined behavior
    end

    alt After this PR (dummy override)
        WM->>D: gogoMessageBox(...)
        D-->>GS: return nullptr
        GS->>GS: ignore nullptr (no-op)
    end
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant GS as GameState::saveGame
    participant WM as TheWindowManager
    participant B as GameWindowManager (base)
    participant D as GameWindowManagerDummy

    Note over WM: Headless mode uses Dummy

    GS->>WM: gogoMessageBox(...)

    alt Before this PR (base class)
        WM->>B: gogoMessageBox(...)
        B-->>B: Build window layout (no display!)
        B--xGS: crash / undefined behavior
    end

    alt After this PR (dummy override)
        WM->>D: gogoMessageBox(...)
        D-->>GS: return nullptr
        GS->>GS: ignore nullptr (no-op)
    end
Loading

Reviews (1): Last reviewed commit: "feat(headless): replicate gogoMessageBox..." | Re-trigger Greptile

@bobtista bobtista added the Enhancement Is new feature or request label Jul 19, 2026
@Skyaero42

Copy link
Copy Markdown

I'm not a fan of this implementation. The use of a dummy class in production is a code smell imho. This reads like a hack rather than a proper solution.

It looks like there is an issue with separation of concerns here. A save call should not be invoking any UI elements. I think the proper solution would be a refactor that ensures proper SoC.

@bobtista

bobtista commented Jul 20, 2026

Copy link
Copy Markdown
Author

I'm not a fan of this implementation. The use of a dummy class in production is a code smell imho. This reads like a hack rather than a proper solution.

It looks like there is an issue with separation of concerns here. A save call should not be invoking any UI elements. I think the proper solution would be a refactor that ensures proper SoC.

I'm with you that a better fix is decoupling save from UI. That might take a few more PRs and more time though. As for the dummy class, it's how we've been doing it. Headless mode in this codebase is built on Dummy device classes eg GameWindowManagerDummy. I'm also with you that there should be clear issues and these PRs should address them.

edit: I'll work on the better solution and will close these PRs

@bobtista bobtista closed this Jul 21, 2026
@bobtista

Copy link
Copy Markdown
Author

#3000
#3001
:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Enhancement Is new feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants