#493 Copy semantics for GDI objects are inconsistent

[Vidar Hasfjord]

The TGdiObject base class is designed as a reference-counted encapsulation around a GDI handle (e.g. HPEN, HBRUSH, HFONT, HPALETTE and HBITMAP). In effect, it works like a smart pointer. Many objects can be made, all encapsulating the same handle, i.e. the same underlying object. Copying a TGdiObject instance does not copy the underlying object pointed to by the handle. It merely copies the handle and keeps track of reference counts.

However, the derived classes (TPen, TBrush, TFont, TPalette and TBitmap) are not consistent in their copy semantics. In particular, TPalette and TBitmap implement deep copy in their copy constructors, as if these classes encapsulated the object, not just the handle. In short, they do not behave like smart pointers, as intended by the TGdiObject base class design.

The deep copying should be moved to a named member function (e.g. DeepCopy), and the copy constructors should behave consistently like smart pointers. Also, move constructors should be implemented, to eliminate superfluous and inefficient reference count updates.

[Vidar Hasfjord]

This issue was fixed as proposed in Owlet in [r5430].

This revision also adds move constructors.

[Ognyan Chernokozhev]

I think it would be best to have this change in the trunk for the future OWLNext 8 release.

[Vidar Hasfjord]

@jogybl wrote:

I think it would be best to have this change in the trunk for the future OWLNext 8 release.

Thanks for the feedback. I created this ticket a long time ago, when version 7 was still the trunk. Now, having retired, I no longer intend to take on the work to merge my changes into the trunk, but feel free to go ahead if you feel brave. :-)

The reason I updated this ticket now, is that I've returned to my overhaul of the GDI classes in Owlet, attempting to eliminate TGdiBase (with TGdiObject already eliminated), replaced by std::shared_ptr, thereby giving all the derived classes proper shared pointer semantics. I will post more on this when I complete the work.

[Ognyan Chernokozhev]

Claude Code:

---

I have merged [r5430] into the trunk, adapted to the trunk code, in [r8977].

The copy constructors of TPen, TBrush, TFont, TPalette and TBitmap now consistently share the source object's handle. A copy owns the handle only if the source object does. Move constructors have been added, and TPalette and TBitmap have a new member function DeepCopy that duplicates the underlying GDI object.

Differences from the Owlet fix:

• In the trunk, TBrush still uses the cache for solid brushes (removed in Owlet by [r5484]). The copy constructor must acquire a reference to the cache entry when copying a cached brush, as TBrush(HBRUSH, NoAutoDelete) does. Without that, the destructor of the copy releases a reference it never acquired, and the cache may then evict and delete the brush while the original is still in use. I confirmed this with a test.
• The NO_GDI_SHARE_HANDLES configuration macro has been removed, since it is incompatible with shared handle semantics.

The fix also resolves an existing bug in the trunk: the TPen and TBrush copy constructors ignored whether the source owned its handle. A copy of an object borrowing a handle (NoAutoDelete) therefore took ownership, and deleted the borrowed handle on destruction.

To find code relying on deep copies, I compiled the trunk sources and examples with the TBitmap and TPalette copy constructors deleted. These places now call DeepCopy:

• TCelArray: copy constructor and assignment operator.
• TBtnBitmap::UpdateFaceColor and TGlyphButton::SetGlyph (OWLExt).
• The aclock example: TClockWindow::Paint draws the clock hands onto a copy of the clock face bitmap.
• The classes example: TBmpViewWindow::CmCopy passes a copy to ToClipboard, which hands the handle over to the clipboard.

While doing so, I found that new TDib(TDib(clipboard)) and new TBitmap(TBitmap(clipboard)) in TBmpViewWindow::CmPaste did not actually copy anything. C++17 guarantees copy elision here, so the new objects wrapped handles owned by the clipboard. The same pattern appeared in the TClipboard documentation. Both have been corrected.

Note for users (breaking change): Copying a TBitmap or TPalette no longer duplicates the bitmap or palette. Code that modifies a copy while the original is still in use must call DeepCopy instead. This includes drawing onto the copy, changing its palette entries, or passing it to ToClipboard. Such code still compiles, so it needs to be reviewed manually when upgrading to OWLNext 8.