Stop indexing an empty page array, and redo a dropped layout pass#65
Merged
Conversation
…mpty page array Three defects in the panel resizing, all found by review rather than by use. BuildLayout ran from CWizTab::OnInitDialog, but MFC restyles a modeless sheet from a popup to WS_CHILD only after WM_INITDIALOG returns. Every rect was therefore measured against the popup's client origin, baking a caption and border into the baseline, which then offset the buttons and the page for the rest of the run. Record it from CDialogContainer::OnInitialUpdate instead, the first point where both sheets are their final selves and still untouched. GetActivePage() is m_pages[GetActiveIndex()] with no bounds check outside a debug ASSERT, and GetActiveIndex() gives -1 when nothing is selected. Both FinalInProgress and EndInProgress empty the array before refilling it, and RemovePage relayouts synchronously, so a WM_SIZE arriving mid-rebuild read m_pages[-1] and dereferenced whatever it found. Go through GetActiveIndex and GetPageCount instead. DeferWindowPos destroys the HDWP when it fails, dropping every position already deferred. The loop carried on with SetWindowPos for the remainder, so the already-queued controls silently kept their old rects. Redo the whole pass instead of continuing a half-applied one. No functional change intended beyond the above; the layout still needs an eye on a VM, since CI cannot see it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019HXNEMWtyrkW7xoD8ewcR3 Signed-off-by: Xavier Roche <roche@httrack.com>
The popup-to-WS_CHILD restyle the first message claimed does not happen: MFC injects the requested style into the dialog template at PSCB_PRECREATE, so the sheet is already a child when WM_INITDIALOG arrives, and ScreenToClient would have cancelled a caption out in any case. What moving the call does buy is that the active page has a window by then, so the page's own rect is measured rather than the hidden tab control standing in for it. State that instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019HXNEMWtyrkW7xoD8ewcR3 Signed-off-by: Xavier Roche <roche@httrack.com>
xroche
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two real defects from #61, plus a call-site move.
GetActivePage()ism_pages[GetActiveIndex()]with no bounds check outside a debug ASSERT, andGetActiveIndex()returns -1 when nothing is selected.FinalInProgressandEndInProgressboth empty the array before refilling it, andRemovePagerelayouts synchronously, so aWM_SIZEarriving mid-rebuild readm_pages[-1]and dereferenced whatever came back. Both call sites now go throughGetActiveIndexandGetPageCount.DeferWindowPosdestroys the HDWP on failure, dropping every position already queued, and the loop then carried on withSetWindowPosfor the remainder so the earlier controls silently kept their old rects. It now redoes the whole pass.BuildLayoutalso moves fromCWizTab::OnInitDialogtoCDialogContainer::OnInitialUpdate. My first rationale for this was wrong: there is no popup-to-WS_CHILD restyle after WM_INITDIALOG, MFC injects the style into the template at PSCB_PRECREATE, and the recorded rects are identical either way. What the move actually buys is that the active page has a window at the later point, som_pageBaseis the page rect rather than the hidden tab control standing in for it.Nothing here is visible in normal use, and VM testing confirmed no change to the startup window.