wxBitmapComboBox is broken under MSW

[troelsk]

In the widgets sample, wxBitmapComboBox yields numerous errors (*) after being filled with bitmaps, "Add widget icons" button, and subsequently, drawing the (selected) combo box item fails (sometimes blank, sometimes half blank, wrong font).

wxMSW trunk, Win7, MSVC2008

To get the dump below, apply patch adding a "Copy log" button to the widgets sample.

(*)
17:02:00: Combobox dropped down
..\..\src\msw\dc.cpp(191): 'SetStretchBltMode' failed with error 0x00000057 (the parameter is incorrect.).
c:\svn\wx29clean\include\wx/msw/private/dc.h(75): 'SetTextColor' failed with error 0x00000057 (the parameter is incorrect.).
c:\svn\wx29clean\include\wx/msw/private/dc.h(88): 'SetBkColor' failed with error 0x00000057 (the parameter is incorrect.).
..\..\src\msw\dc.cpp(2587): 'CreateCompatibleDC' failed with error 0x00000057 (the parameter is incorrect.).
..\..\src\msw\dc.cpp(2558): 'CreateCompatibleBitmap' failed with error 0x00000057 (the parameter is incorrect.).
..\..\src\msw\dc.cpp(2281): 'BitBlt' failed with error 0x00000006 (the handle is invalid.).
..\..\src\msw\dc.cpp(191): 'SetStretchBltMode' failed with error 0x00000057 (the parameter is incorrect.).
..\..\src\msw\dc.cpp(2290): 'StretchBlt' failed with error 0x00000006 (the handle is invalid.).
c:\svn\wx29clean\include\wx/msw/private/dc.h(75): 'SetTextColor' failed with error 0x00000057 (the parameter is incorrect.).
c:\svn\wx29clean\include\wx/msw/private/dc.h(88): 'SetBkColor' failed with error 0x00000057 (the parameter is incorrect.).
..\..\src\msw\dc.cpp(2300): 'StretchBlt' failed with error 0x00000006 (the handle is invalid.).
..\..\src\msw\dc.cpp(2319): 'BitBlt' failed with error 0x00000006 (the handle is invalid.).
..\..\src\msw\dc.cpp(199): 'SetStretchBltMode' failed with error 0x00000057 (the parameter is incorrect.).
..\..\src\msw\dc.cpp(199): 'SetStretchBltMode' failed with error 0x00000057 (the parameter is incorrect.).
c:\svn\wx29clean\include\wx/msw/private/dc.h(75): 'SetTextColor' failed with error 0x00000057 (the parameter is incorrect.).
c:\svn\wx29clean\include\wx/msw/private/dc.h(88): 'SetBkColor' failed with error 0x00000057 (the parameter is incorrect.).
c:\svn\wx29clean\include\wx/msw/private/dc.h(134): 'SetBkMode' failed with error 0x00000057 (the parameter is incorrect.).
..\..\src\msw\dc.cpp(1438): 'TextOut' failed with error 0x00000000 (the operation completed successfully.).
..\..\src\msw\textmeasure.cpp(106): 'GetTextExtentPoint32()' failed with error 0x00000057 (the parameter is incorrect.).
17:02:01: Combobox closed up

[troelsk]

"Copy log" button

[vadz]

I can see this too. I wonder whether it had worked before?

[johnr]

Replying to vadz:

I can see this too. I wonder whether it had worked before?

Yes it worked well until it was broken by r72938 in Nov 2012.

[ericj]

I see what's happening. The caching of the hdcs assumes that the combination window/hdc is unique and we use only wxWindow* as key into the hashmap.

In this particular case the wxPaintDCEx ctor in wxBitmapComboBox::MSWOnDraw is called with the same value for wxWindow*, but different values for dc, so the cache returns a wrong hdc.

Seems we need a combination of wxWindow * and hdc as key into the hashmap. I can only think of a 64bit (or 128bit on 64bit platforms) integer. This would lead to some pretty ugly code (casts) though.

Any suggestions for an elegant solution?

[ericj]

I refactored the code a bit:

The wxPaintDCInfo stucture now addionally stores the wxWindow* pointer.

Instead of a hashmap, i store them in a list and iterate over the list myself to find the entries.

As the cache only contains 0 or 1 entries almost every time, this might also be faster than the previous hashmap.

One thing which is not optimal:
I had to add a call to wxPaintDCImpl::EndPaint() to bmpcbox.cpp, because otherwise the cache entries would not be cleared. This would have to be done every time a wxPaintDCEx is created outside a WM_PAINT handler.

[ericj]

Addition: As usual, this is "open heart surgery" in a crucial part of the paint event handling, so excessive testing is welcome :)

[johnr]

Thanks for taking this up Eric. It does seem a shame to need dual identifiers for the cache just for wxPaintDCEx but I didn't come up with an easy solution that didn't involve adding dc to FindDCInCache and associated code while I was trying to understand things.
Anyhow your solution works in the widgets sample and in my code but I can't claim I have tested it excessively.

[johnr]

Replying to johnr:

It does seem a shame to need dual identifiers for the cache just for wxPaintDCEx but I didn't come up with an easy solution that didn't involve adding dc to FindDCInCache and associated code...

Actually, with another look, I did come up with a cheap option that works like the pre r72938 code and doesn't use a cache at all. That is, there was some code that pretended to access the cache but never passed anything != NULL to FindInCache as in FindInCache();
Patch is below

Index: src/msw/dcclient.cpp
===================================================================
--- src/msw/dcclient.cpp	(revision 73441)
+++ src/msw/dcclient.cpp	(working copy)
@@ -377,15 +377,7 @@
     wxCHECK_RET( dc, wxT("wxPaintDCEx requires an existing device context") );
 
     m_window = window;
-
-    m_hDC = FindDCInCache(m_window);
-    if ( !m_hDC )
-    {
-        // not in cache, record it there
-        gs_PaintDCInfos[m_window] = new wxPaintDCInfoExternal(dc);
-
-        m_hDC = dc;
-    }
+    m_hDC = dc;
 }

[johnr]

For the record:
I persisted looking for a simple solution because in the widgets sample I could only find one control using wxMSW wxPaintDCExImpl::wxPaintDCExImpl. wxBitmapComboBox uses it to paint each list item when displayed in the popup and also the text/bitmap box when the selection is > -1.

Pre r72938 code added the passed WXHDC to the cache but never passed a valid key to retrieve it.

I think Eric's solution is correct if we expect a performance gain from caching wxMSW wxPaintDCExImpl and the gain balances the required extra code.

[vadz]

I'm actually with John here, why do we need to cache HDCs used by wxPaintDCExImpl at all? This doesn't seem to make sense as we get them from the outside anyhow, so why go to all the extra trouble of caching them and then invalidating them when we never gain anything anyhow?

I'll just apply the patch from comment:7 because IMHO it's the simplest and the best fix for the problem.

Thanks!

[VZ]

(In [73625]) Don't cache HDC used by wxPaintDCEx in wxMSW.

This avoids the problem with mistakenly using wrong HDC in wxBitmapComboBox
code which was due to assuming that we can only ever have one paint HDC for
the given window -- while in wxBitmapComboBox case we are passed different
HDCs for the same window via WM_DRAWITEM.

Instead of fixing the cache, just don't use it at all for wxPaintDCEx as we
don't gain anything from doing it anyhow.

Closes #14842.