From b6071fc6c65efd2922e54c8dd916432739c12988 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Wed, 18 Jan 2023 23:42:56 +0000 Subject: [PATCH 1/8] Use margins around the text in wxGenericListCtrl on all platforms For some reason, the margin was only used in wxGTK ever since it was added back in ccdbdc8936 (Added native selection rectangle drawing., 2006-11-11) even though it came with a comment saying that this should be done for all platforms. Finally do this now to make the list look better under e.g. macOS. Closes #23155. --- src/generic/listctrl.cpp | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-) diff --git a/src/generic/listctrl.cpp b/src/generic/listctrl.cpp index 62a2a33df6..987c2f141e 100644 --- a/src/generic/listctrl.cpp +++ b/src/generic/listctrl.cpp @@ -63,30 +63,14 @@ static const int SCROLL_UNIT_X = 15; static const int LINE_SPACING = 0; // extra margins around the text label -#ifdef __WXGTK__ static const int EXTRA_WIDTH = 6; -#else -static const int EXTRA_WIDTH = 4; -#endif - -#ifdef __WXGTK__ static const int EXTRA_HEIGHT = 6; -#else -static const int EXTRA_HEIGHT = 4; -#endif // margin between the window and the items static const int EXTRA_BORDER_X = 2; static const int EXTRA_BORDER_Y = 2; -#ifdef __WXGTK__ - // This probably needs to be done - // on all platforms as the icons - // otherwise nearly touch the border - static const int ICON_OFFSET_X = 2; -#else - static const int ICON_OFFSET_X = 0; -#endif +static const int ICON_OFFSET_X = 2; // offset for the header window static const int HEADER_OFFSET_X = 0; From b7e7d506c5ecdce90ebd2e59f4a4e586084e6e80 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Wed, 18 Jan 2023 23:56:55 +0000 Subject: [PATCH 2/8] Use std::vector instead of wx array in wxGenericListCtrl code This not only simplifies the code but also makes it more efficient by avoiding unnecessary heap allocations, as we can now just store the objects themselves in the vector instead of storing pointers to them. No real changes, this is just a code cleanup/modernization. --- include/wx/generic/private/listctrl.h | 2 +- src/generic/listctrl.cpp | 50 +++++++++++++-------------- 2 files changed, 25 insertions(+), 27 deletions(-) diff --git a/include/wx/generic/private/listctrl.h b/include/wx/generic/private/listctrl.h index d10561372b..a0e3fb48c7 100644 --- a/include/wx/generic/private/listctrl.h +++ b/include/wx/generic/private/listctrl.h @@ -40,7 +40,7 @@ struct wxColWidthInfo } }; -WX_DEFINE_ARRAY_PTR(wxColWidthInfo *, ColWidthArray); +using ColWidthArray = std::vector; //----------------------------------------------------------------------------- // wxListItemData (internal) diff --git a/src/generic/listctrl.cpp b/src/generic/listctrl.cpp index 987c2f141e..5641b8382f 100644 --- a/src/generic/listctrl.cpp +++ b/src/generic/listctrl.cpp @@ -1652,7 +1652,6 @@ wxListMainWindow::~wxListMainWindow() DoDeleteAllItems(); WX_CLEAR_LIST(wxListHeaderDataList, m_columns); - WX_CLEAR_ARRAY(m_aColWidths); delete m_highlightBrush; delete m_highlightUnfocusedBrush; @@ -3474,20 +3473,20 @@ void wxListMainWindow::SetColumnWidth( int col, int width ) calculator.UpdateWithWidth(ComputeMinHeaderWidth(column)); // if the cached column width isn't valid then recalculate it - wxColWidthInfo* const pWidthInfo = m_aColWidths.Item(col); - if ( pWidthInfo->bNeedsUpdate ) + wxColWidthInfo& widthInfo = m_aColWidths[col]; + if ( widthInfo.bNeedsUpdate ) { size_t first_visible, last_visible; GetVisibleLinesRange(&first_visible, &last_visible); calculator.ComputeBestColumnWidth(GetItemCount(), first_visible, last_visible); - pWidthInfo->nMaxWidth = calculator.GetMaxWidth(); - pWidthInfo->bNeedsUpdate = false; + widthInfo.nMaxWidth = calculator.GetMaxWidth(); + widthInfo.bNeedsUpdate = false; } else { - calculator.UpdateWithWidth(pWidthInfo->nMaxWidth); + calculator.UpdateWithWidth(widthInfo.nMaxWidth); } width = calculator.GetMaxWidth() + AUTOSIZE_COL_MARGIN; @@ -3576,11 +3575,11 @@ void wxListMainWindow::SetItem( wxListItem &item ) // update the Max Width Cache if needed int width = GetItemWidthWithImage(&item); - wxColWidthInfo* const pWidthInfo = m_aColWidths.Item(item.m_col); - if ( width > pWidthInfo->nMaxWidth ) + wxColWidthInfo& widthInfo = m_aColWidths.at(item.m_col); + if ( width > widthInfo.nMaxWidth ) { - pWidthInfo->nMaxWidth = width; - pWidthInfo->bNeedsUpdate = true; + widthInfo.nMaxWidth = width; + widthInfo.bNeedsUpdate = true; } } } @@ -4313,9 +4312,9 @@ void wxListMainWindow::DeleteItem( long lindex ) int itemWidth; itemWidth = GetItemWidthWithImage(&item); - wxColWidthInfo *pWidthInfo = m_aColWidths.Item(i); - if ( itemWidth >= pWidthInfo->nMaxWidth ) - pWidthInfo->bNeedsUpdate = true; + wxColWidthInfo& widthInfo = m_aColWidths[i]; + if ( itemWidth >= widthInfo.nMaxWidth ) + widthInfo.bNeedsUpdate = true; } ResetVisibleLinesRange(); @@ -4386,8 +4385,7 @@ void wxListMainWindow::DeleteColumn( int col ) if ( InReportView() ) // we only cache max widths when in Report View { - delete m_aColWidths.Item(col); - m_aColWidths.RemoveAt(col); + m_aColWidths.erase(m_aColWidths.begin() + col); } // invalidate it as it has to be recalculated @@ -4399,8 +4397,8 @@ void wxListMainWindow::DoDeleteAllItems() // We will need to update all columns if any items are inserted again. if ( InReportView() ) { - for ( size_t i = 0; i < m_aColWidths.GetCount(); i++ ) - m_aColWidths.Item(i)->bNeedsUpdate = true; + for ( auto& widthInfo : m_aColWidths ) + widthInfo.bNeedsUpdate = true; } if ( IsEmpty() ) @@ -4444,7 +4442,7 @@ void wxListMainWindow::DeleteAllItems() void wxListMainWindow::DeleteEverything() { WX_CLEAR_LIST(wxListHeaderDataList, m_columns); - WX_CLEAR_ARRAY(m_aColWidths); + m_aColWidths.clear(); DeleteAllItems(); } @@ -4589,13 +4587,13 @@ void wxListMainWindow::InsertItem( wxListItem &item ) wxCHECK_RET( col < m_aColWidths.size(), "invalid item column" ); // calculate the width of the item and adjust the max column width - wxColWidthInfo *pWidthInfo = m_aColWidths.Item(col); + wxColWidthInfo& widthInfo = m_aColWidths[col]; int width = GetItemWidthWithImage(&item); item.SetWidth(width); - if (width > pWidthInfo->nMaxWidth) + if (width > widthInfo.nMaxWidth) { - pWidthInfo->nMaxWidth = width; - pWidthInfo->bNeedsUpdate = true; + widthInfo.nMaxWidth = width; + widthInfo.bNeedsUpdate = true; } } @@ -4642,7 +4640,7 @@ long wxListMainWindow::InsertColumn( long col, const wxListItem &item ) if (item.m_width == wxLIST_AUTOSIZE_USEHEADER) column->SetWidth(ComputeMinHeaderWidth(column)); - wxColWidthInfo *colWidthInfo = new wxColWidthInfo(0, IsVirtual()); + wxColWidthInfo colWidthInfo(0, IsVirtual()); bool insert = (col >= 0) && ((size_t)col < m_columns.GetCount()); if ( insert ) @@ -4650,14 +4648,14 @@ long wxListMainWindow::InsertColumn( long col, const wxListItem &item ) wxListHeaderDataList::compatibility_iterator node = m_columns.Item( col ); m_columns.Insert( node, column ); - m_aColWidths.Insert( colWidthInfo, col ); + m_aColWidths.insert( m_aColWidths.begin() + col, colWidthInfo ); idx = col; } else { - idx = m_aColWidths.GetCount(); + idx = m_aColWidths.size(); m_columns.Append( column ); - m_aColWidths.Add( colWidthInfo ); + m_aColWidths.push_back( colWidthInfo ); } if ( !IsVirtual() ) From 53188d30f05334b786f5bf182f6fe536de0a380f Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Thu, 19 Jan 2023 00:27:24 +0000 Subject: [PATCH 3/8] Make wxListHeaderData::GetItem() const Because there is no reason for it not to be it. --- include/wx/generic/private/listctrl.h | 2 +- src/generic/listctrl.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/wx/generic/private/listctrl.h b/include/wx/generic/private/listctrl.h index a0e3fb48c7..d34c5132d7 100644 --- a/include/wx/generic/private/listctrl.h +++ b/include/wx/generic/private/listctrl.h @@ -133,7 +133,7 @@ public: const wxString& GetText() const { return m_text; } void SetText(const wxString& text) { m_text = text; } - void GetItem( wxListItem &item ); + void GetItem( wxListItem &item ) const; bool IsHit( int x, int y ) const; int GetImage() const; diff --git a/src/generic/listctrl.cpp b/src/generic/listctrl.cpp index 5641b8382f..5f32975b11 100644 --- a/src/generic/listctrl.cpp +++ b/src/generic/listctrl.cpp @@ -347,7 +347,7 @@ bool wxListHeaderData::IsHit( int x, int y ) const return ((x >= m_xpos) && (x <= m_xpos+m_width) && (y >= m_ypos) && (y <= m_ypos+m_height)); } -void wxListHeaderData::GetItem( wxListItem& item ) +void wxListHeaderData::GetItem( wxListItem& item ) const { long mask = item.m_mask; if ( !mask ) From f547bf0647293233fcd209cbb9e6c8119e0c6f6b Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Thu, 19 Jan 2023 02:04:47 +0100 Subject: [PATCH 4/8] Replace wxListItemData::Init() with member initialization Get rid of Init() function and just initialize all the members in their declarations, as this is less verbose, more clear and more robust. Note that we also initialize m_rect now, which was left uninitialized by Init() before, as it's simpler and more reliable to always do even if we happen to overwrite it in the ctor. --- include/wx/generic/private/listctrl.h | 11 ++++------- src/generic/listctrl.cpp | 14 +------------- 2 files changed, 5 insertions(+), 20 deletions(-) diff --git a/include/wx/generic/private/listctrl.h b/include/wx/generic/private/listctrl.h index d34c5132d7..33a63ff775 100644 --- a/include/wx/generic/private/listctrl.h +++ b/include/wx/generic/private/listctrl.h @@ -90,25 +90,22 @@ public: public: // the item image or -1 - int m_image; + int m_image = -1; // user data associated with the item - wxUIntPtr m_data; + wxUIntPtr m_data = 0; // the item coordinates are not used in report mode; instead this pointer is // null and the owner window is used to retrieve the item position and size - wxRect *m_rect; + wxRect *m_rect = nullptr; // the list ctrl we are in wxListMainWindow *m_owner; // custom attributes or nullptr - wxItemAttr *m_attr; + wxItemAttr *m_attr = nullptr; protected: - // common part of all ctors - void Init(); - wxString m_text; }; diff --git a/src/generic/listctrl.cpp b/src/generic/listctrl.cpp index 5f32975b11..efe39e71ba 100644 --- a/src/generic/listctrl.cpp +++ b/src/generic/listctrl.cpp @@ -117,23 +117,11 @@ wxListItemData::~wxListItemData() delete m_rect; } -void wxListItemData::Init() -{ - m_image = -1; - m_data = 0; - - m_attr = nullptr; -} - wxListItemData::wxListItemData(wxListMainWindow *owner) { - Init(); - m_owner = owner; - if ( owner->InReportView() ) - m_rect = nullptr; - else + if ( !owner->InReportView() ) m_rect = new wxRect; } From a5777cdcbd820aba2a1b736c622fea3209c20a1d Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Thu, 19 Jan 2023 02:06:32 +0100 Subject: [PATCH 5/8] Make wxListItemData movable and not copyable This class contains (sometimes) owned pointers and so can't be copied but it can be moved, so make it moveable. No real changes, this is done in preparation for storing objects of this type in a std::vector. --- include/wx/generic/private/listctrl.h | 4 ++++ src/generic/listctrl.cpp | 22 ++++++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/include/wx/generic/private/listctrl.h b/include/wx/generic/private/listctrl.h index 33a63ff775..52679bc78f 100644 --- a/include/wx/generic/private/listctrl.h +++ b/include/wx/generic/private/listctrl.h @@ -50,6 +50,10 @@ class wxListItemData { public: wxListItemData(wxListMainWindow *owner); + wxListItemData(const wxListItemData&) = delete; + wxListItemData(wxListItemData&&); + wxListItemData& operator=(const wxListItemData&) = delete; + wxListItemData& operator=(wxListItemData&&); ~wxListItemData(); void SetItem( const wxListItem &info ); diff --git a/src/generic/listctrl.cpp b/src/generic/listctrl.cpp index efe39e71ba..f441dbd3d4 100644 --- a/src/generic/listctrl.cpp +++ b/src/generic/listctrl.cpp @@ -107,6 +107,28 @@ WX_DEFINE_LIST(wxListHeaderDataList) // wxListItemData // ---------------------------------------------------------------------------- +wxListItemData::wxListItemData(wxListItemData&& other) +{ + m_owner = other.m_owner; + + // Take ownership of the pointers from the other object and reset them. + std::swap(m_attr, other.m_attr); + std::swap(m_rect, other.m_rect); +} + +wxListItemData& wxListItemData::operator=(wxListItemData&& other) +{ + m_image = other.m_image; + m_data = other.m_data; + m_owner = other.m_owner; + + // Swap them to let our pointers be deleted by the other object if necessary. + std::swap(m_attr, other.m_attr); + std::swap(m_rect, other.m_rect); + + return *this; +} + wxListItemData::~wxListItemData() { // in the virtual list control the attributes are managed by the main From f62047f86743f31ae439808b82b104d0e3a998d4 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Thu, 19 Jan 2023 00:27:43 +0000 Subject: [PATCH 6/8] Replace wxList classes used in wxGenericListCtrl with vectors This simplifies the code and avoids unnecessary heap allocations as we can store the objects in the vectors directly instead of storing pointers to them. --- include/wx/generic/private/listctrl.h | 11 +- src/generic/listctrl.cpp | 194 ++++++++------------------ 2 files changed, 62 insertions(+), 143 deletions(-) diff --git a/include/wx/generic/private/listctrl.h b/include/wx/generic/private/listctrl.h index 52679bc78f..1f987e4351 100644 --- a/include/wx/generic/private/listctrl.h +++ b/include/wx/generic/private/listctrl.h @@ -161,13 +161,11 @@ private: // wxListLineData (internal) //----------------------------------------------------------------------------- -WX_DECLARE_LIST(wxListItemData, wxListItemDataList); - class wxListLineData { public: // the list of subitems: only may have more than one item in report mode - wxListItemDataList m_items; + std::vector m_items; // this is not used in report view struct GeometryInfo @@ -211,7 +209,6 @@ public: ~wxListLineData() { - WX_CLEAR_LIST(wxListItemDataList, m_items); delete m_gi; } @@ -487,8 +484,6 @@ private: // wxListMainWindow (internal) //----------------------------------------------------------------------------- -WX_DECLARE_LIST(wxListHeaderData, wxListHeaderDataList); - class wxListMainWindow : public wxWindow { public: @@ -649,7 +644,7 @@ public: void SetColumnWidth( int col, int width ); void GetColumn( int col, wxListItem &item ) const; int GetColumnWidth( int col ) const; - int GetColumnCount() const { return m_columns.GetCount(); } + int GetColumnCount() const { return m_columns.size(); } // returns the sum of the heights of all columns int GetHeaderWidth() const; @@ -796,7 +791,7 @@ protected: wxListLineDataArray m_lines; // the list of column objects - wxListHeaderDataList m_columns; + std::vector m_columns; // currently focused item or -1 size_t m_current; diff --git a/src/generic/listctrl.cpp b/src/generic/listctrl.cpp index f441dbd3d4..60039999dd 100644 --- a/src/generic/listctrl.cpp +++ b/src/generic/listctrl.cpp @@ -91,18 +91,6 @@ static const int HEADER_IMAGE_MARGIN_IN_REPORT_MODE = 2; // space after a checkbox static const int MARGIN_AROUND_CHECKBOX = 5; - -// ---------------------------------------------------------------------------- -// arrays/list implementations -// ---------------------------------------------------------------------------- - -#include "wx/listimpl.cpp" -WX_DEFINE_LIST(wxListItemDataList) - -#include "wx/listimpl.cpp" -WX_DEFINE_LIST(wxListHeaderDataList) - - // ---------------------------------------------------------------------------- // wxListItemData // ---------------------------------------------------------------------------- @@ -434,10 +422,9 @@ wxListLineData::wxListLineData( wxListMainWindow *owner ) void wxListLineData::CalculateSize( wxDC *dc, int spacing ) { - wxListItemDataList::compatibility_iterator node = m_items.GetFirst(); - wxCHECK_RET( node, wxT("no subitems at all??") ); + wxCHECK_RET( !m_items.empty(), wxT("no subitems at all??") ); - wxListItemData *item = node->GetData(); + wxListItemData* const item = &m_items[0]; wxString s; wxCoord lw, lh; @@ -536,10 +523,9 @@ void wxListLineData::CalculateSize( wxDC *dc, int spacing ) void wxListLineData::SetPosition( int x, int y, int spacing ) { - wxListItemDataList::compatibility_iterator node = m_items.GetFirst(); - wxCHECK_RET( node, wxT("no subitems at all??") ); + wxCHECK_RET( !m_items.empty(), wxT("no subitems at all??") ); - wxListItemData *item = node->GetData(); + wxListItemData* const item = &m_items[0]; switch ( GetMode() ) { @@ -604,86 +590,47 @@ void wxListLineData::SetPosition( int x, int y, int spacing ) void wxListLineData::InitItems( int num ) { for (int i = 0; i < num; i++) - m_items.Append( new wxListItemData(m_owner) ); + m_items.emplace_back( m_owner ); } void wxListLineData::SetItem( int index, const wxListItem &info ) { - wxListItemDataList::compatibility_iterator node = m_items.Item( index ); - wxCHECK_RET( node, wxT("invalid column index in SetItem") ); - - wxListItemData *item = node->GetData(); - item->SetItem( info ); + m_items.at(index).SetItem( info ); } void wxListLineData::GetItem( int index, wxListItem &info ) const { - wxListItemDataList::compatibility_iterator node = m_items.Item( index ); - if (node) - { - wxListItemData *item = node->GetData(); - item->GetItem( info ); - } + m_items.at(index).GetItem( info ); } wxString wxListLineData::GetText(int index) const { - wxString s; - - wxListItemDataList::compatibility_iterator node = m_items.Item( index ); - if (node) - { - wxListItemData *item = node->GetData(); - s = item->GetText(); - } - - return s; + return m_items.at(index).GetText(); } void wxListLineData::SetText( int index, const wxString& s ) { - wxListItemDataList::compatibility_iterator node = m_items.Item( index ); - if (node) - { - wxListItemData *item = node->GetData(); - item->SetText( s ); - } + m_items.at(index).SetText(s); } void wxListLineData::SetImage( int index, int image ) { - wxListItemDataList::compatibility_iterator node = m_items.Item( index ); - wxCHECK_RET( node, wxT("invalid column index in SetImage()") ); - - wxListItemData *item = node->GetData(); - item->SetImage(image); + m_items.at(index).SetImage(image); } int wxListLineData::GetImage( int index ) const { - wxListItemDataList::compatibility_iterator node = m_items.Item( index ); - wxCHECK_MSG( node, -1, wxT("invalid column index in GetImage()") ); - - wxListItemData *item = node->GetData(); - return item->GetImage(); + return m_items.at(index).GetImage(); } wxItemAttr *wxListLineData::GetAttr() const { - wxListItemDataList::compatibility_iterator node = m_items.GetFirst(); - wxCHECK_MSG( node, nullptr, wxT("invalid column index in GetAttr()") ); - - wxListItemData *item = node->GetData(); - return item->GetAttr(); + return m_items.at(0).GetAttr(); } void wxListLineData::SetAttr(wxItemAttr *attr) { - wxListItemDataList::compatibility_iterator node = m_items.GetFirst(); - wxCHECK_RET( node, wxT("invalid column index in SetAttr()") ); - - wxListItemData *item = node->GetData(); - item->SetAttr(attr); + m_items.at(0).SetAttr(attr); } void wxListLineData::ApplyAttributes(wxDC *dc, @@ -766,12 +713,11 @@ void wxListLineData::ApplyAttributes(wxDC *dc, void wxListLineData::Draw(wxDC *dc, bool current) { - wxListItemDataList::compatibility_iterator node = m_items.GetFirst(); - wxCHECK_RET( node, wxT("no subitems at all??") ); + wxCHECK_RET( !m_items.empty(), wxT("no subitems at all??") ); ApplyAttributes(dc, m_gi->m_rectHighlight, IsHighlighted(), current); - wxListItemData *item = node->GetData(); + wxListItemData* const item = &m_items[0]; if (item->HasImage()) { // centre the image inside our rectangle, this looks nicer when items @@ -823,12 +769,8 @@ void wxListLineData::DrawInReportMode( wxDC *dc, } size_t col = 0; - for ( wxListItemDataList::compatibility_iterator node = m_items.GetFirst(); - node; - node = node->GetNext(), col++ ) + for ( const auto& item : m_items ) { - wxListItemData *item = node->GetData(); - int width = m_owner->GetColumnWidth(col); if (col == 0 && m_owner->HasCheckBoxes()) width -= x; @@ -839,11 +781,11 @@ void wxListLineData::DrawInReportMode( wxDC *dc, const int wText = width; wxDCClipper clipper(*dc, xOld, rect.y, wText, rect.height); - if ( item->HasImage() ) + if ( item.HasImage() ) { int ix, iy; - m_owner->GetImageSize( item->GetImage(), ix, iy ); - m_owner->DrawImage( item->GetImage(), dc, xOld, yMid - iy/2 ); + m_owner->GetImageSize( item.GetImage(), ix, iy ); + m_owner->DrawImage( item.GetImage(), dc, xOld, yMid - iy/2 ); ix += IMAGE_MARGIN_IN_REPORT_MODE; @@ -851,8 +793,10 @@ void wxListLineData::DrawInReportMode( wxDC *dc, width -= ix; } - if ( item->HasText() ) - DrawTextFormatted(dc, item->GetText(), col, xOld, yMid, width); + if ( item.HasText() ) + DrawTextFormatted(dc, item.GetText(), col, xOld, yMid, width); + + ++col; } } @@ -1661,7 +1605,6 @@ wxListMainWindow::~wxListMainWindow() m_textctrlWrapper->EndEdit(wxListTextCtrlWrapper::End_Destroy); DoDeleteAllItems(); - WX_CLEAR_LIST(wxListHeaderDataList, m_columns); delete m_highlightBrush; delete m_highlightUnfocusedBrush; @@ -1710,7 +1653,7 @@ wxListLineData *wxListMainWindow::GetDummyLine() const // control changed as it would have the incorrect number of fields // otherwise if ( !m_lines.empty() && - m_lines[0]->m_items.GetCount() != (size_t)GetColumnCount() ) + m_lines[0]->m_items.size() != (size_t)GetColumnCount() ) { self->m_lines.Clear(); } @@ -1787,10 +1730,9 @@ wxRect wxListMainWindow::GetLineLabelRect(size_t line) const int image_x = 0; wxListLineData *data = GetLine(line); - wxListItemDataList::compatibility_iterator node = data->m_items.GetFirst(); - if (node) + if ( !data->m_items.empty() ) { - wxListItemData *item = node->GetData(); + wxListItemData *item = &data->m_items[0]; if ( item->HasImage() ) { int ix, iy; @@ -3405,11 +3347,10 @@ wxListMainWindow::ComputeMinHeaderWidth(const wxListHeaderData* column) const void wxListMainWindow::SetColumn( int col, const wxListItem &item ) { - wxListHeaderDataList::compatibility_iterator node = m_columns.Item( col ); + wxCHECK_RET( col >= 0 && col < (int)m_columns.size(), + "invalid column index in SetColumn" ); - wxCHECK_RET( node, wxT("invalid column index in SetColumn") ); - - wxListHeaderData *column = node->GetData(); + wxListHeaderData* const column = &m_columns[col]; column->SetItem( item ); if ( item.m_width == wxLIST_AUTOSIZE_USEHEADER ) @@ -3437,14 +3378,9 @@ public: virtual void UpdateWithRow(int row) override { wxListLineData *line = m_listmain->GetLine( row ); - wxListItemDataList::compatibility_iterator n = line->m_items.Item( GetColumn() ); - - wxCHECK_RET( n, wxS("no subitem?") ); - - wxListItemData* const itemData = n->GetData(); wxListItem item; - itemData->GetItem(item); + line->m_items.at(GetColumn()).GetItem(item); UpdateWithWidth(m_listmain->GetItemWidthWithImage(&item)); } @@ -3468,10 +3404,10 @@ void wxListMainWindow::SetColumnWidth( int col, int width ) if ( headerWin ) headerWin->m_dirty = true; - wxListHeaderDataList::compatibility_iterator node = m_columns.Item( col ); - wxCHECK_RET( node, wxT("no column?") ); + wxCHECK_RET( col >= 0 && col < (int)m_columns.size(), + "invalid column index in SetColumnWidth" ); - wxListHeaderData *column = node->GetData(); + wxListHeaderData* const column = &m_columns[col]; if ( width == wxLIST_AUTOSIZE_USEHEADER || width == wxLIST_AUTOSIZE ) { @@ -3514,7 +3450,7 @@ void wxListMainWindow::SetColumnWidth( int col, int width ) { int margin = GetClientSize().GetX(); for ( int i = 0; i < col && margin > 0; ++i ) - margin -= m_columns.Item(i)->GetData()->GetWidth(); + margin -= m_columns.at(i).GetWidth(); if ( margin > width ) width = margin; @@ -3545,20 +3481,18 @@ int wxListMainWindow::GetHeaderWidth() const void wxListMainWindow::GetColumn( int col, wxListItem &item ) const { - wxListHeaderDataList::compatibility_iterator node = m_columns.Item( col ); - wxCHECK_RET( node, wxT("invalid column index in GetColumn") ); + wxCHECK_RET( col >= 0 && col < (int)m_columns.size(), + "invalid column index in GetColumn" ); - wxListHeaderData *column = node->GetData(); - column->GetItem( item ); + m_columns[col].GetItem( item ); } int wxListMainWindow::GetColumnWidth( int col ) const { - wxListHeaderDataList::compatibility_iterator node = m_columns.Item( col ); - wxCHECK_MSG( node, 0, wxT("invalid column index") ); + wxCHECK_MSG( col >= 0 && col < (int)m_columns.size(), 0, + "invalid column index in GetColumnWidth" ); - wxListHeaderData *column = node->GetData(); - return column->GetWidth(); + return m_columns[col].GetWidth(); } // ---------------------------------------------------------------------------- @@ -4309,20 +4243,17 @@ void wxListMainWindow::DeleteItem( long lindex ) // mark the Column Max Width cache as dirty if the items in the line // we're deleting contain the Max Column Width wxListLineData * const line = GetLine(index); - wxListItemDataList::compatibility_iterator n; wxListItem item; - for (size_t i = 0; i < m_columns.GetCount(); i++) + size_t i = 0; + for ( const auto& it : line->m_items ) { - n = line->m_items.Item( i ); - wxListItemData* itemData; - itemData = n->GetData(); - itemData->GetItem(item); + it.GetItem(item); int itemWidth; itemWidth = GetItemWidthWithImage(&item); - wxColWidthInfo& widthInfo = m_aColWidths[i]; + wxColWidthInfo& widthInfo = m_aColWidths.at(i++); if ( itemWidth >= widthInfo.nMaxWidth ) widthInfo.bNeedsUpdate = true; } @@ -4358,13 +4289,11 @@ void wxListMainWindow::DeleteItem( long lindex ) void wxListMainWindow::DeleteColumn( int col ) { - wxListHeaderDataList::compatibility_iterator node = m_columns.Item( col ); - - wxCHECK_RET( node, wxT("invalid column index in DeleteColumn()") ); + wxCHECK_RET( col >= 0 && col < (int)m_columns.size(), + "invalid column index in DeleteColumn()" ); m_dirty = true; - delete node->GetData(); - m_columns.Erase( node ); + m_columns.erase( m_columns.begin() + col ); if ( !IsVirtual() ) { @@ -4384,12 +4313,10 @@ void wxListMainWindow::DeleteColumn( int col ) // 6. Call DeleteColumn(). // So we need to check for this as otherwise we would simply crash // if this happens. - if ( line->m_items.GetCount() <= static_cast(col) ) + if ( line->m_items.size() <= static_cast(col) ) continue; - wxListItemDataList::compatibility_iterator n = line->m_items.Item( col ); - delete n->GetData(); - line->m_items.Erase(n); + line->m_items.erase(line->m_items.begin() + col); } } @@ -4451,7 +4378,7 @@ void wxListMainWindow::DeleteAllItems() void wxListMainWindow::DeleteEverything() { - WX_CLEAR_LIST(wxListHeaderDataList, m_columns); + m_columns.clear(); m_aColWidths.clear(); DeleteAllItems(); @@ -4646,25 +4573,23 @@ long wxListMainWindow::InsertColumn( long col, const wxListItem &item ) m_dirty = true; if ( InReportView() ) { - wxListHeaderData *column = new wxListHeaderData( item ); + wxListHeaderData column( item ); if (item.m_width == wxLIST_AUTOSIZE_USEHEADER) - column->SetWidth(ComputeMinHeaderWidth(column)); + column.SetWidth(ComputeMinHeaderWidth(&column)); wxColWidthInfo colWidthInfo(0, IsVirtual()); - bool insert = (col >= 0) && ((size_t)col < m_columns.GetCount()); + bool insert = (col >= 0) && ((size_t)col < m_columns.size()); if ( insert ) { - wxListHeaderDataList::compatibility_iterator - node = m_columns.Item( col ); - m_columns.Insert( node, column ); + m_columns.insert( m_columns.begin() + col, column ); m_aColWidths.insert( m_aColWidths.begin() + col, colWidthInfo ); idx = col; } else { idx = m_aColWidths.size(); - m_columns.Append( column ); + m_columns.push_back( column ); m_aColWidths.push_back( colWidthInfo ); } @@ -4673,12 +4598,11 @@ long wxListMainWindow::InsertColumn( long col, const wxListItem &item ) // update all the items for ( size_t i = 0; i < m_lines.size(); i++ ) { - wxListLineData * const line = GetLine(i); - wxListItemData * const data = new wxListItemData(this); + auto& items = GetLine(i)->m_items; if ( insert ) - line->m_items.Insert(col, data); + items.emplace(items.begin() + col, this); else - line->m_items.Append(data); + items.emplace_back(this); } } @@ -5466,7 +5390,7 @@ bool wxGenericListCtrl::DeleteAllItems() bool wxGenericListCtrl::DeleteAllColumns() { - size_t count = m_mainWin->m_columns.GetCount(); + size_t count = m_mainWin->m_columns.size(); for ( size_t n = 0; n < count; n++ ) DeleteColumn( 0 ); return true; From 93da9ff74ed6eabc33828b41fcea8b16550a201d Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Thu, 19 Jan 2023 02:08:04 +0100 Subject: [PATCH 7/8] Use std::unique_ptr<> in wxListLineData and make it movable Don't manage the pointer lifetime manually but use a smart pointer for it: this is simpler and allows to make this class default-movable. Also make it non-copyable as it never actually was (copying it would have resulted in a double free of the owned pointer). --- include/wx/generic/private/listctrl.h | 27 ++++++++++++--------------- src/generic/listctrl.cpp | 5 +---- 2 files changed, 13 insertions(+), 19 deletions(-) diff --git a/include/wx/generic/private/listctrl.h b/include/wx/generic/private/listctrl.h index 1f987e4351..e26b40a723 100644 --- a/include/wx/generic/private/listctrl.h +++ b/include/wx/generic/private/listctrl.h @@ -19,6 +19,8 @@ #include "wx/timer.h" #include "wx/settings.h" +#include + // ============================================================================ // private classes // ============================================================================ @@ -193,8 +195,9 @@ public: m_rectIcon.x += (w - m_rectIcon.width) / 2; m_rectHighlight.x += (w - m_rectHighlight.width) / 2; } - } - *m_gi; + }; + + std::unique_ptr m_gi; // is this item selected? [NB: not used in virtual mode] bool m_highlighted; @@ -206,25 +209,19 @@ public: public: wxListLineData(wxListMainWindow *owner); + wxListLineData(const wxListLineData&) = delete; + wxListLineData(wxListLineData&& other) = default; - ~wxListLineData() - { - delete m_gi; - } + wxListLineData& operator=(const wxListLineData&) = delete; + wxListLineData& operator=(wxListLineData&&) = default; + + ~wxListLineData() = default; // called by the owner when it toggles report view void SetReportView(bool inReportView) { // we only need m_gi when we're not in report view so update as needed - if ( inReportView ) - { - delete m_gi; - m_gi = nullptr; - } - else - { - m_gi = new GeometryInfo; - } + m_gi.reset( inReportView ? nullptr : new GeometryInfo ); } // are we in report mode? diff --git a/src/generic/listctrl.cpp b/src/generic/listctrl.cpp index 60039999dd..af3c245325 100644 --- a/src/generic/listctrl.cpp +++ b/src/generic/listctrl.cpp @@ -409,10 +409,7 @@ wxListLineData::wxListLineData( wxListMainWindow *owner ) { m_owner = owner; - if ( InReportView() ) - m_gi = nullptr; - else // !report - m_gi = new GeometryInfo; + SetReportView(InReportView()); m_highlighted = false; m_checked = false; From b6937799e67b33b728f7b76b6fae377d31d1305a Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Thu, 19 Jan 2023 00:34:36 +0000 Subject: [PATCH 8/8] Store wxListLineData objects, not pointers, in the vector This could be a noticeable optimization as it should be much more efficient to store many lines in a single block of memory instead of using pointers to them and it also simplifies the code as we don't need to delete the pointers any more. --- include/wx/generic/private/listctrl.h | 21 ++++--------- src/generic/listctrl.cpp | 43 ++++++++++++--------------- 2 files changed, 24 insertions(+), 40 deletions(-) diff --git a/include/wx/generic/private/listctrl.h b/include/wx/generic/private/listctrl.h index e26b40a723..3c490cb0ad 100644 --- a/include/wx/generic/private/listctrl.h +++ b/include/wx/generic/private/listctrl.h @@ -308,19 +308,6 @@ private: int width); }; -class wxListLineDataArray : public wxVector -{ -public: - void Clear() - { - for ( size_t n = 0; n < size(); ++n ) - delete (*this)[n]; - clear(); - } - - ~wxListLineDataArray() { Clear(); } -}; - //----------------------------------------------------------------------------- // wxListHeaderWindow (internal) //----------------------------------------------------------------------------- @@ -785,7 +772,7 @@ public: protected: // the array of all line objects for a non virtual list control (for the // virtual list control we only ever use m_lines[0]) - wxListLineDataArray m_lines; + std::vector m_lines; // the list of column objects std::vector m_columns; @@ -860,13 +847,15 @@ protected: { wxASSERT_MSG( n != (size_t)-1, wxT("invalid line index") ); + wxListMainWindow *self = wxConstCast(this, wxListMainWindow); + if ( IsVirtual() ) { - wxConstCast(this, wxListMainWindow)->CacheLineData(n); + self->CacheLineData(n); n = 0; } - return m_lines[n]; + return &self->m_lines[n]; } // get a dummy line which can be used for geometry calculations and such: diff --git a/src/generic/listctrl.cpp b/src/generic/listctrl.cpp index af3c245325..5cfa119a1c 100644 --- a/src/generic/listctrl.cpp +++ b/src/generic/listctrl.cpp @@ -1611,10 +1611,9 @@ wxListMainWindow::~wxListMainWindow() void wxListMainWindow::SetReportView(bool inReportView) { - const size_t count = m_lines.size(); - for ( size_t n = 0; n < count; n++ ) + for ( auto& line : m_lines ) { - m_lines[n]->SetReportView(inReportView); + line.SetReportView(inReportView); } } @@ -1650,22 +1649,21 @@ wxListLineData *wxListMainWindow::GetDummyLine() const // control changed as it would have the incorrect number of fields // otherwise if ( !m_lines.empty() && - m_lines[0]->m_items.size() != (size_t)GetColumnCount() ) + m_lines[0].m_items.size() != (size_t)GetColumnCount() ) { - self->m_lines.Clear(); + self->m_lines.clear(); } if ( m_lines.empty() ) { - wxListLineData *line = new wxListLineData(self); - self->m_lines.push_back(line); + self->m_lines.emplace_back(self); // don't waste extra memory -- there never going to be anything // else/more in this array self->m_lines.shrink_to_fit(); } - return m_lines[0]; + return &self->m_lines[0]; } // ---------------------------------------------------------------------------- @@ -4267,7 +4265,6 @@ void wxListMainWindow::DeleteItem( long lindex ) } else { - delete m_lines[index]; m_lines.erase( m_lines.begin() + index ); } @@ -4295,10 +4292,8 @@ void wxListMainWindow::DeleteColumn( int col ) if ( !IsVirtual() ) { // update all the items - for ( size_t i = 0; i < m_lines.size(); i++ ) + for ( auto& line : m_lines ) { - wxListLineData * const line = GetLine(i); - // In the following atypical but possible scenario it can be // legal to call DeleteColumn() but the items may not have any // values for it: @@ -4310,10 +4305,10 @@ void wxListMainWindow::DeleteColumn( int col ) // 6. Call DeleteColumn(). // So we need to check for this as otherwise we would simply crash // if this happens. - if ( line->m_items.size() <= static_cast(col) ) + if ( line.m_items.size() <= static_cast(col) ) continue; - line->m_items.erase(line->m_items.begin() + col); + line.m_items.erase(line.m_items.begin() + col); } } @@ -4363,7 +4358,7 @@ void wxListMainWindow::DoDeleteAllItems() if ( InReportView() ) ResetVisibleLinesRange(); - m_lines.Clear(); + m_lines.clear(); } void wxListMainWindow::DeleteAllItems() @@ -4531,9 +4526,9 @@ void wxListMainWindow::InsertItem( wxListItem &item ) } } - wxListLineData *line = new wxListLineData(this); + wxListLineData line(this); - line->SetItem( item.m_col, item ); + line.SetItem( item.m_col, item ); if ( item.m_mask & wxLIST_MASK_IMAGE ) { // Reset the buffered height if it's not big enough for the new image. @@ -4548,7 +4543,7 @@ void wxListMainWindow::InsertItem( wxListItem &item ) } } - m_lines.insert( m_lines.begin() + id, line ); + m_lines.insert( m_lines.begin() + id, std::move(line) ); m_dirty = true; @@ -4593,9 +4588,9 @@ long wxListMainWindow::InsertColumn( long col, const wxListItem &item ) if ( !IsVirtual() ) { // update all the items - for ( size_t i = 0; i < m_lines.size(); i++ ) + for ( auto& line : m_lines ) { - auto& items = GetLine(i)->m_items; + auto& items = line.m_items; if ( insert ) items.emplace(items.begin() + col, this); else @@ -4645,13 +4640,13 @@ struct wxListLineComparator { } - bool operator()(wxListLineData* const& line1, - wxListLineData* const& line2) const + bool operator()(const wxListLineData& line1, + const wxListLineData& line2) const { wxListItem item; - line1->GetItem( 0, item ); + line1.GetItem( 0, item ); wxUIntPtr data1 = item.m_data; - line2->GetItem( 0, item ); + line2.GetItem( 0, item ); wxUIntPtr data2 = item.m_data; return m_f(data1, data2, m_data) < 0; }