Merge branch 'osx-bitmap-image'

Avoid crash in wxButton when setting invalid pressed bitmap in wxOSX and
other button bitmaps-related improvements.

See https://github.com/wxWidgets/wxWidgets/pull/2508

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
 -----------------------------------------------------

include/wx/osx/bitmap.h

@@ -204,16 +204,17 @@ public:
     // returns a CGImageRef which must released after usage with CGImageRelease
     CGImageRef CreateCGImage() const ;
 
-    WXImage GetImage() const;
+    // returns nil for invalid bitmap
+    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

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
+        Returns the bitmap used when the mouse is over the button.
-        invalid.
 
-        @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)

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;
+    // Bitmap can be set either explicitly, when the bitmap for the given state
-    virtual void SetBitmap(const wxBitmap& bitmap, wxAnyButton::State which) = 0;
+    // 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);
+                wxBitmap bmpNormal = m_imageData->GetImplicitBitmap(State_Normal);
-                m_imageData->SetBitmap(which == State_Disabled
+                m_imageData->SetImplicitBitmap(which == State_Disabled
                                             ? bmpNormal.ConvertToDisabled()
                                             : bmpNormal,
                                        which);
@@ -714,9 +755,9 @@ 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) || bitmap.IsNull(),
+        wxASSERT_MSG( which == State_Normal,
                       "Must set normal bitmap with the new size first" );
 
 #if wxUSE_UXTHEME
@@ -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<State>(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();

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
 

src/osx/cocoa/dataview.mm

@@ -3438,10 +3438,7 @@ bool wxDataViewIconTextRenderer::MacRender()
 
     cell = (wxImageTextCell*) GetNativeData()->GetItemCell();
     iconText << GetValue();
-    if (iconText.GetIcon().IsOk())
+    [cell setImage:iconText.GetIcon().GetNSImage()];
-        [cell setImage:wxBitmap(iconText.GetIcon()).GetNSImage()];
-    else
-        [cell setImage:nil];
     [cell setStringValue:wxCFStringRef(iconText.GetText()).AsNSString()];
     return true;
 }

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()];
-            [m_osxView setImage:bitmap.GetNSImage()];
-        else
-            [m_osxView setImage:nil];
-
         [m_osxView setNeedsDisplay:YES];
     }
 }

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,9 +949,9 @@ bool wxBitmap::Create(CGContextRef bitmapcontext)
     return GetBitmapData()->IsOk() ;
 }
 
-WXImage wxBitmap::GetImage() const
+WXImage wxBitmap::OSXGetImage() const
 {
-    return GetBitmapData()->GetImage();
+    return IsOk() ? GetBitmapData()->GetImage() : NULL;
 }
 
 wxBitmap wxBitmap::GetSubBitmap(const wxRect &rect) const

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;

tests/controls/buttontest.cpp

@@ -156,10 +156,26 @@ 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());
     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