wxCheckListBox fixes: crash/memory leak when items are dynamically

inserted/deleted corrected and docs updated


git-svn-id: https://svn.wxwidgets.org/svn/wx/wxWidgets/trunk@1829 c3d73ce0-8a6f-49c7-b76d-6d57e0e08775

docs/latex/wx/.cvsignore

@@ -1,13 +1,13 @@
-wx.gid
+WX.GID
-wx.hlp
+WX.HLP
 wx.ref
 wx.cnt
-wx.rtf
+Wx.rtf
-wx.con
+Wx.con
 minimald.hpj
 minimald.ref
 minimald.con
-minimald.hlp
+MINIMALD.HLP
-minimald.gid
+minimald.GID
 minimald.cnt
 minimald.rtf

docs/latex/wx/checklst.tex

@@ -25,7 +25,11 @@ See \helpref{wxListBox}{wxlistbox}.
 
 \wxheading{Event handling}
 
-See \helpref{wxListBox}{wxlistbox}.
+\twocolwidtha{7cm}
+\begin{twocollist}\itemsep=0pt
+\twocolitem{{\bf EVT\_CHECKLISTBOX(id, func)}}{Process a wxEVT\_COMMAND\_CHECKLISTBOX\_TOGGLE event,
+when an item in the check list box is checked or unchecked.}
+\end{twocollist}
 
 \wxheading{See also}
 

include/wx/msw/checklst.h

@@ -40,7 +40,10 @@ public:
                  long style = 0,
                  const wxValidator& validator = wxDefaultValidator,
                  const wxString& name = wxListBoxNameStr);
-//                 const wxFont& font = wxNullFont);
+
+  // override base class virtuals
+  virtual void Delete(int n);
+  virtual void InsertItems(int nItems, const wxString items[], int pos);
 
   // items may be checked
   bool IsChecked(size_t uiIndex) const;

include/wx/msw/listbox.h

@@ -78,6 +78,9 @@ public:
 
     // allows to get the item and use SetXXX functions to set it's appearance
     wxOwnerDrawn *GetItem(size_t n) const { return m_aItems[n]; }
+
+    // get the index of the given item
+    int GetItemIndex(wxOwnerDrawn *item) const { return m_aItems.Index(item); }
 #endif // wxUSE_OWNER_DRAWN
 
     virtual void Append(const wxString& item);

src/msw/checklst.cpp

@@ -9,6 +9,14 @@
 // Licence:     wxWindows license
 ///////////////////////////////////////////////////////////////////////////////
 
+// ============================================================================
+// declarations
+// ============================================================================
+
+// ----------------------------------------------------------------------------
+// headers
+// ----------------------------------------------------------------------------
+
 #ifdef __GNUG__
 #pragma implementation "checklst.h"
 #endif
@@ -34,6 +42,14 @@
 #include "wx/msw/checklst.h"
 
 #include <windows.h>
+#include <windowsx.h>
+
+// ----------------------------------------------------------------------------
+// private functions
+// ----------------------------------------------------------------------------
+
+// get item (converted to right type)
+#define GetItem(n)    ((wxCheckListBoxItem *)(GetItem(n)))
 
 // ============================================================================
 // implementation
@@ -49,6 +65,7 @@
 
 class wxCheckListBoxItem : public wxOwnerDrawn
 {
+friend class wxCheckListBox;
 public:
   // ctor
   wxCheckListBoxItem(wxCheckListBox *pParent, size_t nIndex);
@@ -58,8 +75,8 @@ public:
 
   // simple accessors
   bool IsChecked() const  { return m_bChecked;        }
-  void Check(bool bCheck) { m_bChecked = bCheck;      }
+  void Check(bool bCheck);
-  void Toggle();
+  void Toggle() { Check(!IsChecked()); }
 
 private:
   bool            m_bChecked;
@@ -190,9 +207,20 @@ bool wxCheckListBoxItem::OnDrawItem(wxDC& dc, const wxRect& rc,
 }
 
 // change the state of the item and redraw it
-void wxCheckListBoxItem::Toggle()
+void wxCheckListBoxItem::Check(bool check)
 {
-  m_bChecked = !m_bChecked;
+    m_bChecked = check;
+
+    // index may be chanegd because new items were added/deleted
+    if ( m_pParent->GetItemIndex(this) != (int)m_nIndex )
+    {
+        // update it
+        int index = m_pParent->GetItemIndex(this);
+
+        wxASSERT_MSG( index != wxNOT_FOUND, "what does this item do here?" );
+
+        m_nIndex = (size_t)index;
+    }
 
     size_t nHeight = m_pParent->GetItemHeight();
     size_t y = m_nIndex * nHeight;
@@ -229,13 +257,40 @@ wxCheckListBox::wxCheckListBox(wxWindow *parent, wxWindowID id,
                                const wxPoint& pos, const wxSize& size,
                                int nStrings, const wxString choices[],
                                long style, const wxValidator& val,
-                               const wxString& name) // , const wxFont& font)
+                               const wxString& name)
-                // don't use ctor with arguments! we must call Create()
-                // ourselves from here.
               : wxListBox()
-//                , m_font(font)
 {
-  Create(parent, id, pos, size, nStrings, choices, style|wxLB_OWNERDRAW, val, name);
+    Create(parent, id, pos, size, nStrings, choices,
+           style | wxLB_OWNERDRAW, val, name);
+}
+
+void wxCheckListBox::Delete(int N)
+{
+    wxCHECK_RET( N >= 0 && N < m_noItems,
+                 "invalid index in wxListBox::Delete" );
+
+    wxListBox::Delete(N);
+
+    // free memory
+    delete m_aItems[N];
+
+    m_aItems.Remove(N);
+}
+
+void wxCheckListBox::InsertItems(int nItems, const wxString items[], int pos)
+{
+    wxCHECK_RET( pos >= 0 && pos <= m_noItems,
+                 "invalid index in wxCheckListBox::InsertItems" );
+
+    wxListBox::InsertItems(nItems, items, pos);
+
+    int i;
+    for ( i = 0; i < nItems; i++ ) {
+        wxOwnerDrawn *pNewItem = CreateItem((size_t)(pos + i));
+        pNewItem->SetName(items[i]);
+        m_aItems.Insert(pNewItem, (size_t)(pos + i));
+        ListBox_SetItemData((HWND)GetHWND(), i + pos, pNewItem);
+    }
 }
 
 // create/retrieve item
@@ -251,9 +306,6 @@ wxOwnerDrawn *wxCheckListBox::CreateItem(size_t nIndex)
   return pItem;
 }
 
-// get item (converted to right type)
-#define GetItem(n)    ((wxCheckListBoxItem *)(GetItem(n)))
-
 // return item size
 // ----------------
 bool wxCheckListBox::MSWOnMeasure(WXMEASUREITEMSTRUCT *item)
@@ -301,7 +353,7 @@ void wxCheckListBox::OnLeftClick(wxMouseEvent& event)
 {
   // clicking on the item selects it, clicking on the checkmark toggles
   if ( event.GetX() <= wxOwnerDrawn::GetDefaultMarginWidth() ) {
-    // # better use LB_ITEMFROMPOINT perhaps?
+    // FIXME better use LB_ITEMFROMPOINT perhaps?
     size_t nItem = ((size_t)event.GetY()) / m_nItemHeight;
     if ( nItem < (size_t)m_noItems )
       GetItem(nItem)->Toggle();

src/msw/listbox.cpp

@@ -10,22 +10,22 @@
 ///////////////////////////////////////////////////////////////////////////////
 
 #ifdef __GNUG__
-#pragma implementation "listbox.h"
+    #pragma implementation "listbox.h"
 #endif
 
 // For compilers that support precompilation, includes "wx.h".
 #include "wx/wxprec.h"
 
 #ifdef __BORLANDC__
-#pragma hdrstop
+    #pragma hdrstop
 #endif
 
 #ifndef WX_PRECOMP
-#include "wx/listbox.h"
+    #include "wx/listbox.h"
-#include "wx/settings.h"
+    #include "wx/settings.h"
-#include "wx/brush.h"
+    #include "wx/brush.h"
-#include "wx/font.h"
+    #include "wx/font.h"
-#include "wx/dc.h"
+    #include "wx/dc.h"
 #endif
 
 #include "wx/msw/private.h"
@@ -34,13 +34,13 @@
 #include <windowsx.h>
 
 #ifndef __TWIN32__
-#ifdef __GNUWIN32__
+    #ifdef __GNUWIN32__
-#include <wx/msw/gnuwin32/extra.h>
+        #include <wx/msw/gnuwin32/extra.h>
-#endif
+    #endif
 #endif
 
 #ifdef GetCharWidth
-#undef GetCharWidth
+    #undef GetCharWidth
 #endif
 
 #if wxUSE_OWNER_DRAWN
@@ -88,12 +88,12 @@ wxOwnerDrawn *wxListBox::CreateItem(size_t n)
 
 bool wxListBox::MSWCommand(WXUINT param, WXWORD WXUNUSED(id))
 {
-/*
+    /*
        if (param == LBN_SELCANCEL)
        {
        event.extraLong = FALSE;
        }
-*/
+     */
     if (param == LBN_SELCHANGE)
     {
         wxCommandEvent event(wxEVT_COMMAND_LISTBOX_SELECTED, m_windowId);
@@ -125,28 +125,20 @@ bool wxListBox::MSWCommand(WXUINT param, WXWORD WXUNUSED(id))
         event.SetEventObject( this );
         GetEventHandler()->ProcessEvent(event) ;
         return TRUE;
-/*
-    {
-#if WXWIN_COMPATIBILITY
-      wxWindow *parent = (wxWindow *)GetParent();
-      if (parent)
-        parent->GetEventHandler()->OnDefaultAction(this);
-#endif
-      return TRUE;
-    }
- */
     }
+
     return FALSE;
 }
 
-// Listbox item
+    // Listbox item
 wxListBox::wxListBox()
 {
     m_noItems = 0;
     m_selected = 0;
 }
 
-bool wxListBox::Create(wxWindow *parent, wxWindowID id,
+bool wxListBox::Create(wxWindow *parent,
+                       wxWindowID id,
                        const wxPoint& pos,
                        const wxSize& size,
                        int n, const wxString choices[],
@@ -161,7 +153,8 @@ bool wxListBox::Create(wxWindow *parent, wxWindowID id,
     SetName(name);
     SetValidator(validator);
 
-  if (parent) parent->AddChild(this);
+    if (parent)
+        parent->AddChild(this);
 
     wxSystemSettings settings;
     SetBackgroundColour(settings.GetSystemColour(wxSYS_COLOUR_WINDOW));
@@ -194,6 +187,7 @@ bool wxListBox::Create(wxWindow *parent, wxWindowID id,
         wstyle |= LBS_OWNERDRAWFIXED;
     }
 #endif
+
     // Without this style, you get unexpected heights, so e.g. constraint layout
     // doesn't work properly
     wstyle |= LBS_NOINTEGRALHEIGHT;
@@ -232,20 +226,6 @@ bool wxListBox::Create(wxWindow *parent, wxWindowID id,
         Append(choices[ui]);
     }
 
-  /* Not needed -- done in Append
-#if wxUSE_OWNER_DRAWN
-    if ( m_windowStyle & wxLB_OWNERDRAW ) {
-      for (ui = 0; ui < (size_t)n; ui++) {
-        // create new item which will process WM_{DRAW|MEASURE}ITEM messages
-        wxOwnerDrawn *pNewItem = CreateItem(ui);
-        pNewItem->SetName(choices[ui]);
-        m_aItems.Add(pNewItem);
-        ListBox_SetItemData(hwnd, ui, pNewItem);
-      }
-    }
-#endif
-*/
-
     if ( (m_windowStyle & wxLB_MULTIPLE) == 0 )
         SendMessage(hwnd, LB_SETCURSEL, 0, 0);
 
@@ -265,7 +245,7 @@ wxListBox::~wxListBox()
     while ( uiCount-- != 0 ) {
         delete m_aItems[uiCount];
     }
-#endif
+#endif // wxUSE_OWNER_DRAWN
 }
 
 void wxListBox::SetupColours()
@@ -276,6 +256,9 @@ void wxListBox::SetupColours()
 
 void wxListBox::SetFirstItem(int N)
 {
+    wxCHECK_RET( N >= 0 && N < m_noItems,
+                 "invalid index in wxListBox::SetFirstItem" );
+
     SendMessage(hwnd,LB_SETTOPINDEX,(WPARAM)N,(LPARAM)0) ;
 }
 
@@ -283,14 +266,18 @@ void wxListBox::SetFirstItem(const wxString& s)
 {
     int N = FindString(s) ;
 
-  if (N>=0)
+    if ( N >= 0 )
         SetFirstItem(N) ;
 }
 
 void wxListBox::Delete(int N)
 {
+    wxCHECK_RET( N >= 0 && N < m_noItems,
+                 "invalid index in wxListBox::Delete" );
+
     SendMessage(hwnd, LB_DELETESTRING, N, 0);
-  m_noItems --;
+    m_noItems--;
+
     SetHorizontalExtent("");
 }
 
@@ -324,6 +311,7 @@ void wxListBox::Append(const wxString& item, char *Client_data)
     }
     else
 #endif
+
     ListBox_SetItemData(hwnd, index, Client_data);
 
     SetHorizontalExtent(item);
@@ -381,12 +369,24 @@ void wxListBox::Clear()
 {
     ListBox_ResetContent(hwnd);
 
+#if wxUSE_OWNER_DRAWN
+    size_t uiCount = m_aItems.Count();
+    while ( uiCount-- != 0 ) {
+        delete m_aItems[uiCount];
+    }
+
+    m_aItems.Clear();
+#endif // wxUSE_OWNER_DRAWN
+
     m_noItems = 0;
     ListBox_GetHorizontalExtent(hwnd);
 }
 
 void wxListBox::SetSelection(int N, bool select)
 {
+    wxCHECK_RET( N >= 0 && N < m_noItems,
+                 "invalid index in wxListBox::SetSelection" );
+
     if ((m_windowStyle & wxLB_MULTIPLE) || (m_windowStyle & wxLB_EXTENDED))
         SendMessage(hwnd, LB_SETSEL, select, N);
     else
@@ -400,22 +400,34 @@ void wxListBox::SetSelection(int N, bool select)
 
 bool wxListBox::Selected(int N) const
 {
+    wxCHECK_MSG( N >= 0 && N < m_noItems, FALSE,
+                 "invalid index in wxListBox::Selected" );
+
     return SendMessage(hwnd, LB_GETSEL, N, 0) == 0 ? FALSE : TRUE;
 }
 
 void wxListBox::Deselect(int N)
 {
+    wxCHECK_RET( N >= 0 && N < m_noItems,
+                 "invalid index in wxListBox::Deselect" );
+
     if ((m_windowStyle & wxLB_MULTIPLE) || (m_windowStyle & wxLB_EXTENDED))
         SendMessage(hwnd, LB_SETSEL, FALSE, N);
 }
 
 char *wxListBox::GetClientData(int N) const
 {
+    wxCHECK_MSG( N >= 0 && N < m_noItems, NULL,
+                 "invalid index in wxListBox::GetClientData" );
+
     return (char *)SendMessage(hwnd, LB_GETITEMDATA, N, 0);
 }
 
 void wxListBox::SetClientData(int N, char *Client_data)
 {
+    wxCHECK_RET( N >= 0 && N < m_noItems,
+                 "invalid index in wxListBox::SetClientData" );
+
     if ( ListBox_SetItemData(hwnd, N, Client_data) == LB_ERR )
         wxLogDebug("LB_SETITEMDATA failed");
 }
@@ -466,12 +478,17 @@ int wxListBox::GetSelection() const
 // Find string for position
 wxString wxListBox::GetString(int N) const
 {
-  if (N < 0 || N > m_noItems)
+    wxCHECK_MSG( N >= 0 && N < m_noItems, "",
-    return wxString("");
+                 "invalid index in wxListBox::GetClientData" );
 
-  int len = (int)SendMessage(hwnd, LB_GETTEXT, N, (LONG)wxBuffer);
+    int len = ListBox_GetTextLen(hwnd, N);
-  wxBuffer[len] = 0;
+
-  return wxString(wxBuffer);
+    // +1 for terminating NUL
+    wxString result;
+    ListBox_GetText(hwnd, N, result.GetWriteBuf(len + 1));
+    result.UngetWriteBuf();
+
+    return result;
 }
 
 void wxListBox::DoSetSize(int x, int y, int width, int height, int sizeFlags)
@@ -523,9 +540,10 @@ void wxListBox::DoSetSize(int x, int y, int width, int height, int sizeFlags)
     if (control_width <= 0)
         control_width = (float)DEFAULT_ITEM_WIDTH;
 
-//  wxDebugMsg("About to set the listbox height to %d", (int)control_height);
+    MoveWindow(hwnd,
-  MoveWindow(hwnd, (int)control_x, (int)control_y,
+               (int)control_x, (int)control_y,
-                   (int)control_width, (int)control_height, TRUE);
+               (int)control_width, (int)control_height,
+               TRUE);
 
 }
 
@@ -592,27 +610,22 @@ void wxListBox::SetHorizontalExtent(const wxString& s)
 void
 wxListBox::InsertItems(int nItems, const wxString items[], int pos)
 {
+    wxCHECK_RET( pos >= 0 && pos <= m_noItems,
+                 "invalid index in wxListBox::InsertItems" );
+
     int i;
     for (i = 0; i < nItems; i++)
         ListBox_InsertString(hwnd, i + pos, items[i]);
     m_noItems += nItems;
 
-  #if wxUSE_OWNER_DRAWN
-    if ( m_windowStyle & wxLB_OWNERDRAW ) {
-      for ( i = 0; i < nItems; i++ ) {
-        wxOwnerDrawn *pNewItem = CreateItem((size_t)(pos + i));
-        pNewItem->SetName(items[i]);
-        m_aItems.Insert(pNewItem, (size_t)(pos + i));
-        ListBox_SetItemData(hwnd, i, pNewItem);
-      }
-    }
-  #endif
-
     SetHorizontalExtent("");
 }
 
 void wxListBox::SetString(int N, const wxString& s)
 {
+    wxCHECK_RET( N >= 0 && N < m_noItems,
+                 "invalid index in wxListBox::SetString" );
+
     int sel = -1;
     if (!(m_windowStyle & wxLB_MULTIPLE) && !(m_windowStyle & wxLB_EXTENDED))
         sel = GetSelection();
@@ -714,58 +727,6 @@ WXHBRUSH wxListBox::OnCtlColor(WXHDC pDC, WXHWND pWnd, WXUINT nCtlColor,
 
 long wxListBox::MSWWindowProc(WXUINT nMsg, WXWPARAM wParam, WXLPARAM lParam)
 {
-#if 0
-  switch (nMsg)
-  {
-        case WM_INITDIALOG:
-        case WM_ACTIVATE:
-        case WM_SETFOCUS:
-        case WM_KILLFOCUS:
-	case WM_CREATE:
-	case WM_PAINT:
-	case WM_QUERYDRAGICON:
-        case WM_SIZE:
-        case WM_RBUTTONDOWN:
-        case WM_RBUTTONUP:
-        case WM_RBUTTONDBLCLK:
-        case WM_MBUTTONDOWN:
-        case WM_MBUTTONUP:
-        case WM_MBUTTONDBLCLK:
-        case WM_LBUTTONDOWN:
-        case WM_LBUTTONUP:
-        case WM_LBUTTONDBLCLK:
-        case WM_MOUSEMOVE:
-        case WM_COMMAND:
-        case WM_NOTIFY:
-        case WM_DESTROY:
-        case WM_MENUSELECT:
-        case WM_INITMENUPOPUP:
-        case WM_DRAWITEM:
-        case WM_MEASUREITEM:
-        case WM_KEYDOWN:
-        case WM_KEYUP:
-        case WM_CHAR: // Always an ASCII character
-        case WM_HSCROLL:
-        case WM_VSCROLL:
-        case WM_CTLCOLORBTN:
-        case WM_CTLCOLORDLG:
-        case WM_CTLCOLORLISTBOX:
-        case WM_CTLCOLORMSGBOX:
-        case WM_CTLCOLORSCROLLBAR:
-        case WM_CTLCOLORSTATIC:
-        case WM_CTLCOLOREDIT:
-        case WM_SYSCOLORCHANGE:
-        case WM_ERASEBKGND:
-        case WM_MDIACTIVATE:
-        case WM_DROPFILES:
-        case WM_QUERYENDSESSION:
-        case WM_CLOSE:
-        case WM_GETMINMAXINFO:
-		case WM_NCHITTEST:
-            return MSWDefWindowProc(nMsg, wParam, lParam );
-    }
-#endif
-
     return wxControl::MSWWindowProc(nMsg, wParam, lParam);
 }
 
@@ -779,7 +740,8 @@ long wxListBox::MSWWindowProc(WXUINT nMsg, WXWPARAM wParam, WXLPARAM lParam)
 #define OWNER_DRAWN_LISTBOX_EXTRA_SPACE    (1)
 
 // the height is the same for all items
-// ## should be changed for LBS_OWNERDRAWVARIABLE style listboxes
+// TODO should be changed for LBS_OWNERDRAWVARIABLE style listboxes
+
 // NB: can't forward this to wxListBoxItem because LB_SETITEMDATA
 //     message is not yet sent when we get here!
 bool wxListBox::MSWOnMeasure(WXMEASUREITEMSTRUCT *item)
@@ -806,16 +768,17 @@ bool wxListBox::MSWOnDraw(WXDRAWITEMSTRUCT *item)
     wxCHECK( ((m_windowStyle & wxLB_OWNERDRAW) == wxLB_OWNERDRAW), FALSE );
 
     DRAWITEMSTRUCT *pStruct = (DRAWITEMSTRUCT *)item;
-  wxListBoxItem *pItem = (wxListBoxItem *)SendMessage(hwnd, LB_GETITEMDATA, 
-                                                      pStruct->itemID, 0);
 
-  wxCHECK( (int)pItem != LB_ERR, FALSE );
+    long data = ListBox_GetItemData(hwnd, pStruct->itemID);
+
+    wxCHECK( data && (data != LB_ERR), FALSE );
+
+    wxListBoxItem *pItem = (wxListBoxItem *)data;
 
     wxDC dc;
     dc.SetHDC((WXHDC)pStruct->hDC, FALSE);
-  wxRect rect(pStruct->rcItem.left, pStruct->rcItem.top,
+    wxRect rect(wxPoint(pStruct->rcItem.left, pStruct->rcItem.top),
-              pStruct->rcItem.right - pStruct->rcItem.left, 
+            wxPoint(pStruct->rcItem.right, pStruct->rcItem.bottom));
-              pStruct->rcItem.bottom - pStruct->rcItem.top);
 
     return pItem->OnDrawItem(dc, rect,
             (wxOwnerDrawn::wxODAction)pStruct->itemAction,