Fix memory leak in the art provider sample (PR #23418)
PB
The list client data must be destroyed not only when closing the resource browser dialog (see 754f75c) but also whenever the art client is changed.
Closes #23417
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/23418
Commit Summary
- bc890c0 Fix memory leak in the art provider sample
File Changes
(2 files)
- M samples/artprov/artbrows.cpp (15)
- M samples/artprov/artbrows.h (2)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/23418.patch
- https://github.com/wxWidgets/wxWidgets/pull/23418.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
PB
As I do not like using raw pointers much, here is an alternative solution
diff --git a/samples/artprov/artbrows.cpp b/samples/artprov/artbrows.cpp index e33319c348..ec030a4730 100644 --- a/samples/artprov/artbrows.cpp +++ b/samples/artprov/artbrows.cpp @@ -35,7 +35,8 @@ else \ ind = 0; \ list->InsertItem(index, #id, ind); \ - list->SetItemPtrData(index, wxPtrToUInt(new wxString(id))); \ + artIdList.push_back(id); \ + list->SetItemData(index, static_cast<long>(artIdList.size() - 1)); \ index++; \ } @@ -56,7 +57,7 @@ static void FillClients(wxChoice *choice) } static void FillBitmaps(wxImageList *images, wxListCtrl *list, - int& index, + int& index, wxVector<wxArtID>& artIdList, const wxArtClient& client, const wxSize& size) { ART_ICON(wxART_ERROR) @@ -194,15 +195,6 @@ wxArtBrowserDialog::wxArtBrowserDialog(wxWindow *parent) SetArtClient(wxART_MESSAGE_BOX); } -wxArtBrowserDialog::~wxArtBrowserDialog() -{ - const int itemCount = m_list->GetItemCount(); - - // item data are set by the ART_ICON macro - for ( int i = 0; i < itemCount; ++i ) - delete reinterpret_cast<wxString*>(m_list->GetItemData(i)); -} - wxSize wxArtBrowserDialog::GetSelectedBitmapSize() const { const int size = bitmapSizes[m_sizes->GetSelection()]; @@ -222,23 +214,21 @@ void wxArtBrowserDialog::SetArtClient(const wxArtClient& client) if (sel < 0) sel = 0; m_list->DeleteAllItems(); - FillBitmaps(img, m_list, index, client, wxSize(16, 16)); + m_artIdList.clear(); + FillBitmaps(img, m_list, index, m_artIdList, client, wxSize(16, 16)); m_list->AssignImageList(img, wxIMAGE_LIST_SMALL); m_list->SetColumnWidth(0, wxLIST_AUTOSIZE); m_list->SetItemState(sel, wxLIST_STATE_FOCUSED, wxLIST_STATE_FOCUSED); m_client = client; - - const wxString *data = (const wxString*)m_list->GetItemData(sel); - SetArtBitmap(*data, m_client); + SetArtBitmap(m_artIdList[m_list->GetItemData(sel)], m_client); } void wxArtBrowserDialog::OnSelectItem(wxListEvent &event) -{ - const wxString *data = (const wxString*)event.GetData(); - m_currentArtId = *data; - SetArtBitmap(*data, m_client, GetSelectedBitmapSize()); +{ + m_currentArtId = m_artIdList[event.GetIndex()]; + SetArtBitmap(m_currentArtId, m_client, GetSelectedBitmapSize()); } void wxArtBrowserDialog::OnChangeSize(wxCommandEvent& WXUNUSED(event)) samples/artprov/artbrows.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/samples/artprov/artbrows.h b/samples/artprov/artbrows.h index 3827762538..5b89695165 100644 --- a/samples/artprov/artbrows.h +++ b/samples/artprov/artbrows.h @@ -13,6 +13,7 @@ #include "wx/dialog.h" #include "wx/artprov.h" +#include <wx/vector.h> class WXDLLIMPEXP_FWD_CORE wxListCtrl; class WXDLLIMPEXP_FWD_CORE wxListEvent; @@ -22,7 +23,6 @@ class wxArtBrowserDialog : public wxDialog { public: wxArtBrowserDialog(wxWindow *parent); - ~wxArtBrowserDialog(); void SetArtClient(const wxArtClient& client); void SetArtBitmap(const wxArtID& id, const wxArtClient& client, const wxSize& size = wxDefaultSize); @@ -39,7 +39,8 @@ private: wxStaticText *m_text; wxString m_client; wxChoice *m_sizes; - wxString m_currentArtId; + wxVector<wxArtID> m_artIdList; + wxArtID m_currentArtId; wxDECLARE_EVENT_TABLE(); };
BTW, when testing the code changes I noticed that (on MSW) the art provider in some cases uses a different art for the same id depending on client, even when the resolution is the same?!
Is this expected behaviour?
I did not look into it, but it seems to happen for the arts where the icon is obtained with SHGetStockIconInfo().
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Thanks for fixing this! I think I'm just going to apply this one as I don't quite understand the other one: if we use indices, why do we need to use client data at all any longer, couldn't we just use the item index directly?
I have no idea why we don't use the native icon for wxART_MESSAGE_BOX, but this certainly doesn't seem intentional, i.e. would be worth looking into...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
PB
Thanks for fixing this! I think I'm just going to apply this one as I don't quite understand the other one: if we use indices, why do we need to use client data at all any longer, couldn't we just use the item index directly?
Yes, we could. This is my mistake, when I am just used not to use item index directly as a container index, as the list item order can change (sorting or adding/removing items). But as you prefer the first version, I am not going to change the second one.
I have no idea why we don't use the native icon for wxART_MESSAGE_BOX, but this certainly doesn't seem intentional, i.e. would be worth looking into...
I thought the message box one was native and the other came from Tango. But the Tango one (in art/tango/dialog_error.h) looks very different. I have also realized that the other icon is displayed only when in the sample I select "Default" (which translates to 40x40 on my system), in any other available size the icon is the same as for the message box client.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
PB
As for the art for some ids looking differently on MSW depending on client/resolution. For example, this code
mainSizer->Add(new wxStaticBitmap(this, wxID_ANY, wxArtProvider::GetBitmap(wxART_ERROR, wxART_OTHER))); mainSizer->Add(new wxStaticBitmap(this, wxID_ANY, wxArtProvider::GetBitmap(wxART_ERROR, wxART_OTHER, wxSize(64, 64))));
results in (both images obtained with Win32 API)
So, when calling wxArtProvider::GetBitmap(wxART_ERROR, wxART_MESSAGE_BOX):
Case 1. Without the size specified, the bitmap is obtained with ::LoadIcon(IDI_ERROR) in wxICOResourceHandler::LoadIcon() at the default size (-1x-1 ends at 40x40 for me).
Case 2. With the size specified (64x64 in my example), the bitmap is obtained with ::SHDefExtractIcon() in ::MSWGetBitmapFromIconLocation(). It is because ::SHDefExtractIcon() is also called first in Case 1, but it fails there when requesting size -1x-1, so the code tries another route.
BTW, the images that used to be displayed in message boxes still get special treatment, but it used only in Case 1 (regardless of client).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.