Opened 8 years ago

Closed 2 months ago

Last modified 6 weeks ago

#3901 closed defect (fixed)

wxCommandEvent::GetString() returns empty string for combobox events

Reported by: blizzymadden Owned by: vadz
Priority: normal Milestone: 3.1.0
Component: GUI-all Version:
Keywords: wxBitmapComboBox wxValidator Cc: blizzymadden, p_michalczyk
Blocked By: Blocking:
Patch: yes

Description

If you hook up a wxGenericValidator (with a wxString in it) to a wxBitmapComboBox, the string never gets updated when you call TransferDataToWindow() and vice versa. Example:

wxBitmapComboBox* languageCombo = new wxBitmapComboBox(this, wxID_ANY, _T(""), wxDefaultPosition, wxSize(100, -1), choiceStrings, wxCB_DROPDOWN|wxCB_READONLY, wxGenericValidator(&m_langName));

No matter what "m_langName" is, the combobox is empty when it appears, and when you call TransferDataToWindow(), m_langName is emptied. If I swap in a regular wxComboBox into this code it works perfectly.

Attachments (2)

Save-current-value-of-wxComboBox-in-event.patch download (716 bytes) - added by awi 3 months ago.
Save current value in the event object.
text-event-string-test.diff download (2.0 KB) - added by vadz 2 months ago.
Unit test patch attempting (but failing) to reproduce the problem

Download all attachments as: .zip

Change History (20)

comment:1 Changed 8 years ago by p_michalczyk

Please send some working code, then I will take a closer look at it.

comment:2 Changed 8 years ago by blizzymadden

Do you need the code for the entire dialog? Just drop this into any dialog's creation code and note that this combobox is empty when it appears. And when you call TransferDataFromWindow() from the OnOK event then the string variable is blank. Swapping in a regular wxComboBox will say "Danish" when it appears and will set the string variable to whatever was selected when you call TransferDataFromWindow().

wxArrayString choiceStrings;
choiceStrings.Add(_("None"));
choiceStrings.Add(_("Danish"));

wxString m_langName = _("Danish");

wxBitmapComboBox* languageCombo = new wxBitmapComboBox(this, wxID_ANY,
_T(""), wxDefaultPosition, wxSize(100,
-1), choiceStrings,
wxCB_DROPDOWN|wxCB_READONLY,
wxGenericValidator(&m_langName));

Add languageCombo to a sizer or the dialog

TransferDataToWindow();

comment:3 Changed 8 years ago by p_michalczyk

I'm trying to get wxBitmapComboBox to work with validators, but there are some difficulties. This difficulties complicate solution in such a way, that this solution cannot be thought of as simple/clean. And I don't want to post dirty hacks just to make things work for a while. So please be patient. I won't forget about it.

comment:4 Changed 8 years ago by p_michalczyk

As a workaround you use BitmapComboBox without CB_READONLY style.

comment:5 Changed 6 years ago by wojdyr

  • Component set to GUI-all
  • Keywords wxBitmapComboBox wxValidator added

comment:6 Changed 3 months ago by oneeyeman

This ticket is still stands.
For the reference here is the patch to the widgets sample I used for testing:

diff -bru wxWidgets.orig/samples/widgets/bmpcombobox.cpp wxWidgets/samples/widgets/bmpcombobox.cpp
--- wxWidgets.orig/samples/widgets/bmpcombobox.cpp	2014-07-31 21:17:54.000000000 -0700
+++ wxWidgets/samples/widgets/bmpcombobox.cpp	2014-08-02 23:00:26.000000000 -0700
@@ -40,6 +40,7 @@
 #endif
 
 #include "wx/stattext.h"
+#include "wx/valgen.h"
 #include "wx/dc.h"
 #include "wx/dcmemory.h"
 #include "wx/sizer.h"
@@ -185,6 +186,7 @@
                *m_textDelete;
 
 private:
+	wxString m_langName;
     wxDECLARE_EVENT_TABLE();
     DECLARE_WIDGETS_PAGE(BitmapComboBoxWidgetsPage)
 };
@@ -384,11 +386,15 @@
 
     // right pane
     wxSizer *sizerRight = new wxBoxSizer(wxVERTICAL);
+	wxArrayString choices;
+	choices.Add( "None" );
+	choices.Add( "Language" );
+	m_langName = "Language";
     m_combobox = new wxBitmapComboBox();
     m_combobox->Create(this, BitmapComboBoxPage_Combo, wxEmptyString,
                        wxDefaultPosition, wxDefaultSize,
-                       0, NULL,
-                       wxCB_READONLY);
+                       choices,
+                       wxCB_READONLY, wxGenericValidator(&m_langName));
 
 #if defined(wxGENERIC_BITMAPCOMBOBOX)
     // This will sure make the list look nicer when larger images ar

comment:7 Changed 3 months ago by vadz

  • Status changed from new to confirmed

comment:8 Changed 3 months ago by awi

  • Patch set

I tested (latest trunk, Win 7) wxGenericValidator validator with both wxBitmapComboBox and wxComboBox and with and without wxCB_READONLY flag and everything seems to work fine. TransferDataToWindow() updates selection in the combobox and TransferDataFromWindow() updates corresponding variable.
I don't see in the previous test case the call of TransferDataToWindow() just after combobox creation and maybe this was the reason of unsuccessful test?

The only problem I encountered was failed assertion in wxEVT_TEXT event handler (ComboboxWidgetsPage::OnComboText) after TransferDataToWindow() call (for control in non-read-only mode). This happened due to the discrepancy between text selected in the control and value received in the event object.
When selection is done manually then just selected value is stored in the wxEVT_TEXT event object. When selection is done by validator then wxEVT_TEXT event creation is delegated to wxTextEntry::SetValue() which by default doesn't store selected text in the event object.
Because apparently it was assumed for comboboxes that selected value should be stored in the wxEVT_TEXT event object then for the sake of consistency it should be also done in wxTextEntry::SetValue() for these particular controls.
Patch fixing this issue: Save-current-value-of-wxComboBox-in-event.patch

Test case:

  • samples/widgets/bmpcombobox.cpp

    a b  
    4040#endif 
    4141 
    4242#include "wx/stattext.h" 
     43#include "wx/valgen.h" 
    4344#include "wx/dc.h" 
    4445#include "wx/dcmemory.h" 
    4546#include "wx/sizer.h" 
    protected: 
    185186               *m_textDelete; 
    186187 
    187188private: 
     189    wxString m_langName; 
    188190    wxDECLARE_EVENT_TABLE(); 
    189191    DECLARE_WIDGETS_PAGE(BitmapComboBoxWidgetsPage) 
    190192}; 
    void BitmapComboBoxWidgetsPage::CreateContent() 
    384386 
    385387    // right pane 
    386388    wxSizer *sizerRight = new wxBoxSizer(wxVERTICAL); 
     389    wxArrayString choices; 
     390    choices.Add( "None" ); 
     391    choices.Add( "Language" ); 
     392    m_langName = "Language"; 
    387393    m_combobox = new wxBitmapComboBox(); 
    388394    m_combobox->Create(this, BitmapComboBoxPage_Combo, wxEmptyString, 
    389395                       wxDefaultPosition, wxDefaultSize, 
    390                        0, NULL, 
    391                        wxCB_READONLY); 
     396//                       0, NULL, 
     397//                       wxCB_READONLY); 
     398                       choices, 
     399                       0/*wxCB_READONLY*/, wxGenericValidator(&m_langName)); 
     400    TransferDataToWindow(); 
    392401 
    393402#if defined(wxGENERIC_BITMAPCOMBOBOX) 
    394403    // This will sure make the list look nicer when larger images are used. 
    void BitmapComboBoxWidgetsPage::OnButtonDeleteSel(wxCommandEvent& WXUNUSED(event 
    540549 
    541550void BitmapComboBoxWidgetsPage::OnButtonClear(wxCommandEvent& WXUNUSED(event)) 
    542551{ 
    543     m_combobox->Clear(); 
     552    TransferDataFromWindow(); 
     553    wxLogMessage(wxString::Format("Transferred value: %s", m_langName)); 
     554 
     555//    m_combobox->Clear(); 
    544556} 
    545557 
    546558void BitmapComboBoxWidgetsPage::OnButtonInsert(wxCommandEvent& WXUNUSED(event)) 
  • samples/widgets/combobox.cpp

    a b  
    4343#include "itemcontainer.h" 
    4444#include "widgets.h" 
    4545#include "icons/combobox.xpm" 
     46#include "wx/valgen.h" 
    4647 
    4748// ---------------------------------------------------------------------------- 
    4849// constants 
    protected: 
    168169               *m_textCur; 
    169170 
    170171private: 
     172   wxString m_langName; 
    171173    wxDECLARE_EVENT_TABLE(); 
    172174    DECLARE_WIDGETS_PAGE(ComboboxWidgetsPage) 
    173175}; 
    void ComboboxWidgetsPage::CreateContent() 
    375377 
    376378    // right pane 
    377379    wxSizer *sizerRight = new wxBoxSizer(wxVERTICAL); 
     380    wxArrayString choices; 
     381    choices.Add( "None" ); 
     382    choices.Add( "Language" ); 
     383    m_langName = "Language"; 
     384 
    378385    m_combobox = new wxComboBox(this, ComboPage_Combo, wxEmptyString, 
    379386                                wxDefaultPosition, wxDefaultSize, 
    380                                 0, NULL, 
    381                                 0); 
     387//                                0, NULL, 
     388  //                              0); 
     389                       choices, 
     390                       0/*wxCB_READONLY*/, wxGenericValidator(&m_langName)); 
     391    TransferDataToWindow(); 
     392 
    382393    sizerRight->Add(m_combobox, 0, wxGROW | wxALL, 5); 
    383394    sizerRight->SetMinSize(150, 0); 
    384395    m_sizerCombo = sizerRight; // save it to modify it later 
    void ComboboxWidgetsPage::OnButtonSetValue(wxCommandEvent& WXUNUSED(event)) 
    512523 
    513524void ComboboxWidgetsPage::OnButtonClear(wxCommandEvent& WXUNUSED(event)) 
    514525{ 
    515     m_combobox->Clear(); 
     526    TransferDataFromWindow(); 
     527    wxLogMessage(wxString::Format("Transferred value: %s", m_langName)); 
     528 
     529//    m_combobox->Clear(); 
    516530} 
    517531 
    518532void ComboboxWidgetsPage::OnButtonInsert(wxCommandEvent& WXUNUSED(event)) 

Changed 3 months ago by awi

Save current value in the event object.

comment:9 Changed 3 months ago by oneeyeman

AWI,
The original bug report says, that when the wxBitmapComboBox is created with the Generic Validator, then the value is not initially present/selected in the control. Please re-read the OP.

Therefore I think that the bug is still stands.

comment:10 Changed 3 months ago by vadz

  • Milestone set to 3.1.0
  • Summary changed from validator doesn't work with wxBitmapComboBox to wxCommandEvent::GetString() returns empty string for combobox events

Damn, how did we manage to break wxEvent::GetString() for combobox events like this without even noticing it :-( I would have thought the tests would have caught this but apparently not. Thanks a lot for noticing this!

Anyhow, this patch should definitely be applied to 3.0. For the trunk I think it would be preferable to have a virtual FillEventText() method instead.

And, Igor, I'm saying this in the most polite way possible, but please reread the original report and awi's comments yourself. I have no idea why do you think awi is wrong when he is absolutely correct and your example from comment:6 indeed never had any chance of working because the data is never transferred (again, thanks for noticing this, awi).

comment:11 follow-up: Changed 3 months ago by vadz

Hmm, another idea would be to make wxCommandEvent::GetString() also check for wxComboBox. This doesn't have any real advantage performance-wise (well, we save on wxComboBox::GetValue() call if it's never needed, but this is minor), but at least it keeps all the code related to this hack in the same place.

Any thoughts about the relative [dis]advantages of these approaches?

comment:12 in reply to: ↑ 11 Changed 3 months ago by awi

Replying to vadz:

Hmm, another idea would be to make wxCommandEvent::GetString() also check for wxComboBox. This doesn't have any real advantage performance-wise (well, we save on wxComboBox::GetValue() call if it's never needed, but this is minor), but at least it keeps all the code related to this hack in the same place.

Definitely, it would be better to have all this stuff in one place.
The only potential drawback I see is that if someone would like to get the old control value from the event object and modify control value directly from within the event handler then the only valid sequence of operations would be:

  • Get old value from the event object (using event.GetString).
  • Modify current control value (using e.g. event.GetEventObject()->SetValue).

Otherwise the 'old' value would be overwritten.

comment:13 follow-up: Changed 2 months ago by vadz

I must be missing something here. I tried to add a quick unit test to test that the problem is not only fixed now but will remain fixed, but the test passes, under MSW, even without any further changes, see the attached test patch.

What is going wrong (right?) here?

Changed 2 months ago by vadz

Unit test patch attempting (but failing) to reproduce the problem

comment:14 in reply to: ↑ 13 Changed 2 months ago by awi

Replying to vadz:

What is going wrong (right?) here?

If in your test unit the new value would be equal to the current value
(by replacing entry->SetValue("abcdef"); with entry->SetValue("abcdefg");)
then execution path would the same as if it was executed by validator and test should fail because empty string will be passed to the event.

comment:15 Changed 2 months ago by vadz

  • Owner set to vadz
  • Status changed from confirmed to accepted

Great, thanks for the tip, I do see the bug in the test now and will check in them and the fix soon.

Thanks again!

comment:16 Changed 2 months ago by VZ

  • Resolution set to fixed
  • Status changed from accepted to closed

In 77056:

Return correct string from wxEVT_TEXT wxComboBox events.

wxCommandEvent::GetString() could return empty string for the
programmatically-generated wxEVT_TEXT events from a wxComboBox.

Fix this by extending the on-demand string retrieval in wxCommandEvent to
wxComboBox as well (it was done only for wxTextCtrl).

Closes #3901.

comment:17 Changed 2 months ago by VZ

In 77057:

Return correct string from wxEVT_TEXT wxComboBox events.

wxCommandEvent::GetString() could return empty string for the
programmatically-generated wxEVT_TEXT events from a wxComboBox.

Fix this by extending the on-demand string retrieval in wxCommandEvent to
wxComboBox as well (it was done only for wxTextCtrl).

Also add a unit test checking that the string has the expected value in the
events sent by all wxTextEntry-derived controls.

Closes #3901.

comment:18 Changed 6 weeks ago by VZ

In 77661:

Fix crash in unit tests after TextEntryTestCase::Editable().

The class TextEventHandler added in r77057 (see #3901) setup an event handler
which wasn't disconnected when the handler was destroyed, which resulted in a
crash later as the window it was connected to continued to exist and generate
wxEVT_TEXT events.

Note: See TracTickets for help on using tickets.