wxOwnerDrawnComboBox is no longer redrawn when displaying a wxColourDialog in wxEVT_COMBOBOX (since wxWidgets-3.1.6) (Issue #22362)

taler21

unread,

Apr 25, 2022, 8:48:58 AM4/25/22

to wx-...@googlegroups.com, Subscribed

Describe the bug
A wxOwnerDrawnComboBox is no longer redrawn when displaying a wxColourDialog in the event handler of its wxEVT_COMBOBOX event.

Up to and including wxWidgets-3.1.5 it worked without any problems.
The change in behaviour was introduced with commit d311c705d7ffc9f97c900dd4ed5d21489577f583.

To Reproduce
This issue can be reproduced with the combo sample using the following modifications:

patch for combo sample

diff --git "a/samples/combo/combo.cpp" "b/samples/combo/combo.cpp"
index 59399d839b..3d39cfe71a 100644
--- "a/samples/combo/combo.cpp"
+++ "b/samples/combo/combo.cpp"
@@ -34,6 +34,7 @@
 
 #include "wx/combo.h"
 #include "wx/odcombo.h"
+#include "wx/colordlg.h"
 
 // ----------------------------------------------------------------------------
 // resources
@@ -172,7 +173,17 @@ bool MyApp::OnInit()
 
 class wxPenStyleComboBox : public wxOwnerDrawnComboBox
 {
+    wxColour m_PenColour;
+
+    void OnSelected(wxCommandEvent& event);
+    wxDECLARE_EVENT_TABLE();
+
 public:
+    wxPenStyleComboBox()
+    {
+        m_PenColour = *wxBLACK;
+    }
+
     virtual void OnDrawItem( wxDC& dc,
                              const wxRect& rect,
                              int item,
@@ -212,6 +223,7 @@ public:
         wxPen pen( dc.GetTextForeground(), 3, penStyle );
 
         // Get text colour as pen colour
+        pen.SetColour( m_PenColour );
         dc.SetPen( pen );
 
         if ( !(flags & wxODCB_PAINTING_CONTROL) )
@@ -262,6 +274,28 @@ public:
 
 };
 
+wxBEGIN_EVENT_TABLE( wxPenStyleComboBox, wxOwnerDrawnComboBox )
+    EVT_COMBOBOX(wxID_ANY, wxPenStyleComboBox::OnSelected)
+wxEND_EVENT_TABLE()
+
+void wxPenStyleComboBox::OnSelected(wxCommandEvent& event)
+{
+    wxColourData data;
+    data.SetChooseFull( true );
+    data.SetColour( m_PenColour );
+
+    wxColourDialog dlg(this, &data);
+    dlg.Center();
+    if ( dlg.ShowModal() == wxID_OK )
+    {
+        m_PenColour = dlg.GetColourData().GetColour();
+    }
+
+    //Refresh();
+
+    event.Skip();
+}
+
 
 // ----------------------------------------------------------------------------
 // wxListView Custom popup interface

Steps to reproduce the behaviour:

Start the sample.
Open the selection list of the top combobox.
Select (for example) 'Long Dash'.
Move the displayed wxColourDialog so that it hides only half of the wxOwnerDrawnComboBox.
Choose a colour, for example red.
Only the hidden part of the wxOwnerDrawnComboBox will be redrawn.

It will be fully redrawn when you move the mouse over it.

Notice that the folowing API error is logged.

..\..\src\msw\window.cpp(595): 'SetFocus' failed with error 0x00000057 (Wrong parameter.).

So unfortunately, commit a5a5b1b doesn't seem to have fixed all 'SetFocus' failures introduced with commit d311c70 after all.

A possible fix or workaround would be to call Refresh() in the wxEVT_COMBOBOX event handler, but I don't think that's intentional to be necessary now, isn't it?

Platform and version information

wxWidgets version you use: 3.1.6 and latest master (5d55918)
wxWidgets port you use: wxMSW
OS and its version: Windows 7 and Windows 10

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.

VZ

unread,

Apr 25, 2022, 8:31:41 PM4/25/22

to wx-...@googlegroups.com, Subscribed

I think you do need to call Refresh() because I don't see what else can guarantee that the combobox gets redrawn. In fact, when testing under Windows 7 I don't see anything being redrawn at all, i.e. the combobox remains fully black until the mouse moves over it or it's refreshed in some other way (and then it becomes fully red). The fact that it "worked" before seems more like a fluke, or at least I see no reasonable explanation for why should it work reliably, do you?

The focus error message is due to the fact that wxComboCtrl does weird things with focus and this does seem to be worth fixing, but it's not really critical and I'm not sure if it's a regression neither. I'll try to look at it, but this doesn't seem nearly as bad as I first thought.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

taler21

unread,

Apr 26, 2022, 1:55:41 AM4/26/22

to wx-...@googlegroups.com, Subscribed

I think you do need to call Refresh() because I don't see what else can guarantee that the combobox gets redrawn.

The fact that it "worked" before seems more like a fluke, or at least I see no reasonable explanation for why should it work reliably, do you?

All I can say is that up to and including wxWidgets-3.1.5 it worked without calling Refresh() on MSW and still works on Linux with wxWidgets-3.1.6 as well (tested with GTK2).
The change in behaviour was introduced with commit d311c70.

But if you say calling Refresh() is the right thing to do, I will change my code to do that. Thanks for the clarification!

The focus error message is due to the fact that wxComboCtrl does weird things with focus and this does seem to be worth fixing, but it's not really critical and I'm not sure if it's a regression neither. I'll try to look at it, but this doesn't seem nearly as bad as I first thought.

It's definitely a regression in wxWidgets-3.1.6 introduced with commit d311c70.
Commit a5a5b1b fixed some of these introduced 'SetFocus' failures, but unfortunately not for this case.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

VZ

unread,

Apr 27, 2022, 2:04:29 PM4/27/22

to wx-...@googlegroups.com, Subscribed

I think calling Refresh() makes sense here and it definitely doesn't/can't do any harm, so yes, I'd do it in any case. But it's a bit weird that this changed after d311c70, so I'll still try to debug it to understand why exactly did it happen because perhaps it could result in other changes in behaviour and in this case I might need to do something to restore compatibility.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

VZ

unread,

May 12, 2022, 2:02:09 PM5/12/22

to wx-...@googlegroups.com, Subscribed

I've finally looked at this and did find a problem (see the commit above), fixing which should make the original code work again (at least it works fine with the provided patch now), so thanks again for reporting this!

I still believe that Refresh() would do no harm here and I'm not sure if the other systems actually refresh the parent window after dismissing the dialog, as MSW does, but at least for MSW compatibility should be restored now, so I think this can be closed.

But please let me know if you still see anything wrong, TIA!

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

VZ

unread,

May 12, 2022, 2:02:10 PM5/12/22

to wx-...@googlegroups.com, Subscribed

Closed #22362.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

taler21

unread,

May 13, 2022, 1:20:34 AM5/13/22

to wx-...@googlegroups.com, Subscribed

I can confirm that it is now working again as before d311c70.
Thanks for looking at this and fixing the regression!

Will use Refresh(). Thank you for pointing this out and explaining what is best practice in this case!