From ff4cd704122d7c60eda92ef7757060c97a324bf5 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Fri, 3 Sep 2021 00:28:58 +0200 Subject: [PATCH 1/7] Avoid crash in wxBitmap::GetImage() for invalid bitmaps Just return NULL from this (wxOSX private, in spite of not using a port-specific prefix) method. This fixes crash in wxButton::SetBitmapXXX(wxNullBitmap), as shown by the new test case which used to crash but doesn't do it any longer. Closes #19257. --- include/wx/osx/bitmap.h | 1 + src/osx/core/bitmap.cpp | 2 +- tests/controls/buttontest.cpp | 5 +++++ 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/include/wx/osx/bitmap.h b/include/wx/osx/bitmap.h index d1388a85fb..6484a5e7e2 100644 --- a/include/wx/osx/bitmap.h +++ b/include/wx/osx/bitmap.h @@ -204,6 +204,7 @@ public: // returns a CGImageRef which must released after usage with CGImageRelease CGImageRef CreateCGImage() const ; + // returns nil for invalid bitmap WXImage GetImage() const; #if wxOSX_USE_COCOA // returns an autoreleased version of the image diff --git a/src/osx/core/bitmap.cpp b/src/osx/core/bitmap.cpp index cb9f2163e3..b4e8163c6f 100644 --- a/src/osx/core/bitmap.cpp +++ b/src/osx/core/bitmap.cpp @@ -951,7 +951,7 @@ bool wxBitmap::Create(CGContextRef bitmapcontext) WXImage wxBitmap::GetImage() const { - return GetBitmapData()->GetImage(); + return IsOk() ? GetBitmapData()->GetImage() : NULL; } wxBitmap wxBitmap::GetSubBitmap(const wxRect &rect) const diff --git a/tests/controls/buttontest.cpp b/tests/controls/buttontest.cpp index c6a5725646..4a13623a6d 100644 --- a/tests/controls/buttontest.cpp +++ b/tests/controls/buttontest.cpp @@ -160,6 +160,11 @@ TEST_CASE_METHOD(ButtonTestCase, "Button::Bitmap", "[button]") // updating the bitmap later, as it used to be the case in wxGTK (#18898). m_button->SetLabel(wxString()); CHECK_NOTHROW( m_button->Disable() ); + + // Also check that setting an invalid bitmap doesn't do anything untoward, + // such as crashing, as it used to do in wxOSX (#19257). + CHECK_NOTHROW( m_button->SetBitmapPressed(wxNullBitmap) ); + CHECK( !m_button->GetBitmapPressed().IsOk() ); } #endif //wxUSE_BUTTON From 169a33c23820fea85614e61e6e1771886dc48cd3 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Fri, 3 Sep 2021 19:57:59 +0100 Subject: [PATCH 2/7] Remove always true check from assert in MSW wxAnyButton code The bitmap here is always valid as the invalid case is handled in the beginning of the function. --- src/msw/anybutton.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/msw/anybutton.cpp b/src/msw/anybutton.cpp index 67c7640b7b..adcf5ca9e1 100644 --- a/src/msw/anybutton.cpp +++ b/src/msw/anybutton.cpp @@ -716,7 +716,7 @@ void wxAnyButton::DoSetBitmap(const wxBitmap& bitmap, State which) if ( m_imageData && bitmap.GetSize() != m_imageData->GetBitmap(State_Normal).GetSize() ) { - wxASSERT_MSG( (which == State_Normal) || bitmap.IsNull(), + wxASSERT_MSG( which == State_Normal, "Must set normal bitmap with the new size first" ); #if wxUSE_UXTHEME From 0f79f69d88662b092d912c39bbf7702d7fdc197b Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Fri, 3 Sep 2021 20:12:19 +0100 Subject: [PATCH 3/7] Return empty bitmaps from wxButton if not explicitly set in wxMSW Do this for consistency with the other ports and because this seems more useful anyhow. Update the documentation to make this behaviour more clear and document this change as a (minor) incompatibility in wxMSW. Also add more unit tests to check for this behaviour. Note this also fixes the problem with the unit test added in the grandparent commit under MSW. --- docs/changes.txt | 4 ++ interface/wx/anybutton.h | 21 +++++---- src/msw/anybutton.cpp | 80 +++++++++++++++++++++++++++-------- tests/controls/buttontest.cpp | 11 +++++ 4 files changed, 90 insertions(+), 26 deletions(-) diff --git a/docs/changes.txt b/docs/changes.txt index c6bba1ebd9..7653c3a2f5 100644 --- a/docs/changes.txt +++ b/docs/changes.txt @@ -129,6 +129,10 @@ Changes in behaviour not resulting in compilation errors - wxSpinCtrl::SetValue(wxString) overload doesn't generate any events with wxMSW, which was already the documented behaviour. +- wxButton::GetBitmap{Current,Disabled,Focus,Pressed}() only return valid + bitmaps in wxMSW if the corresponding Set had been called before, as in the + other ports, instead of returning the normal bitmap as fallback in this case. + Changes in behaviour which may result in build errors ----------------------------------------------------- diff --git a/interface/wx/anybutton.h b/interface/wx/anybutton.h index c993b062e2..29c0eb12ce 100644 --- a/interface/wx/anybutton.h +++ b/interface/wx/anybutton.h @@ -41,10 +41,10 @@ public: wxBitmap GetBitmap() const; /** - Returns the bitmap used when the mouse is over the button, which may be - invalid. + Returns the bitmap used when the mouse is over the button. - @see SetBitmapCurrent() + The returned bitmap is only valid if SetBitmapCurrent() had been + previously called. @since 2.9.1 (available as wxBitmapButton::GetBitmapHover() in previous versions) @@ -52,18 +52,20 @@ public: wxBitmap GetBitmapCurrent() const; /** - Returns the bitmap for the disabled state, which may be invalid. + Returns the bitmap used for the disabled state. - @see SetBitmapDisabled() + The returned bitmap is only valid if SetBitmapDisabled() had been + previously called. @since 2.9.1 (available in wxBitmapButton only in previous versions) */ wxBitmap GetBitmapDisabled() const; /** - Returns the bitmap for the focused state, which may be invalid. + Returns the bitmap used for the focused state. - @see SetBitmapFocus() + The returned bitmap is only valid if SetBitmapFocus() had been + previously called. @since 2.9.1 (available in wxBitmapButton only in previous versions) */ @@ -82,9 +84,10 @@ public: wxBitmap GetBitmapLabel() const; /** - Returns the bitmap for the pressed state, which may be invalid. + Returns the bitmap used when the button is pressed. - @see SetBitmapPressed() + The returned bitmap is only valid if SetBitmapPressed() had been + previously called. @since 2.9.1 (available as wxBitmapButton::GetBitmapSelected() in previous versions) diff --git a/src/msw/anybutton.cpp b/src/msw/anybutton.cpp index adcf5ca9e1..21c9bd0823 100644 --- a/src/msw/anybutton.cpp +++ b/src/msw/anybutton.cpp @@ -101,11 +101,45 @@ extern wxWindowMSW *wxWindowBeingErased; // From src/msw/window.cpp class wxButtonImageData: public wxObject { public: - wxButtonImageData() { } + wxButtonImageData() + { + for ( int n = 0; n < wxAnyButton::State_Max; ++n ) + { + // The normal bitmap is always set explicitly when the image data + // is created, but the others ones are not (yet). + m_bitmapSetExplicitly[n] = n == wxAnyButton::State_Normal; + } + } + virtual ~wxButtonImageData() { } - virtual wxBitmap GetBitmap(wxAnyButton::State which) const = 0; - virtual void SetBitmap(const wxBitmap& bitmap, wxAnyButton::State which) = 0; + // Bitmap can be set either explicitly, when the bitmap for the given state + // is specified by the application, or implicitly, when the bitmap for some + // state is set as a side effect of setting another bitmap. + // + // These functions check the flags stored in the base class remembering + // whether each bitmap is implicit or explicit and behave accordingly. + wxBitmap GetExplicitBitmap(wxAnyButton::State which) const + { + return m_bitmapSetExplicitly[which] ? GetBitmap(which) : wxBitmap(); + } + + void SetExplicitBitmap(const wxBitmap& bitmap, wxAnyButton::State which) + { + SetBitmap(bitmap, which); + m_bitmapSetExplicitly[which] = true; + } + + wxBitmap GetImplicitBitmap(wxAnyButton::State which) const + { + return GetBitmap(which); + } + + void SetImplicitBitmap(const wxBitmap& bitmap, wxAnyButton::State which) + { + SetBitmap(bitmap, which); + m_bitmapSetExplicitly[which] = false; + } virtual wxSize GetBitmapMargins() const = 0; virtual void SetBitmapMargins(wxCoord x, wxCoord y) = 0; @@ -114,6 +148,13 @@ public: virtual void SetBitmapPosition(wxDirection dir) = 0; private: + // These functions are private to force using explicit/implicit versions of + // them in the code to make it clear which bitmap is needed. + virtual wxBitmap GetBitmap(wxAnyButton::State which) const = 0; + virtual void SetBitmap(const wxBitmap& bitmap, wxAnyButton::State which) = 0; + + bool m_bitmapSetExplicitly[wxAnyButton::State_Max]; + wxDECLARE_NO_COPY_CLASS(wxButtonImageData); }; @@ -129,7 +170,7 @@ class wxODButtonImageData : public wxButtonImageData public: wxODButtonImageData(wxAnyButton *btn, const wxBitmap& bitmap) { - SetBitmap(bitmap, wxAnyButton::State_Normal); + SetExplicitBitmap(bitmap, wxAnyButton::State_Normal); #if wxUSE_IMAGE SetBitmap(bitmap.ConvertToDisabled(), wxAnyButton::State_Disabled); #endif @@ -505,7 +546,7 @@ void wxAnyButton::AdjustForBitmapSize(wxSize &size) const wxCHECK_RET( m_imageData, wxT("shouldn't be called if no image") ); // account for the bitmap size, including the user-specified margins - const wxSize sizeBmp = m_imageData->GetBitmap(State_Normal).GetSize() + const wxSize sizeBmp = m_imageData->GetImplicitBitmap(State_Normal).GetSize() + 2*m_imageData->GetBitmapMargins(); const wxDirection dirBmp = m_imageData->GetBitmapPosition(); if ( dirBmp == wxLEFT || dirBmp == wxRIGHT ) @@ -660,7 +701,7 @@ WXLRESULT wxAnyButton::MSWWindowProc(WXUINT nMsg, WXWPARAM wParam, WXLPARAM lPar if ( IsEnabled() && ( wxUxThemeIsActive() || - (m_imageData && m_imageData->GetBitmap(State_Current).IsOk()) + (m_imageData && m_imageData->GetImplicitBitmap(State_Current).IsOk()) ) ) { @@ -678,7 +719,7 @@ WXLRESULT wxAnyButton::MSWWindowProc(WXUINT nMsg, WXWPARAM wParam, WXLPARAM lPar wxBitmap wxAnyButton::DoGetBitmap(State which) const { - return m_imageData ? m_imageData->GetBitmap(which) : wxBitmap(); + return m_imageData ? m_imageData->GetExplicitBitmap(which) : wxBitmap(); } void wxAnyButton::DoSetBitmap(const wxBitmap& bitmap, State which) @@ -697,8 +738,8 @@ void wxAnyButton::DoSetBitmap(const wxBitmap& bitmap, State which) else { // Replace the removed bitmap with the normal one. - wxBitmap bmpNormal = m_imageData->GetBitmap(State_Normal); - m_imageData->SetBitmap(which == State_Disabled + wxBitmap bmpNormal = m_imageData->GetImplicitBitmap(State_Normal); + m_imageData->SetImplicitBitmap(which == State_Disabled ? bmpNormal.ConvertToDisabled() : bmpNormal, which); @@ -714,7 +755,7 @@ void wxAnyButton::DoSetBitmap(const wxBitmap& bitmap, State which) // Check if we already had bitmaps of different size. if ( m_imageData && - bitmap.GetSize() != m_imageData->GetBitmap(State_Normal).GetSize() ) + bitmap.GetSize() != m_imageData->GetImplicitBitmap(State_Normal).GetSize() ) { wxASSERT_MSG( which == State_Normal, "Must set normal bitmap with the new size first" ); @@ -767,7 +808,7 @@ void wxAnyButton::DoSetBitmap(const wxBitmap& bitmap, State which) } else { - m_imageData->SetBitmap(bitmap, which); + m_imageData->SetExplicitBitmap(bitmap, which); // if the focus bitmap is specified but current one isn't, use // the focus bitmap for hovering as well if this is consistent @@ -777,9 +818,9 @@ void wxAnyButton::DoSetBitmap(const wxBitmap& bitmap, State which) // and also makes it much easier to do "the right thing" for // all platforms (some of them, such as Windows, have "hot" // buttons while others don't) - if ( which == State_Focused && !m_imageData->GetBitmap(State_Current).IsOk() ) + if ( which == State_Focused && !m_imageData->GetExplicitBitmap(State_Current).IsOk() ) { - m_imageData->SetBitmap(bitmap, State_Current); + m_imageData->SetImplicitBitmap(bitmap, State_Current); } } @@ -1226,11 +1267,16 @@ void wxAnyButton::MakeOwnerDrawn() // if necessary. if ( m_imageData && wxDynamicCast(m_imageData, wxODButtonImageData) == NULL ) { - wxODButtonImageData* newData = new wxODButtonImageData(this, m_imageData->GetBitmap(State_Normal)); + wxODButtonImageData* newData = new wxODButtonImageData(this, m_imageData->GetImplicitBitmap(State_Normal)); for ( int n = 0; n < State_Max; n++ ) { State st = static_cast(n); - newData->SetBitmap(m_imageData->GetBitmap(st), st); + + wxBitmap bmp = m_imageData->GetExplicitBitmap(st); + if ( bmp.IsOk() ) + newData->SetExplicitBitmap(bmp, st); + else + newData->SetImplicitBitmap(m_imageData->GetImplicitBitmap(st), st); } newData->SetBitmapPosition(m_imageData->GetBitmapPosition()); wxSize margs = m_imageData->GetBitmapMargins(); @@ -1390,9 +1436,9 @@ bool wxAnyButton::MSWOnDraw(WXDRAWITEMSTRUCT *wxdis) // draw the image, if any if ( m_imageData ) { - wxBitmap bmp = m_imageData->GetBitmap(GetButtonState(this, state)); + wxBitmap bmp = m_imageData->GetImplicitBitmap(GetButtonState(this, state)); if ( !bmp.IsOk() ) - bmp = m_imageData->GetBitmap(State_Normal); + bmp = m_imageData->GetImplicitBitmap(State_Normal); const wxSize sizeBmp = bmp.GetSize(); const wxSize margin = m_imageData->GetBitmapMargins(); diff --git a/tests/controls/buttontest.cpp b/tests/controls/buttontest.cpp index 4a13623a6d..ce958bc5e9 100644 --- a/tests/controls/buttontest.cpp +++ b/tests/controls/buttontest.cpp @@ -156,6 +156,17 @@ TEST_CASE_METHOD(ButtonTestCase, "Button::Bitmap", "[button]") CHECK(m_button->GetBitmap().IsOk()); + // The call above shouldn't affect any other bitmaps as returned by the API + // even though the same (normal) bitmap does appear for all the states. + CHECK( !m_button->GetBitmapCurrent().IsOk() ); + CHECK( !m_button->GetBitmapDisabled().IsOk() ); + CHECK( !m_button->GetBitmapFocus().IsOk() ); + CHECK( !m_button->GetBitmapPressed().IsOk() ); + + // Do set one of the bitmaps now. + m_button->SetBitmapPressed(wxArtProvider::GetBitmap(wxART_ERROR)); + CHECK( m_button->GetBitmapPressed().IsOk() ); + // Check that resetting the button label doesn't result in problems when // updating the bitmap later, as it used to be the case in wxGTK (#18898). m_button->SetLabel(wxString()); From 6f4c57260b3ffcd50d29431f003e3cdd0f54af78 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Fri, 3 Sep 2021 21:26:05 +0200 Subject: [PATCH 4/7] Use wxBitmap::GetUIImage() in wxiOS code It looks like this should be the right function to use, rather than the more generic GetImage() which is going to be renamed in the upcoming commits. --- src/osx/iphone/button.mm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/osx/iphone/button.mm b/src/osx/iphone/button.mm index 6db36c9852..4fe7f56de3 100644 --- a/src/osx/iphone/button.mm +++ b/src/osx/iphone/button.mm @@ -111,7 +111,7 @@ wxWidgetImplType* wxWidgetImpl::CreateBitmapButton( wxWindowMac* wxpeer, v.frame = r; if (bitmap.IsOk()) - [v setImage:bitmap.GetImage() forState:UIControlStateNormal]; + [v setImage:bitmap.GetUIImage() forState:UIControlStateNormal]; wxWidgetIPhoneImpl* c = new wxWidgetIPhoneImpl( wxpeer, v ); return c; From be923dc9fe8d1c98b36dd445e7645495659023b4 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Fri, 3 Sep 2021 21:27:58 +0200 Subject: [PATCH 5/7] Use wxBitmap::GetNSImage() in Cocoa code Use Cocoa-specific function rather than generically-named GetImage(). No real changes. --- src/osx/carbon/graphics.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/osx/carbon/graphics.cpp b/src/osx/carbon/graphics.cpp index 8dfcacacef..77585ff9d0 100644 --- a/src/osx/carbon/graphics.cpp +++ b/src/osx/carbon/graphics.cpp @@ -2419,7 +2419,7 @@ void wxMacCoreGraphicsContext::DrawBitmap( const wxBitmap &bmp, wxDouble x, wxDo if (EnsureIsValid()) { CGRect r = CGRectMake( (CGFloat) x , (CGFloat) y , (CGFloat) w , (CGFloat) h ); - wxOSXDrawNSImage( m_cgContext, &r, bmp.GetImage()); + wxOSXDrawNSImage( m_cgContext, &r, bmp.GetNSImage()); } #else wxGraphicsBitmap bitmap = GetRenderer()->CreateBitmap(bmp); @@ -2482,7 +2482,7 @@ void wxMacCoreGraphicsContext::DrawIcon( const wxIcon &icon, wxDouble x, wxDoubl #if wxOSX_USE_COCOA { CGRect r = CGRectMake( (CGFloat) x , (CGFloat) y , (CGFloat) w , (CGFloat) h ); - wxOSXDrawNSImage( m_cgContext, &r, icon.GetImage()); + wxOSXDrawNSImage( m_cgContext, &r, icon.GetNSImage()); } #endif From 2574b9172e928099a8b410ac3aecfc19a1db65f7 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Fri, 3 Sep 2021 21:31:45 +0200 Subject: [PATCH 6/7] Rename wxBitmap::GetImage() to OSXGetImage() Make it more clear that this function is specific to Mac ports. --- include/wx/osx/bitmap.h | 6 +++--- src/osx/core/bitmap.cpp | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/include/wx/osx/bitmap.h b/include/wx/osx/bitmap.h index 6484a5e7e2..bae0e68537 100644 --- a/include/wx/osx/bitmap.h +++ b/include/wx/osx/bitmap.h @@ -205,16 +205,16 @@ public: CGImageRef CreateCGImage() const ; // returns nil for invalid bitmap - WXImage GetImage() const; + WXImage OSXGetImage() const; #if wxOSX_USE_COCOA // returns an autoreleased version of the image WX_NSImage GetNSImage() const - { return GetImage(); } + { return OSXGetImage(); } #endif #if wxOSX_USE_IPHONE // returns an autoreleased version of the image WX_UIImage GetUIImage() const - { return GetImage(); } + { return OSXGetImage(); } #endif #if WXWIN_COMPATIBILITY_3_0 diff --git a/src/osx/core/bitmap.cpp b/src/osx/core/bitmap.cpp index b4e8163c6f..d7211a50e4 100644 --- a/src/osx/core/bitmap.cpp +++ b/src/osx/core/bitmap.cpp @@ -782,7 +782,7 @@ wxBitmapRefData::~wxBitmapRefData() bool wxBitmap::CopyFromIcon(const wxIcon& icon) { - return Create( icon.GetImage() ); + return Create( icon.OSXGetImage() ); } wxBitmap::wxBitmap(const char bits[], int the_width, int the_height, int no_bits) @@ -949,7 +949,7 @@ bool wxBitmap::Create(CGContextRef bitmapcontext) return GetBitmapData()->IsOk() ; } -WXImage wxBitmap::GetImage() const +WXImage wxBitmap::OSXGetImage() const { return IsOk() ? GetBitmapData()->GetImage() : NULL; } From 89552a02f5b404b61b945c64214b5684d258c767 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Fri, 3 Sep 2021 21:35:03 +0200 Subject: [PATCH 7/7] Remove unnecessary checks before calling GetNSImage() Now that this function is guaranteed to return NULL if the bitmap is invalid, the code can be simplified to rely on it. No real changes. --- src/osx/cocoa/dataview.mm | 5 +---- src/osx/cocoa/window.mm | 6 +----- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/src/osx/cocoa/dataview.mm b/src/osx/cocoa/dataview.mm index 3b603a36e9..ac596c5114 100644 --- a/src/osx/cocoa/dataview.mm +++ b/src/osx/cocoa/dataview.mm @@ -3438,10 +3438,7 @@ bool wxDataViewIconTextRenderer::MacRender() cell = (wxImageTextCell*) GetNativeData()->GetItemCell(); iconText << GetValue(); - if (iconText.GetIcon().IsOk()) - [cell setImage:wxBitmap(iconText.GetIcon()).GetNSImage()]; - else - [cell setImage:nil]; + [cell setImage:iconText.GetIcon().GetNSImage()]; [cell setStringValue:wxCFStringRef(iconText.GetText()).AsNSString()]; return true; } diff --git a/src/osx/cocoa/window.mm b/src/osx/cocoa/window.mm index de39ab8afe..8858d3aaee 100644 --- a/src/osx/cocoa/window.mm +++ b/src/osx/cocoa/window.mm @@ -3444,11 +3444,7 @@ void wxWidgetCocoaImpl::SetBitmap( const wxBitmap& bitmap ) { if ( [m_osxView respondsToSelector:@selector(setImage:)] ) { - if (bitmap.IsOk()) - [m_osxView setImage:bitmap.GetNSImage()]; - else - [m_osxView setImage:nil]; - + [m_osxView setImage:bitmap.GetNSImage()]; [m_osxView setNeedsDisplay:YES]; } }