Merge branch 'paint-debug'

Detect invalid use of wxPaintDC/wxPaintEvent better.

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

docs/changes.txt

@@ -103,6 +103,10 @@ Changes in behaviour which may result in build errors
   then you will need to add these libraries to your make or project files
   yourself.
 
+- wxPaintEvent objects can no longer be created by the application code. This
+  was never supposed to work and is now forbidden at compile-time instead of
+  resulting in errors during run-time.
+
 - WXWIN_OS_DESCRIPTION doesn't exist any longer, use wxGetOsDescription().
 
 - Never documented and not always available private wxGetClipboardData()

include/wx/event.h

@@ -2335,39 +2335,17 @@ private:
  wxEVT_NC_PAINT
  */
 
-#if wxDEBUG_LEVEL && defined(__WXMSW__)
-    #define wxHAS_PAINT_DEBUG
-
-    // see comments in src/msw/dcclient.cpp where g_isPainting is defined
-    extern WXDLLIMPEXP_CORE int g_isPainting;
-#endif // debug
-
 class WXDLLIMPEXP_CORE wxPaintEvent : public wxEvent
 {
+    // This ctor is only intended to be used by wxWidgets itself, so it's
+    // intentionally declared as private when not building the library itself.
+#ifdef WXBUILDING
 public:
-    wxPaintEvent(int Id = 0)
+#endif // WXBUILDING
-        : wxEvent(Id, wxEVT_PAINT)
+    explicit wxPaintEvent(wxWindowBase* window = NULL);
-    {
-#ifdef wxHAS_PAINT_DEBUG
-        // set the internal flag for the duration of redrawing
-        g_isPainting++;
-#endif // debug
-    }
 
-    // default copy ctor and dtor are normally fine, we only need them to keep
+public:
-    // g_isPainting updated in debug build
+    // default copy ctor and dtor are fine
-#ifdef wxHAS_PAINT_DEBUG
-    wxPaintEvent(const wxPaintEvent& event)
-            : wxEvent(event)
-    {
-        g_isPainting++;
-    }
-
-    virtual ~wxPaintEvent()
-    {
-        g_isPainting--;
-    }
-#endif // debug
 
     virtual wxEvent *Clone() const wxOVERRIDE { return new wxPaintEvent(*this); }
 
@@ -2377,11 +2355,14 @@ private:
 
 class WXDLLIMPEXP_CORE wxNcPaintEvent : public wxEvent
 {
+    // This ctor is only intended to be used by wxWidgets itself, so it's
+    // intentionally declared as private when not building the library itself.
+#ifdef WXBUILDING
 public:
-    wxNcPaintEvent(int winid = 0)
+#endif // WXBUILDING
-        : wxEvent(winid, wxEVT_NC_PAINT)
+    explicit wxNcPaintEvent(wxWindowBase* window = NULL);
-        { }
 
+public:
     virtual wxEvent *Clone() const wxOVERRIDE { return new wxNcPaintEvent(*this); }
 
 private:

include/wx/msw/private/paint.h

@@ -0,0 +1,39 @@
+///////////////////////////////////////////////////////////////////////////////
+// Name:        wx/msw/private/paint.h
+// Purpose:     Helpers for handling repainting
+// Author:      Vadim Zeitlin
+// Created:     2020-02-10
+// Copyright:   (c) 2020 Vadim Zeitlin <vadim@wxwidgets.org>
+// Licence:     wxWindows licence
+///////////////////////////////////////////////////////////////////////////////
+
+#ifndef _WX_MSW_PRIVATE_PAINT_H_
+#define _WX_MSW_PRIVATE_PAINT_H_
+
+#include "wx/stack.h"
+
+namespace wxMSWImpl
+{
+
+// Data used by WM_PAINT handler
+struct PaintData
+{
+    explicit PaintData(wxWindowMSW* window_)
+        : window(window_),
+          createdPaintDC(false)
+    {
+    }
+
+    // The window being repainted (never null).
+    wxWindowMSW* const window;
+
+    // True if the user-defined paint handler created wxPaintDC.
+    bool createdPaintDC;
+};
+
+// Stack storing data for the possibly nested WM_PAINT handlers.
+extern wxStack<PaintData> paintStack;
+
+} // namespace wxMSWImpl
+
+#endif // _WX_MSW_PRIVATE_PAINT_H_

interface/wx/event.h

@@ -2215,9 +2215,14 @@ class wxPaintEvent : public wxEvent
 {
 public:
     /**
-        Constructor.
+        Constructor for exclusive use of wxWidgets itself.
+
+        Note that the objects of this class can @em not be created from
+        application code, they're only created by the library itself. If you
+        need a window to be repainted, use wxWindow::Refresh() instead of
+        trying to manually create an event of this class.
     */
-    wxPaintEvent(int id = 0);
+    explicit wxPaintEvent(wxWindow* window);
 };
 
 

src/common/event.cpp

@@ -458,6 +458,22 @@ wxString wxCommandEvent::GetString() const
     return m_cmdString;
 }
 
+// ----------------------------------------------------------------------------
+// wxPaintEvent and wxNcPaintEvent
+// ----------------------------------------------------------------------------
+
+wxPaintEvent::wxPaintEvent(wxWindowBase* window)
+    : wxEvent(window ? window->GetId() : 0, wxEVT_PAINT)
+{
+    SetEventObject(window);
+}
+
+wxNcPaintEvent::wxNcPaintEvent(wxWindowBase* window)
+    : wxEvent(window ? window->GetId() : 0, wxEVT_NC_PAINT)
+{
+    SetEventObject(window);
+}
+
 // ----------------------------------------------------------------------------
 // wxUpdateUIEvent
 // ----------------------------------------------------------------------------

src/dfb/window.cpp

@@ -684,8 +684,7 @@ void wxWindowDFB::PaintWindow(const wxRect& rect)
     // only send wxNcPaintEvent if drawing at least part of nonclient area:
     if ( !clientRect.Contains(rect) )
     {
-        wxNcPaintEvent eventNc(GetId());
+        wxNcPaintEvent eventNc(this);
-        eventNc.SetEventObject(this);
         HandleWindowEvent(eventNc);
     }
     else
@@ -697,8 +696,7 @@ void wxWindowDFB::PaintWindow(const wxRect& rect)
     // only send wxPaintEvent if drawing at least part of client area:
     if ( rect.Intersects(clientRect) )
     {
-        wxPaintEvent eventPt(GetId());
+        wxPaintEvent eventPt(this);
-        eventPt.SetEventObject(this);
         HandleWindowEvent(eventPt);
     }
     else

src/gtk/window.cpp

@@ -5205,12 +5205,10 @@ void wxWindowGTK::GTKSendPaintEvents(const GdkRegion* region)
             wxFAIL_MSG( "unsupported background style" );
     }
 
-    wxNcPaintEvent nc_paint_event( GetId() );
+    wxNcPaintEvent nc_paint_event( this );
-    nc_paint_event.SetEventObject( this );
     HandleWindowEvent( nc_paint_event );
 
-    wxPaintEvent paint_event( GetId() );
+    wxPaintEvent paint_event( this );
-    paint_event.SetEventObject( this );
     HandleWindowEvent( paint_event );
 
 #if wxGTK_HAS_COMPOSITING_SUPPORT

src/gtk1/glcanvas.cpp

@@ -65,8 +65,7 @@ extern "C" {
 static gint
 gtk_glwindow_map_callback( GtkWidget * WXUNUSED(widget), wxGLCanvas *win )
 {
-    wxPaintEvent event( win->GetId() );
+    wxPaintEvent event( win );
-    event.SetEventObject( win );
     win->HandleWindowEvent( event );
 
     win->GetUpdateRegion().Clear();
@@ -302,8 +301,7 @@ void wxGLCanvas::OnInternalIdle()
 {
     if (!m_updateRegion.IsEmpty())
     {
-        wxPaintEvent event( GetId() );
+        wxPaintEvent event( this );
-        event.SetEventObject( this );
         HandleWindowEvent( event );
 
         GetUpdateRegion().Clear();

src/gtk1/window.cpp

@@ -3599,12 +3599,10 @@ void wxWindowGTK::GtkSendPaintEvents()
         m_clearRegion.Clear();
     }
 
-    wxNcPaintEvent nc_paint_event( GetId() );
+    wxNcPaintEvent nc_paint_event( this );
-    nc_paint_event.SetEventObject( this );
     HandleWindowEvent( nc_paint_event );
 
-    wxPaintEvent paint_event( GetId() );
+    wxPaintEvent paint_event( this );
-    paint_event.SetEventObject( this );
     HandleWindowEvent( paint_event );
 
     m_clipPaintRegion = false;

src/motif/window.cpp

@@ -1621,8 +1621,7 @@ void wxWindow::DoPaint()
         eraseEvent.SetEventObject(this);
         HandleWindowEvent(eraseEvent);
 
-        wxPaintEvent event(GetId());
+        wxPaintEvent event(this);
-        event.SetEventObject(this);
         HandleWindowEvent(event);
 
         m_needsRefresh = false;

src/msw/dcclient.cpp

@@ -36,6 +36,7 @@
 #include "wx/stack.h"
 
 #include "wx/msw/private.h"
+#include "wx/msw/private/paint.h"
 
 // ----------------------------------------------------------------------------
 // local data structures
@@ -138,18 +139,6 @@ PaintDCInfos gs_PaintDCInfos;
 
 } // anonymous namespace
 
-// ----------------------------------------------------------------------------
-// global variables
-// ----------------------------------------------------------------------------
-
-#ifdef wxHAS_PAINT_DEBUG
-    // a global variable which we check to verify that wxPaintDC are only
-    // created in response to WM_PAINT message - doing this from elsewhere is a
-    // common programming error among wxWidgets programmers and might lead to
-    // very subtle and difficult to debug refresh/repaint bugs.
-    int g_isPainting = 0;
-#endif // wxHAS_PAINT_DEBUG
-
 // ===========================================================================
 // implementation
 // ===========================================================================
@@ -269,20 +258,13 @@ wxPaintDCImpl::wxPaintDCImpl( wxDC *owner, wxWindow *window ) :
 {
     wxCHECK_RET( window, wxT("NULL canvas in wxPaintDCImpl ctor") );
 
-#ifdef wxHAS_PAINT_DEBUG
+    using namespace wxMSWImpl;
-    if ( g_isPainting <= 0 )
+    wxCHECK_RET( !paintStack.empty(),
-    {
+                 "wxPaintDC can't be created outside wxEVT_PAINT handler" );
-        wxFAIL_MSG( wxT("wxPaintDCImpl may be created only in EVT_PAINT handler!") );
+    wxCHECK_RET( paintStack.top().window == window,
-
+                 "wxPaintDC must be associated with the window being repainted" );
-        return;
-    }
-#endif // wxHAS_PAINT_DEBUG
-
-    // see comments in src/msw/window.cpp where this is defined
-    extern wxStack<bool> wxDidCreatePaintDC;
-
-    wxDidCreatePaintDC.top() = true;
 
+    paintStack.top().createdPaintDC = true;
 
     m_window = window;
 

src/msw/window.cpp

@@ -63,7 +63,6 @@
 #include "wx/popupwin.h"
 #include "wx/power.h"
 #include "wx/scopeguard.h"
-#include "wx/stack.h"
 #include "wx/sysopt.h"
 
 #if wxUSE_DRAG_AND_DROP
@@ -81,6 +80,7 @@
 
 #include "wx/msw/private.h"
 #include "wx/msw/private/keyboard.h"
+#include "wx/msw/private/paint.h"
 #include "wx/msw/private/winstyle.h"
 #include "wx/msw/dcclient.h"
 #include "wx/msw/seh.h"
@@ -5240,8 +5240,7 @@ wxColour wxWindowMSW::MSWGetThemeColour(const wchar_t *themeName,
 // endless stream of WM_PAINT messages for this window resulting in a lot of
 // difficult to debug problems (e.g. impossibility to repaint other windows,
 // lack of timer and idle events and so on)
-extern wxStack<bool> wxDidCreatePaintDC;
+wxStack<wxMSWImpl::PaintData> wxMSWImpl::paintStack;
-wxStack<bool> wxDidCreatePaintDC;
 
 bool wxWindowMSW::HandlePaint()
 {
@@ -5257,14 +5256,16 @@ bool wxWindowMSW::HandlePaint()
 
     m_updateRegion = wxRegion((WXHRGN) hRegion);
 
-    wxDidCreatePaintDC.push(false);
+    using namespace wxMSWImpl;
 
-    wxPaintEvent event(m_windowId);
+    paintStack.push(PaintData(this));
-    event.SetEventObject(this);
+
+    wxPaintEvent event(this);
 
     bool processed = HandleWindowEvent(event);
 
-    if ( wxDidCreatePaintDC.top() && !processed )
+    const bool createdPaintDC = paintStack.top().createdPaintDC;
+    if ( createdPaintDC && !processed )
     {
         // Event handler did paint something as wxPaintDC object was created
         // but then it must have skipped the event to indicate that default
@@ -5278,8 +5279,7 @@ bool wxWindowMSW::HandlePaint()
     // note that we must generate NC event after the normal one as otherwise
     // BeginPaint() will happily overwrite our decorations with the background
     // colour
-    wxNcPaintEvent eventNc(m_windowId);
+    wxNcPaintEvent eventNc(this);
-    eventNc.SetEventObject(this);
     HandleWindowEvent(eventNc);
 
     // don't keep an HRGN we don't need any longer (GetUpdateRegion() can only
@@ -5288,16 +5288,16 @@ bool wxWindowMSW::HandlePaint()
 
     wxPaintDCImpl::EndPaint((wxWindow *)this);
 
+    paintStack.pop();
+
     // It doesn't matter whether the event was actually processed or not here,
     // what matters is whether we already painted, and hence validated, the
     // window or not. If we did, either the event was processed or we called
     // OnPaint() above, so we should return true. If we did not, even the event
     // was processed, we must still call MSWDefWindowProc() to ensure that the
     // window is validated, i.e. to avoid the problem described in the comment
-    // before wxDidCreatePaintDC definition above.
+    // before paintStack definition above.
-    const bool ret = wxDidCreatePaintDC.top();
+    return createdPaintDC;
-    wxDidCreatePaintDC.pop();
-    return ret;
 }
 
 // Can be called from an application's OnPaint handler

src/osx/window_osx.cpp

@@ -1989,9 +1989,8 @@ bool wxWindowMac::MacDoRedraw( long time )
     {
         // paint the window itself
 
-        wxPaintEvent event(GetId());
+        wxPaintEvent event(this);
         event.SetTimestamp(time);
-        event.SetEventObject(this);
         handled = HandleWindowEvent(event);
     }
 
@@ -2040,8 +2039,7 @@ void wxWindowMac::MacPaintChildrenBorders()
         if ( m_updateRegion.Contains(clientOrigin.x+x-10, clientOrigin.y+y-10, w+20, h+20) )
         {
             // paint custom borders
-            wxNcPaintEvent eventNc( child->GetId() );
+            wxNcPaintEvent eventNc( child );
-            eventNc.SetEventObject( child );
             if ( !child->HandleWindowEvent( eventNc ) )
             {
                 child->MacPaintBorders(0, 0) ;

src/qt/glcanvas.cpp

@@ -62,7 +62,7 @@ void wxQtGLWidget::resizeGL(int w, int h)
 
 void wxQtGLWidget::paintGL()
 {
-    wxPaintEvent event( GetHandler()->GetId() );
+    wxPaintEvent event( GetHandler() );
     EmitEvent(event);
 }
 

src/qt/window.cpp

@@ -1281,8 +1281,7 @@ bool wxWindowQt::QtHandlePaintEvent ( QWidget *handler, QPaintEvent *event )
                 }
 
                 // send the paint event (wxWindowDC will draw directly):
-                wxPaintEvent paint( GetId() );
+                wxPaintEvent paint( this );
-                paint.SetEventObject(this);
                 handled = ProcessWindowEvent(paint);
                 m_updateRegion.Clear();
             }

src/univ/topluniv.cpp

@@ -216,8 +216,7 @@ long wxTopLevelWindow::GetDecorationsStyle() const
 
 void wxTopLevelWindow::RefreshTitleBar()
 {
-    wxNcPaintEvent event(GetId());
+    wxNcPaintEvent event(this);
-    event.SetEventObject(this);
     GetEventHandler()->ProcessEvent(event);
 }
 

src/x11/window.cpp

@@ -1282,8 +1282,7 @@ void wxWindowX11::SendPaintEvents()
 
     m_clipPaintRegion = true;
 
-    wxPaintEvent paint_event( GetId() );
+    wxPaintEvent paint_event( this );
-    paint_event.SetEventObject( this );
     HandleWindowEvent( paint_event );
 
     m_updateRegion.Clear();
@@ -1324,8 +1323,7 @@ void wxWindowX11::SendNcPaintEvents()
         }
     }
 
-    wxNcPaintEvent nc_paint_event( GetId() );
+    wxNcPaintEvent nc_paint_event( this );
-    nc_paint_event.SetEventObject( this );
     HandleWindowEvent( nc_paint_event );
 
     m_updateNcArea = false;

tests/events/evthandler.cpp

@@ -534,3 +534,14 @@ public:
     void OnIdle(wxIdleEvent&) { }
 };
 #endif // C++11
+
+// Another compilation-time-only test, but this one checking that these event
+// objects can't be created from outside of the library.
+#ifdef TEST_INVALID_EVENT_CREATION
+
+void TestEventCreation()
+{
+    wxPaintEvent eventPaint;
+}
+
+#endif // TEST_INVALID_EVENT_CREATION

tests/test.bkl

@@ -365,9 +365,9 @@ failtest_combobox:
 
 failtest_evthandler:
 	@$(RM) test_evthandler.o
-	@for d in GLOBAL STATIC METHOD FUNCTOR NO_HANDLER DERIVED WRONG_CLASS; do \
+	@for d in BIND_GLOBAL BIND_STATIC BIND_METHOD BIND_FUNCTOR BIND_NO_HANDLER BIND_DERIVED BIND_WRONG_CLASS EVENT_CREATION; do \
-	    if $(MAKE) CXXWARNINGS=-DTEST_INVALID_BIND_$$d test_evthandler.o 2>/dev/null; then \
+	    if $(MAKE) CXXWARNINGS=-DTEST_INVALID_$$d test_evthandler.o 2>/dev/null; then \
-	        echo "*** Compilation with TEST_INVALID_BIND_$$d unexpectedly succeeded.">&2; \
+	        echo "*** Compilation with TEST_INVALID_$$d unexpectedly succeeded.">&2; \
 	        exit 1; \
 	    fi; \
 	done; \