From 5c6bce627b289d2c935f3a73ad0977f880d8707d Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 26 Nov 2022 19:52:09 +0100 Subject: [PATCH 1/6] Check wxImageList validity in all of its methods Some of wxImageList methods asserted when called on an invalid image list while others just failed silently. Assert in all of them now for consistency and to help detecting problems in the code using invalid wxImageList objects. Extend the documentation and the tests. --- docs/changes.txt | 5 +++++ interface/wx/imaglist.h | 4 ++++ src/generic/imaglist.cpp | 9 +++++++-- src/msw/imaglist.cpp | 16 ++++++++++++++++ tests/graphics/imagelist.cpp | 22 ++++++++++++++++------ 5 files changed, 48 insertions(+), 8 deletions(-) diff --git a/docs/changes.txt b/docs/changes.txt index d1118c9390..5373966573 100644 --- a/docs/changes.txt +++ b/docs/changes.txt @@ -41,6 +41,11 @@ Changes in behaviour not resulting in compilation errors even if the grid is currently too small to show all of them or if there are no unfrozen rows/columns remaining. +- Calling wxImageList methods on an invalid wxImageList object now consistently + results in assert failures instead of just failing silently. To avoid these + asserts, make sure that wxImageList is created with a valid size before + operating on it. + Changes in behaviour which may result in build errors ----------------------------------------------------- diff --git a/interface/wx/imaglist.h b/interface/wx/imaglist.h index 55d3a68826..720690813b 100644 --- a/interface/wx/imaglist.h +++ b/interface/wx/imaglist.h @@ -48,6 +48,10 @@ class wxImageList : public wxObject public: /** Default ctor. + + Note that the object created using the default ctor is invalid and + calling any methods other than Create() on it will result in an + assertion failure. */ wxImageList(); diff --git a/src/generic/imaglist.cpp b/src/generic/imaglist.cpp index 20dc318daa..4a0df58511 100644 --- a/src/generic/imaglist.cpp +++ b/src/generic/imaglist.cpp @@ -112,8 +112,7 @@ wxBitmap wxGenericImageList::GetImageListBitmap(const wxBitmap& bitmap) const int wxGenericImageList::Add( const wxBitmap &bitmap ) { // Cannot add image to invalid list - if ( m_size == wxSize(0, 0) ) - return -1; + wxCHECK_MSG( m_size != wxSize(0, 0), -1, "Invalid image list" ); // We use the logical size here as image list images size is specified in // logical pixels, just as window coordinates and sizes are. @@ -160,6 +159,8 @@ int wxGenericImageList::Add( const wxBitmap& bitmap, const wxColour& maskColour const wxBitmap *wxGenericImageList::DoGetPtr( int index ) const { + wxCHECK_MSG( m_size != wxSize(0, 0), nullptr, "Invalid image list" ); + if ( index < 0 || (size_t)index >= m_images.size() ) return nullptr; @@ -208,6 +209,8 @@ wxGenericImageList::Replace(int index, bool wxGenericImageList::Remove( int index ) { + wxCHECK_MSG( m_size != wxSize(0, 0), false, "Invalid image list" ); + if ( index < 0 || (size_t)index >= m_images.size() ) return false; @@ -218,6 +221,8 @@ bool wxGenericImageList::Remove( int index ) bool wxGenericImageList::RemoveAll() { + wxCHECK_MSG( m_size != wxSize(0, 0), false, "Invalid image list" ); + m_images.clear(); return true; diff --git a/src/msw/imaglist.cpp b/src/msw/imaglist.cpp index d326621bec..e3ecff5ef7 100644 --- a/src/msw/imaglist.cpp +++ b/src/msw/imaglist.cpp @@ -293,6 +293,8 @@ wxImageList::GetImageListBitmaps(wxMSWBitmaps& bitmaps, // 'bitmap' and 'mask'. int wxImageList::Add(const wxBitmap& bitmap, const wxBitmap& mask) { + wxASSERT_MSG( m_hImageList, wxT("invalid image list") ); + wxMSWBitmaps bitmaps; GetImageListBitmaps(bitmaps, bitmap, mask); @@ -310,6 +312,8 @@ int wxImageList::Add(const wxBitmap& bitmap, const wxBitmap& mask) // 'bitmap'. int wxImageList::Add(const wxBitmap& bitmap, const wxColour& maskColour) { + wxASSERT_MSG( m_hImageList, wxT("invalid image list") ); + wxMSWBitmaps bitmaps; wxMask mask(bitmap, maskColour); GetImageListBitmaps(bitmaps, bitmap, mask.GetBitmap()); @@ -342,6 +346,8 @@ bool wxImageList::Replace(int index, const wxBitmap& bitmap, const wxBitmap& mask) { + wxASSERT_MSG( m_hImageList, wxT("invalid image list") ); + wxMSWBitmaps bitmaps; GetImageListBitmaps(bitmaps, bitmap, mask); @@ -365,6 +371,8 @@ bool wxImageList::Replace(int i, const wxIcon& icon) // Removes the image at the given index. bool wxImageList::Remove(int index) { + wxASSERT_MSG( m_hImageList, wxT("invalid image list") ); + bool ok = index >= 0 && ImageList_Remove(GetHImageList(), index) != FALSE; if ( !ok ) { @@ -377,6 +385,8 @@ bool wxImageList::Remove(int index) // Remove all images bool wxImageList::RemoveAll() { + wxASSERT_MSG( m_hImageList, wxT("invalid image list") ); + // don't use ImageList_RemoveAll() because mingw32 headers don't have it bool ok = ImageList_Remove(GetHImageList(), -1) != FALSE; if ( !ok ) @@ -397,6 +407,8 @@ bool wxImageList::Draw(int index, int flags, bool solidBackground) { + wxASSERT_MSG( m_hImageList, wxT("invalid image list") ); + wxDCImpl *impl = dc.GetImpl(); wxMSWDCImpl *msw_impl = wxDynamicCast( impl, wxMSWDCImpl ); if (!msw_impl) @@ -445,6 +457,8 @@ bool wxImageList::Draw(int index, // Get the bitmap wxBitmap wxImageList::GetBitmap(int index) const { + wxASSERT_MSG( m_hImageList, wxT("invalid image list") ); + int bmp_width = 0, bmp_height = 0; GetSize(index, bmp_width, bmp_height); @@ -508,6 +522,8 @@ wxBitmap wxImageList::GetBitmap(int index) const // Get the icon wxIcon wxImageList::GetIcon(int index) const { + wxASSERT_MSG( m_hImageList, wxT("invalid image list") ); + HICON hIcon = ImageList_ExtractIcon(0, GetHImageList(), index); if (hIcon) { diff --git a/tests/graphics/imagelist.cpp b/tests/graphics/imagelist.cpp index bbe0b564d5..4e564ff91a 100644 --- a/tests/graphics/imagelist.cpp +++ b/tests/graphics/imagelist.cpp @@ -609,11 +609,11 @@ TEST_CASE("ImageList:NegativeTests", "[imagelist][negative]") CHECK(h == 0); #endif - int idx = il.Add(bmp); - CHECK(idx == -1); #ifdef __WXDEBUG__ + CHECK_THROWS(il.Add(bmp)); REQUIRE_THROWS(il.GetImageCount()); #else + CHECK(il.Add(bmp) == -1); CHECK(il.GetImageCount() == 0); #endif } @@ -644,23 +644,33 @@ TEST_CASE("ImageList:NegativeTests", "[imagelist][negative]") CHECK(h == 0); #endif - int idx = il.Add(bmp); - CHECK(idx == -1); #ifdef __WXDEBUG__ + CHECK_THROWS(il.Add(bmp)); REQUIRE_THROWS(il.GetImageCount()); #else + CHECK(il.Add(bmp) == -1); CHECK(il.GetImageCount() == 0); #endif - ok = il.Replace(0, bmp); - CHECK_FALSE(ok); #ifdef __WXDEBUG__ + CHECK_THROWS(il.Replace(0, bmp)); REQUIRE_THROWS(il.GetImageCount()); #else + CHECK_FALSE(il.Replace(0, bmp)); CHECK(il.GetImageCount() == 0); #endif } + SECTION("Add to invalid image list") + { + wxImageList il; +#ifdef __WXDEBUG__ + CHECK_THROWS( il.Add(bmp) ); +#else + CHECK( il.Add(bmp) == -1 ); +#endif + } + SECTION("Invalid Get/Replace/Remove indices") { wxImageList il(32, 32, false); From 02f7a5361dec17f2c70eb30d8f0779b7eaa7ae64 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 26 Nov 2022 20:08:35 +0100 Subject: [PATCH 2/6] Fix wxImageList creation in wxGenericPrintSetupDialog Size must be provided when creating wxImageList, otherwise calling Add() on it fails later. --- src/generic/prntdlgg.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/generic/prntdlgg.cpp b/src/generic/prntdlgg.cpp index 2792f55d55..391c27d181 100644 --- a/src/generic/prntdlgg.cpp +++ b/src/generic/prntdlgg.cpp @@ -487,8 +487,9 @@ void wxGenericPrintSetupDialog::Init(wxPrintData* data) m_printerListCtrl = new wxListCtrl( this, wxPRINTID_PRINTER, wxDefaultPosition, wxSize(wxDefaultCoord,100), wxLC_REPORT|wxLC_SINGLE_SEL|wxSUNKEN_BORDER ); - wxImageList *image_list = new wxImageList; - image_list->Add( wxBitmap(check_xpm) ); + wxBitmap checkBmp(check_xpm); + wxImageList *image_list = new wxImageList( checkBmp.GetWidth(), checkBmp.GetHeight() ) ; + image_list->Add( checkBmp ); m_printerListCtrl->AssignImageList( image_list, wxIMAGE_LIST_SMALL ); m_printerListCtrl->InsertColumn( 0, wxT(" "), wxLIST_FORMAT_LEFT, 20 ); From 6832a58695049ddcf0feb159243a2b696cd4fd5b Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 26 Nov 2022 20:09:36 +0100 Subject: [PATCH 3/6] Fix inserting items into wxListCtrl in wxGenericPrintSetupDialog Set the ID before calling InsertItem(), otherwise it just fails (inserting an item without ID used to work in the generic version but never worked in the MSW one and was changed back in 2.9 to not work in the generic version neither for consistency). --- src/generic/prntdlgg.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/generic/prntdlgg.cpp b/src/generic/prntdlgg.cpp index 391c27d181..dcbb1af81e 100644 --- a/src/generic/prntdlgg.cpp +++ b/src/generic/prntdlgg.cpp @@ -501,7 +501,8 @@ void wxGenericPrintSetupDialog::Init(wxPrintData* data) item.SetMask( wxLIST_MASK_TEXT ); item.SetColumn( 1 ); item.SetText( _("Default printer") ); - item.SetId( m_printerListCtrl->InsertItem( item ) ); + item.SetId( 0 ); + m_printerListCtrl->InsertItem( item ); if (data->GetPrinterName().empty()) { From f076655b384dda11d4a4799fa0e09e3436c9e652 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 26 Nov 2022 20:10:57 +0100 Subject: [PATCH 4/6] Stop dispatching events from wxGenericPrintSetupDialog ctor Block while waiting for "lpstat -v" to execute: it shouldn't take a long time to run it and dispatching events from inside this dialog ctor was really unexpected. --- src/generic/prntdlgg.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/generic/prntdlgg.cpp b/src/generic/prntdlgg.cpp index dcbb1af81e..d18e1e856f 100644 --- a/src/generic/prntdlgg.cpp +++ b/src/generic/prntdlgg.cpp @@ -519,7 +519,7 @@ void wxGenericPrintSetupDialog::Init(wxPrintData* data) wxArrayString errors; wxArrayString output; - long res = wxExecute( wxT("lpstat -v"), output, errors, wxEXEC_NODISABLE ); + long res = wxExecute( wxT("lpstat -v"), output, errors, wxEXEC_NOEVENTS ); if (res >= 0 && errors.GetCount() == 0) { size_t i; From 0c977490354ae38a87b51c2f5f2925b841935053 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 26 Nov 2022 20:13:02 +0100 Subject: [PATCH 5/6] Use simpler wxStaticBoxSizer ctor in wxGenericPrintDialog No real changes, just let wxStaticBoxSizer create wxStaticBox on its own instead of creating it explicitly just to give it to the sizer. This makes the code shorter and simpler. --- src/generic/prntdlgg.cpp | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/generic/prntdlgg.cpp b/src/generic/prntdlgg.cpp index d18e1e856f..61d5d68f05 100644 --- a/src/generic/prntdlgg.cpp +++ b/src/generic/prntdlgg.cpp @@ -169,8 +169,7 @@ void wxGenericPrintDialog::Init(wxWindow * WXUNUSED(parent)) wxPrintFactory* factory = wxPrintFactory::GetFactory(); - wxStaticBoxSizer *topsizer = new wxStaticBoxSizer( - new wxStaticBox( this, wxID_ANY, _( "Printer options" ) ), wxHORIZONTAL ); + wxStaticBoxSizer *topsizer = new wxStaticBoxSizer( wxHORIZONTAL, this, _( "Printer options" ) ); wxFlexGridSizer *flex = new wxFlexGridSizer( 2 ); flex->AddGrowableCol( 1 ); topsizer->Add( flex, 1, wxGROW ); @@ -482,7 +481,7 @@ void wxGenericPrintSetupDialog::Init(wxPrintData* data) // printer selection - wxStaticBoxSizer *printer_sizer = new wxStaticBoxSizer( new wxStaticBox( this, wxID_ANY, _("Printer") ), wxVERTICAL ); + wxStaticBoxSizer *printer_sizer = new wxStaticBoxSizer( wxVERTICAL, this, _("Printer") ); main_sizer->Add( printer_sizer, 0, wxALL|wxGROW, 10 ); m_printerListCtrl = new wxListCtrl( this, wxPRINTID_PRINTER, @@ -593,8 +592,7 @@ void wxGenericPrintSetupDialog::Init(wxPrintData* data) wxBoxSizer *item2 = new wxBoxSizer( wxVERTICAL ); - wxStaticBox *item4 = new wxStaticBox( this, wxPRINTID_STATIC, _("Paper size") ); - wxStaticBoxSizer *item3 = new wxStaticBoxSizer( item4, wxVERTICAL ); + wxStaticBoxSizer *item3 = new wxStaticBoxSizer( wxVERTICAL, this, _("Paper size") ); m_paperTypeChoice = CreatePaperTypeChoice(); item3->Add( m_paperTypeChoice, 0, wxALIGN_CENTER|wxALL, 5 ); @@ -609,8 +607,7 @@ void wxGenericPrintSetupDialog::Init(wxPrintData* data) m_orientationRadioBox= new wxRadioBox( this, wxPRINTID_ORIENTATION, _("Orientation"), wxDefaultPosition, wxDefaultSize, 2, strs6, 1, wxRA_SPECIFY_ROWS ); item2->Add( m_orientationRadioBox, 0, wxGROW|wxALIGN_CENTER_VERTICAL|wxALL, 5 ); - wxStaticBox *item8 = new wxStaticBox( this, wxID_ANY, _("Options") ); - wxStaticBoxSizer *item7 = new wxStaticBoxSizer( item8, wxHORIZONTAL ); + wxStaticBoxSizer *item7 = new wxStaticBoxSizer( wxHORIZONTAL, this, _("Options") ); m_colourCheckBox = new wxCheckBox( this, wxPRINTID_PRINTCOLOUR, _("Print in colour") ); item7->Add( m_colourCheckBox, 0, wxALIGN_CENTER|wxALL, 5 ); @@ -621,8 +618,7 @@ void wxGenericPrintSetupDialog::Init(wxPrintData* data) // spooling options (on the right) - wxStaticBox *item11 = new wxStaticBox( this, wxID_ANY, _("Print spooling") ); - wxStaticBoxSizer *item10 = new wxStaticBoxSizer( item11, wxVERTICAL ); + wxStaticBoxSizer *item10 = new wxStaticBoxSizer( wxVERTICAL, this, _("Print spooling") ); wxStaticText *item12 = new wxStaticText( this, wxID_ANY, _("Printer command:") ); item10->Add( item12, 0, wxALIGN_CENTER_VERTICAL|wxALL, 5 ); @@ -831,8 +827,7 @@ wxGenericPageSetupDialog::wxGenericPageSetupDialog( wxWindow *parent, wxBoxSizer *mainsizer = new wxBoxSizer( wxVERTICAL ); // 1) top - wxStaticBoxSizer *topsizer = new wxStaticBoxSizer( - new wxStaticBox(this,wxPRINTID_STATIC, _("Paper size")), wxHORIZONTAL ); + wxStaticBoxSizer *topsizer = new wxStaticBoxSizer( wxHORIZONTAL, this, _("Paper size") ); size_t n = wxThePrintPaperDatabase->GetCount(); wxString *choices = new wxString [n]; From efac76ec8eb066b74faf06358bb56426ef1c0958 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 26 Nov 2022 20:14:03 +0100 Subject: [PATCH 6/6] Rewrite wxGenericPrintSetupDialog sizer-related code Use sensible variable names instead of auto-generated itemNN and wxSizerFlags-based API. This makes the code simpler to read and modify and also avoids tons of asserts due to using meaningless sizer styles. --- src/generic/prntdlgg.cpp | 65 ++++++++++++++++++---------------------- 1 file changed, 29 insertions(+), 36 deletions(-) diff --git a/src/generic/prntdlgg.cpp b/src/generic/prntdlgg.cpp index 61d5d68f05..cfddd10c44 100644 --- a/src/generic/prntdlgg.cpp +++ b/src/generic/prntdlgg.cpp @@ -582,79 +582,72 @@ void wxGenericPrintSetupDialog::Init(wxPrintData* data) } } + const wxSizerFlags border = wxSizerFlags().Border(); - printer_sizer->Add( m_printerListCtrl, 0, wxALL|wxGROW, 5 ); + printer_sizer->Add( m_printerListCtrl, wxSizerFlags(border).Expand() ); - wxBoxSizer *item1 = new wxBoxSizer( wxHORIZONTAL ); - main_sizer->Add( item1, 0, wxALL, 5 ); + wxBoxSizer *all_options = new wxBoxSizer( wxHORIZONTAL ); + main_sizer->Add( all_options, border ); // printer options (on the left) - wxBoxSizer *item2 = new wxBoxSizer( wxVERTICAL ); + wxBoxSizer *print_options = new wxBoxSizer( wxVERTICAL ); - wxStaticBoxSizer *item3 = new wxStaticBoxSizer( wxVERTICAL, this, _("Paper size") ); + wxStaticBoxSizer *paper_size = new wxStaticBoxSizer( wxVERTICAL, this, _("Paper size") ); m_paperTypeChoice = CreatePaperTypeChoice(); - item3->Add( m_paperTypeChoice, 0, wxALIGN_CENTER|wxALL, 5 ); + paper_size->Add( m_paperTypeChoice, border ); - item2->Add( item3, 0, wxALIGN_CENTER|wxALL, 5 ); + print_options->Add( paper_size, border ); - wxString strs6[] = + wxString orientations[] = { _("Portrait"), _("Landscape") }; - m_orientationRadioBox= new wxRadioBox( this, wxPRINTID_ORIENTATION, _("Orientation"), wxDefaultPosition, wxDefaultSize, 2, strs6, 1, wxRA_SPECIFY_ROWS ); - item2->Add( m_orientationRadioBox, 0, wxGROW|wxALIGN_CENTER_VERTICAL|wxALL, 5 ); + m_orientationRadioBox= new wxRadioBox( this, wxPRINTID_ORIENTATION, _("Orientation"), wxDefaultPosition, wxDefaultSize, 2, orientations, 1, wxRA_SPECIFY_ROWS ); + print_options->Add( m_orientationRadioBox, wxSizerFlags(border).Expand() ); - wxStaticBoxSizer *item7 = new wxStaticBoxSizer( wxHORIZONTAL, this, _("Options") ); + wxStaticBoxSizer *other_options = new wxStaticBoxSizer( wxHORIZONTAL, this, _("Options") ); m_colourCheckBox = new wxCheckBox( this, wxPRINTID_PRINTCOLOUR, _("Print in colour") ); - item7->Add( m_colourCheckBox, 0, wxALIGN_CENTER|wxALL, 5 ); + other_options->Add( m_colourCheckBox, border ); - item2->Add( item7, 0, wxGROW|wxALIGN_CENTER_VERTICAL|wxALL, 5 ); + print_options->Add( other_options, wxSizerFlags(border).Expand() ); - item1->Add( item2, 0, wxALIGN_CENTER_HORIZONTAL, 5 ); + all_options->Add( print_options ); // spooling options (on the right) - wxStaticBoxSizer *item10 = new wxStaticBoxSizer( wxVERTICAL, this, _("Print spooling") ); + wxStaticBoxSizer *spool_options = new wxStaticBoxSizer( wxVERTICAL, this, _("Print spooling") ); - wxStaticText *item12 = new wxStaticText( this, wxID_ANY, _("Printer command:") ); - item10->Add( item12, 0, wxALIGN_CENTER_VERTICAL|wxALL, 5 ); - - wxBoxSizer *item13 = new wxBoxSizer( wxHORIZONTAL ); - - item13->Add( 20, 20, 0, wxALIGN_CENTER|wxALL, 5 ); + spool_options->Add( new wxStaticText( this, wxID_ANY, _("Printer command:") ), border ); m_printerCommandText = new wxTextCtrl( this, wxPRINTID_COMMAND, wxEmptyString, wxDefaultPosition, wxSize(160,wxDefaultCoord) ); - item13->Add( m_printerCommandText, 0, wxALIGN_CENTER|wxALL, 5 ); + wxBoxSizer *command_sizer = new wxBoxSizer( wxHORIZONTAL ); + command_sizer->Add( 20, 20 ); + command_sizer->Add( m_printerCommandText ); + spool_options->Add( command_sizer, border ); - item10->Add( item13, 0, wxALIGN_CENTER|wxALL, 0 ); - - wxStaticText *item15 = new wxStaticText( this, wxID_ANY, _("Printer options:") ); - item10->Add( item15, 0, wxALIGN_CENTER_VERTICAL|wxALL, 5 ); - - wxBoxSizer *item16 = new wxBoxSizer( wxHORIZONTAL ); - - item16->Add( 20, 20, 0, wxALIGN_CENTER|wxALL, 5 ); + spool_options->Add( new wxStaticText( this, wxID_ANY, _("Printer options:") ), border ); m_printerOptionsText = new wxTextCtrl( this, wxPRINTID_OPTIONS, wxEmptyString, wxDefaultPosition, wxSize(160,wxDefaultCoord) ); - item16->Add( m_printerOptionsText, 0, wxALIGN_CENTER|wxALL, 5 ); + wxBoxSizer *options_sizer = new wxBoxSizer( wxHORIZONTAL ); + options_sizer->Add( 20, 20 ); + options_sizer->Add( m_printerOptionsText ); + spool_options->Add( options_sizer, border ); - item10->Add( item16, 0, wxALIGN_CENTER|wxALL, 0 ); - - item1->Add( item10, 0, wxALIGN_CENTER_HORIZONTAL|wxALL, 5 ); + all_options->Add( spool_options, wxSizerFlags(border).Expand() ); #if wxUSE_STATLINE // static line - main_sizer->Add( new wxStaticLine( this, wxID_ANY ), 0, wxEXPAND | wxLEFT|wxRIGHT|wxTOP, 10 ); + main_sizer->Add( new wxStaticLine( this, wxID_ANY ), wxSizerFlags().Expand().DoubleBorder(wxLEFT|wxRIGHT|wxTOP) ); #endif // buttons - main_sizer->Add( CreateButtonSizer( wxOK|wxCANCEL), 0, wxEXPAND|wxALL, 10 ); + main_sizer->Add( CreateButtonSizer( wxOK|wxCANCEL), wxSizerFlags().Expand().DoubleBorder() ); SetSizer( main_sizer );