Fixes #5433. Make View.Draw self-draw context flow explicit#5440
Merged
harder merged 2 commits intoMay 29, 2026
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors View.Draw so the self-draw context lifecycle is centralized and easier to reason about, preserving the redraw behavior introduced by #5431 while reducing future regression risk.
Changes:
- Moves
_localDrawContextcreation/use/merge into a newDrawSelfContenthelper. - Keeps the self-clear phase separate to preserve draw order: clear → SubViews → self text/content.
- Adds documentation for the
_localDrawContextlifecycle and a planning note for #5433.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
Terminal.Gui/ViewBase/View.Drawing.cs |
Centralizes self-content drawing and _localDrawContext lifecycle while preserving existing draw order and child-only redraw behavior. |
plans/5433-explicit-self-draw-context.md |
Documents the issue, intended refactor, verification expectations, and acceptance criteria mapping. |
harder
marked this pull request as ready for review
May 29, 2026 17:36
Extract the self-content draw (Text + Content + local-context merge) into a private DrawSelfContent helper that owns the full _localDrawContext lifecycle for a self-redraw pass: create, populate, merge. Move the context creation out of the viewport-clear block — DoClearViewport only writes the shared context, so the local context isn't needed until DrawSelfContent, keeping create/populate /merge in one place. This replaces three separate `if (needsDrawSelf)` blocks (whose alignment had to be maintained by hand) with a clear/self split documented as intentional around DoDrawSubViews, plus an authoritative lifecycle comment on the _localDrawContext field describing why it is preserved across child-only passes (TransparentMouse CachedDrawnRegion contract from tui-cs#5431). Behavior-preserving; draw order unchanged. Side effect: collapsing creation and use into one method lets nullable flow analysis prove _localDrawContext non-null, removing a CS8602 warning. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
harder
force-pushed
the
fix-5433-explicit-self-draw-context
branch
from
May 29, 2026 17:38
98c2bfa to
42fa6c1
Compare
tig
approved these changes
May 29, 2026
tig
left a comment
Member
There was a problem hiding this comment.
Nice that collapsing it kills the CS8602 instead of forgiving it. The field remarks documenting “preserve on child-only passes” is the bit that stops someone re-breaking TransparentMouse later.
This is hero work and I love your methodical approach!
This was referenced Jun 2, 2026
This was referenced Jun 15, 2026
This was referenced Jun 22, 2026
This was referenced Jun 29, 2026
This was referenced Jul 1, 2026
This was referenced Jul 13, 2026
62 tasks
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
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.
Summary
Makes the
View.Draw (...)self-draw flow explicit so the_localDrawContextlifecycle is owned in one place instead of being spread across three hand-alignedif (needsDrawSelf)blocks. Pure refactor — no rendering, ordering, or invalidation behavior changes.Split from #4973; follow-up to #5431 (#5358), which introduced the
needsDrawSelf/_localDrawContextmechanism and explicitly deferred this cleanup.Background
PR #5431 fixed the child-only redraw fan-out from #5358 by capturing
needsDrawSelf = NeedsDrawbeforeDoDrawAdornmentsescalatesNeedsDrawRect, then gating the self-content draw on it. That fix is correct, but its implementation spread the per-view_localDrawContextlifecycle across three separateif (needsDrawSelf)blocks inView.Draw (...):_localDrawContext,SetAttributeForRole+DoClearViewport,DoDrawSubViews)DoDrawText+DoDrawContentinto_localDrawContext, then merge into the shared context._localDrawContextis consumed later inDoDrawCompleteforTransparentMouseCachedDrawnRegionhit-testing. The subtle invariants — recreate only on self-redraw, preserve across child-only passes, consume inDoDrawComplete— were split across distant blocks, so a future edit could easily reset or split the lifecycle at the wrong point and reintroduce the #5431 review regression (TransparentMouseParent_ChildOnlyDirty_PreservesCachedDrawnRegion).Changes
View.Drawing.cs— new privateDrawSelfContent (DrawContext sharedContext)that owns the entire_localDrawContextlifecycle for a self-redraw pass: (re)create it, draw Text and Content into it, then merge its region into the shared context.Draw (...)now calls this once, gated onneedsDrawSelf, after SubViews.View.Drawing.cs—Draw (...)no longer creates_localDrawContextalongside the viewport clear.DoClearViewportwrites only the shared context, so the local context isn't needed untilDrawSelfContent; moving creation there keeps create → populate → merge in one place. The remaining "clear self viewport" block carries a comment explaining that self-draw is intentionally split aroundDoDrawSubViewsto preserve order.View.Drawing.cs—_localDrawContextfield gains an authoritative<remarks>documenting the lifecycle: recreated only on self-redraw passes (inDrawSelfContent), left untouched on child-only passes soDoDrawCompletekeeps a validCachedDrawnRegion.Draw order is unchanged: Clear (self) → SubViews → flush
_lastClearedViewport→ Text/Content (self).Side effect: removes a warning
Collapsing
_localDrawContext's creation and use into one method lets nullable flow analysis prove it non-null, removing the pre-existingCS8602(Dereference of a possibly null reference) warning at the oldcontext.AddDrawnRegion (_localDrawContext.GetDrawnRegion ())site.Testing
dotnet build --no-restore— 0 warnings, 0 errors (and removes one pre-existing warning, per above).dotnet test --project Tests/UnitTestsParallelizable --no-build— 17260 pass / 17 pre-existing skips.SubViewOnlyRedrawTests(incl.TransparentMouseParent_ChildOnlyDirty_PreservesCachedDrawnRegion),RegionAwareClearViewportTests,NeedsDrawTests.No new tests: this is a behavior-preserving refactor whose contract is already pinned by the #5431 tests above. Coverage is maintained.
Acceptance criteria mapping
DrawSelfContentowns the_localDrawContextcreate/populate/merge; the clear half is a single documented block whose split from content is explained._localDrawContextonly on self-redrawDrawSelfContent, called only whenneedsDrawSelf.DrawSelfContentnot called whenneedsDrawSelfis false; field left untouched (documented on the field).To pull down this PR locally