Repository navigation
bugfix(gui): Free push button data the headless dummy window cannot keep - #3480
tintinhamans wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe window manager now reports whether it is a dummy. Push-button data creation returns null in dummy mode. Five setters use a shared helper and return without updating fields when data is unavailable. ChangesDummy-mode push-button data handling
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly identifies a GUI bug fix for leaked push-button data in headless dummy windows. |
| Description check | |
The description accurately explains the headless-mode leak, identifies the affected behavior, and summarizes the implemented fix. |
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
 [Low impact] The PR appears safe to merge.
|
tintinhamans
added
Bug
a112357 to
90622b0
Compare
| void GadgetButtonDrawOverlayImage( GameWindow *g, const Image *image ) | ||
| { | ||
| if( g == nullptr ) | ||
| if( !isPushButtonWindow( g ) ) |
There was a problem hiding this comment.
This breaks the veterancy overlay on the unit portrait. CameoWindow in ControlBar.wnd is a USER window drawn with W3DGadgetPushButtonImageDraw, and ControlBar.cpp:2665 sets its overlay through GadgetButtonDrawOverlayImage, which now returns early. Could we fix the headless dummy window instead?
There was a problem hiding this comment.
right, the cameo gets one too and ControlBar frees it. moved the fix to the dummy window path instead, setters now drop the data when the window can’t keep it
tintinhamans
changed the title
90622b0 to
fdd8b0a
Compare
| { | ||
| pData = getNewPushButtonData(); | ||
| g->winSetUserData( pData ); | ||
| // The headless dummy window does not keep user data |
There was a problem hiding this comment.
I do not like that the user code needs to accomodate the special headless dummy implementation. Dummy should offer the same contract as the non-dummy implementation. Can we implement this differently?
There was a problem hiding this comment.
restoring the normal window lifecycle fixes it, but requires headless to load WND files and run the UI callbacks. did you have a smaller dummy-side fix in mind that avoids doing that?
There was a problem hiding this comment.
First idea of improvement is to return null in getNewPushButtonData if TheWindowManager->isDummy() returns true.
There was a problem hiding this comment.
added isDummy() so it skips the allocation, and dropped the read-back check
fdd8b0a to
55bd6b8
Compare
4 participants
The push-button setters allocate
PushButtonDatawhen a window has none. The headless dummy always returns null for user data, so repeated calls keep allocating data that cannot be retrieved or cleaned up. The debug leak report of a headless replay grows by six blocks per match from this alone.getNewPushButtonDatanow checksTheWindowManager->isDummy()and returns null before allocating. The setters share a helper that handles that result, without the old store/read-back/delete check.Normal buttons and the USER cameo window still get their data and cleanup as before. Headless keeps its existing dummy windows, without loading WND files or enabling UI callbacks.