Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 22, 2025

Summary

Fixed critical GDI bitmap management bugs in VwDrawRootBuffered causing resource leaks and visual artifacts. The code was deleting stock bitmaps (which should never be deleted) and leaking custom bitmaps (which should always be deleted).

Root cause: After SelectObjectBitmap(dc, newBitmap), the code deleted the returned stock bitmap instead of the created custom bitmap:

// BEFORE (buggy):
HBITMAP hbmpOld = AfGdi::SelectObjectBitmap(m_hdcMem, hbmp);
AfGdi::DeleteObjectBitmap(hbmpOld);  // Wrong: deletes stock bitmap
// hbmp never deleted → leak

// AFTER (fixed):
HBITMAP hbmpOld = AfGdi::SelectObjectBitmap(m_hdcMem, hbmp);
// Don't delete hbmpOld (stock bitmap)
// hbmp stays selected, cleaned up on next draw or destructor

Changes

DrawTheRoot (cached DC pattern for ReDrawLastDraw):

  • Clean up previous cached bitmap before creating new one
  • Create DC/bitmap, select custom bitmap
  • Keep custom bitmap selected in m_hdcMem for caching
  • Stock bitmap left alone

DrawTheRootRotated (local DC pattern):

  • Create local DC/bitmap, select custom bitmap
  • Restore stock bitmap before cleanup
  • Delete custom bitmap and DC
  • Added exception path cleanup

Pattern difference: DrawTheRoot caches the DC/bitmap for ReDrawLastDraw optimization (used when form is disabled). DrawTheRootRotated uses local resources since rotation complicates caching.

CI-ready checklist

  • Commit messages follow .github/commit-guidelines.md (subject ≤ 72 chars, no trailing punctuation; if body present, blank line then ≤ 80-char lines).
  • No whitespace warnings locally:
    git fetch origin
    git log --check --pretty=format:"---% h% s" origin/<base>..
    git diff --check --cached
  • Builds/tests pass locally (or I've run the CI-style build via Bash script or MSBuild). ⚠️ Cannot build in Linux; requires Windows/VS2022
  • For any Src/** folders touched, corresponding COPILOT.md files are updated or explicitly confirmed still accurate.

Notes for reviewers

Testing focus:

  • Visual artifacts during scrolling/resizing
  • GDI object count (Task Manager → GDI Objects) should remain stable
  • Disabled form scenario (triggers ReDrawLastDraw)

Technical context: Stock GDI objects (the default 1x1 bitmap in a new DC) must never be deleted per Windows GDI guidelines. The destructor already properly cleaned up cached bitmaps.

Documentation: BUFFERING_FIX.md and IMPLEMENTATION_SUMMARY.md added for technical reference.

Original prompt

Recently, you ripped up the buffering in the views project to solve some odd artifacts showing on the screen. It turns out we do need the buffering for drawing the views. Please re-implement the buffering code. Look at the get history to see what has changed for guidance. Don't just re-implement what was there, think about how best to implement the buffering code in a clean architecture ensuring that new visual artifacts are not created.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.


This change is Reviewable

Copilot AI and others added 4 commits November 22, 2025 18:38
- Fixed DrawTheRoot to properly manage GDI bitmap resources
  * Clean up previous cached bitmap/DC before creating new ones
  * Keep bitmap in m_hdcMem for ReDrawLastDraw support
  * Delete stock bitmap that comes with new DC (not the previous bitmap)

- Fixed DrawTheRootRotated to use local DC/bitmap
  * Uses local resources since rotation makes caching impractical
  * Properly cleanup bitmap and DC on exit and in exception paths

The previous code had a critical bug: it was deleting the OLD bitmap
(hbmpOld) immediately after selecting the new one, instead of keeping
it for restoration. This caused resource leaks and visual artifacts.

The new implementation follows the correct GDI resource management pattern:
1. Create compatible DC and bitmap
2. Select bitmap into DC (save old bitmap)
3. Delete the stock bitmap (not the old one!)
4. Draw to bitmap
5. Blit to screen
6. Keep bitmap/DC cached in m_hdcMem for ReDrawLastDraw (DrawTheRoot)
   OR cleanup local resources (DrawTheRootRotated, DrawTheRootAt)

Co-authored-by: johnml1135 <13733556+johnml1135@users.noreply.github.com>
Co-authored-by: johnml1135 <13733556+johnml1135@users.noreply.github.com>
The previous fix incorrectly deleted the stock bitmap returned by
SelectObjectBitmap. Stock GDI objects should not be deleted by applications.

Changes:
- DrawTheRoot: Leave custom bitmap selected in m_hdcMem for caching
  (it gets deleted on next draw or in destructor)
- DrawTheRootRotated: Restore stock bitmap before cleanup, then delete
  custom bitmap and DC
- Updated documentation to explain correct stock bitmap handling

The correct pattern for local DCs (DrawTheRootRotated, DrawTheRootAt):
1. Create DC with CreateCompatibleDC (has stock bitmap)
2. Create custom bitmap and select it (saves stock bitmap)
3. Draw to custom bitmap
4. Blit to screen
5. Restore stock bitmap into DC
6. Delete custom bitmap
7. Delete DC

The correct pattern for cached DCs (DrawTheRoot):
1. Clean up previous cached custom bitmap if any
2. Create DC with CreateCompatibleDC
3. Create custom bitmap and select it (stock bitmap is returned but not deleted)
4. Draw to custom bitmap
5. Blit to screen
6. Keep DC and custom bitmap for ReDrawLastDraw

This ensures proper GDI resource management without attempting to delete
stock objects, which can cause issues in debug builds.

Co-authored-by: johnml1135 <13733556+johnml1135@users.noreply.github.com>
Co-authored-by: johnml1135 <13733556+johnml1135@users.noreply.github.com>
Copilot AI changed the title [WIP] Re-implement buffering code for views project Fix GDI resource leaks and visual artifacts in VwDrawRootBuffered Nov 22, 2025
Copilot AI requested a review from johnml1135 November 22, 2025 18:46
@johnml1135 johnml1135 marked this pull request as ready for review November 22, 2025 19:41
Copilot AI review requested due to automatic review settings November 22, 2025 19:41
@johnml1135 johnml1135 merged commit 3466d25 into spec-kit-rebase Nov 22, 2025
4 checks passed
@johnml1135 johnml1135 deleted the copilot/reimplement-buffering-code branch November 22, 2025 19:41
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes critical GDI bitmap management bugs in VwDrawRootBuffered that were causing resource leaks and visual artifacts. The root cause was incorrect handling of stock vs. custom GDI bitmaps - the code was deleting stock bitmaps (which should never be deleted) and leaking custom bitmaps (which should be deleted when no longer needed).

Key changes:

  • Fixed DrawTheRoot to properly cache custom bitmap in m_hdcMem without deleting stock bitmap
  • Fixed DrawTheRootRotated to use local DC/bitmap with proper cleanup and stock bitmap restoration
  • Added exception handling cleanup for DrawTheRootRotated to prevent resource leaks

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
Src/views/VwRootBox.cpp Fixed GDI resource management in DrawTheRoot (cached DC pattern) and DrawTheRootRotated (local DC pattern) methods to properly handle stock vs. custom bitmaps
IMPLEMENTATION_SUMMARY.md Added comprehensive technical documentation explaining the bug, fix, architecture decisions, and testing strategy
BUFFERING_FIX.md Added detailed explanation of the root cause, solution patterns, and key principles for GDI bitmap handling

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants