ComboBox DeleteItem() not being called for all items?
AndersG
CTab Ctrl delete the underlying...). The problem is that I leak
strings that are allocated along with ComboBox items. This does not
happen for ComboBoxes that are destroyed when the dialog is destroyed.
It only happens with those that are emptied and filled during the
lifetime of the dialog, ie those that are dynamically updated when you
move from record to record
I finally got the time to investigate further and this is what I have:
void CLpCombo::DeleteItem(LPDELETEITEMSTRUCT lpDeleteItemStruct)
{
// Delete CString pointed to by item.
TRACE("\nDeleting %d from control %d (%s)",
lpDeleteItemStruct->itemID,
lpDeleteItemStruct->CtlID,
*((CString*)lpDeleteItemStruct->itemData));
delete (CString*)lpDeleteItemStruct->itemData;
}
and in the calling dialog:
// Delete old list
int i = pCombo->GetCount();
TRACE("\nItems: %d", i);
for(int j=0;j<i;j++)
{
TRACE("\nDeleting item %d, data: %X", j, pCombo->GetItemData(0));
rc = pCombo->DeleteString(0);
TRACE(" returns %d", rc);
}
Trace looks like this:
Items: 3
Deleting item 0, data: 19B50C0
Deleting 0 from control 1014 (0) returns 2
Deleting item 1, data: 195CBA0
Deleting 0 from control 1014 (11) returns 1
Deleting item 2, data: 195CAF0 returns 0
Ie, it appears as though DeleteItem is never called for the last item
in the list? It is 100% repeatable, ie it never gets called for lists
where there is only one item in the box. Called once for lists with
two items etc.
Giovanni Dicanio
> // Delete old list
> int i = pCombo->GetCount();
> TRACE("\nItems: %d", i);
> for(int j=0;j<i;j++)
> {
> TRACE("\nDeleting item %d, data: %X", j, pCombo->GetItemData(0));
> rc = pCombo->DeleteString(0);
> TRACE(" returns %d", rc);
> }
[...]
> Ie, it appears as though DeleteItem is never called for the last item
> in the list? It is 100% repeatable, ie it never gets called for lists
> where there is only one item in the box. Called once for lists with
> two items etc.
Have you tried deleting from last index to 0, like this ?
// Delete all items from the combobox:
for (int i = pCombo->GetCount() - 1; i >= 0; i--)
{
pCombo->DeleteString( i );
}
Giovanni
Giovanni Dicanio
problem, but might be worth writing):
AndersG wrote:
> I finally got the time to investigate further and this is what I have:
>
> void CLpCombo::DeleteItem(LPDELETEITEMSTRUCT lpDeleteItemStruct)
> {
> // Delete CString pointed to by item.
> TRACE("\nDeleting %d from control %d (%s)",
> lpDeleteItemStruct->itemID,
> lpDeleteItemStruct->CtlID,
> *((CString*)lpDeleteItemStruct->itemData));
>
> delete (CString*)lpDeleteItemStruct->itemData;
When you have a %s and you want to pass a CString, it now works, but is
considered not very safe.
It works because of some "magic" in the internal design of CString, but
if you have a %s and variable argument list, is better to explicitly
static_cast to LPCTSTR, or use CString::GetString() method, e.g.
CString str;
int x;
int y;
TRACE("Some data: %d, %s, %d", x, str.GetString(), y );
So, I would adjust your code a bit like this:
void CLpCombo::DeleteItem(LPDELETEITEMSTRUCT lpDeleteItemStruct)
{
// Get custom CString pointer from lpDeleteItemStruct
CString * pstr =
reinterpret_cast<CString *>( lpDeleteItemStruct->itemData );
// Trace deletion
TRACE("\nDeleting %d from control %d (%s)",
lpDeleteItemStruct->itemID,
lpDeleteItemStruct->CtlID,
pstr->GetString()));
// Free previously allocated CString
delete pstr;
}
Giovanni
AndersG
<giovanniDOTdica...@REMOVEMEgmail.com> wrote:
> To add to what I wrote in previous message (not related to your main
> problem, but might be worth writing):
>
Thanks a bundle! I used your DeleteItem() instead and tried:
// Delete all items from the combobox:
for (int i = pCombo->GetCount() - 1; i >= 0; i--)
{
pCombo->DeleteString( i );
}
Still the same issue. Only gets called twice for a combo with three
items:
Deleting 2 from control 1014 (10)
Deleting 1 from control 1014 (11)
AndersG
<giovanniDOTdica...@REMOVEMEgmail.com> wrote:
> problem, but might be worth writing):
PS. I assume it might be prudent to use:
if(lpDeleteItemStruct->itemData != 0)
{
do deletion
}
Just in case?
The way I have devised to get around this, unless I can find a real
solution is to use:
//
// Tom hela combon och se till att strängarna tas bort
//
void CLpCombo::Clear()
{
int i;
CString msg;
TRACE("\nCLpCombo::Clear()");
for(i=GetCount()-1;i>=0;i--)
{
ClearItem(i);
}
}
//
// Toa bort en post och se till att strängen tas bort
// Returnerar samma som DeleteItem
//
int CLpCombo::ClearItem(int i)
{
CString* value=((CString*)GetItemDataPtr(i));
TRACE("\nCLpCombo::ClearItem() %d deleting %d", GetDlgCtrlID(),
GetItemData(i));
if(value!=NULL)
{
delete value; // Radera strängen
SetItemData(i,0); // och indikera att den är borta
}
return DeleteString(i);
}
And call those when I need to clear the combos. I assume that I should
adjust them as well, using
CString * pstr =
reinterpret_cast<CString *>( lpDeleteItemStruct->itemData
??
Giovanni Dicanio
> Thanks a bundle! I used your DeleteItem() instead and tried:
>
> // Delete all items from the combobox:
> for (int i = pCombo->GetCount() - 1; i >= 0; i--)
> {
> pCombo->DeleteString( i );
> }
>
> Still the same issue. Only gets called twice for a combo with three
> items:
I verified that... it happens to me too (VS2008, without SP1).
I may be missing something (it's late here :) but it sounds strange to me..
As a workaround, I tried defining a custom CMyCombo class derived from
CComboBox, and I redefined [*] the DeleteString() and ResetContent()
methods, to implement a custom cleanup operation of the CString* stored
in item data.
Basically, I used code like this (comments included):
<code>
int CMyCombo::DeleteString(UINT nIndex)
{
// Check if is there any custom item data to delete first
SafeDeleteItem(nIndex);
// Normal delete string
return CComboBox::DeleteString(nIndex);
}
void CMyCombo::ResetContent()
{
// Check if is there any custom item data to delete first
for (int i = 0; i < GetCount(); i++)
SafeDeleteItem(i);
// Normal reset content
CComboBox::ResetContent();
}
void CMyCombo::SafeDeleteItem(int nIndex)
{
DWORD_PTR itemData = GetItemData(nIndex);
ASSERT(itemData != CB_ERR);
if (itemData == 0)
return; // Nothing to do
// Get custom CString pointer from lpDeleteItemStruct
CString * pstr = reinterpret_cast<CString *>(itemData);
// Free previously allocated CString
delete pstr;
pstr = NULL;
// Clear item data
VERIFY( SetItemData(nIndex, 0) != CB_ERR );
}
</code>
A test MFC project is attached.
In my simple tests, there was no memory leak.
HTH,
Giovanni
[*] These methods are not 'virtual' in CComboBox class, so it is not a
C++ virtual method override.
Giovanni Dicanio
A small note:
> void CMyCombo::SafeDeleteItem(int nIndex)
> {
...
> // Get custom CString pointer from lpDeleteItemStruct
> CString * pstr = reinterpret_cast<CString *>(itemData);
The above comment contains a leftover from the previous code I posted here.
In this version, the CString pointer is not from lpDeleteItemStruct,
instead it is just from GetItemData(), of course.
Giovanni
AndersG
I am glad that it is a bona-fide bug and not just me ;) I will try
your workaround as it feels cleaner that mine.
AndersG
<giovanniDOTdica...@REMOVEMEgmail.com> wrote:
> Giovanni Dicanio wrote:
>
Yay! No more "Memory leak detected".. I also had to remove void
CLpCombo::DeleteItem(LPDELETEITEMSTRUCT lpDeleteItemStruct) since it
was no longer needed.
Thanks again! Many of these classes were originally written win VC6 on
Win95 and I know that it did work then. Not sure when this error
reared it's ugly head, but it might have been when goign from 2000 to
Xp.
Giovanni Dicanio
> Yay! No more "Memory leak detected".. I also had to remove void
> CLpCombo::DeleteItem(LPDELETEITEMSTRUCT lpDeleteItemStruct) since it
> was no longer needed.
>
> Thanks again! Many of these classes were originally written win VC6 on
> Win95 and I know that it did work then. Not sure when this error
> reared it's ugly head, but it might have been when goign from 2000 to
> Xp.
Anders: you're welcome.
I'm curious, and I will try to investigate this problem next in time.
If I discover something interesting, I will post back here.
Giovanni
AndersG
> If I discover something interesting, I will post back here.
>
> Giovanni
OK. I will make a note to check this thread in a year ;)
Giovanni Dicanio
Actually, you don't need that long time ;-)
My first thought about this issue was that maybe MFC had some bugs, so I
chose to try to develop a pure Win32 SDK C++ app, to avoid the MFC
layer, and see what happens.
Surprisingly to me, the "bug" was present again (i.e. WM_DELETEITEM is
not called when CB_RESETCONTENT is called, and is not called when
CB_DELETESTRING is called for the last item).
After a web search I found this post by an SDK MVP, that confirms the
issue, and I found more details also in DELETEITEMSTRUCT at MSDN:
http://msdn.microsoft.com/en-us/library/bb775151.aspx
Here's the post by Sten Westerback (MVP SDK)
http://www.eggheadcafe.com/software/aspnet/29288946/wmdeleteitem-does-not-wo.aspx
<quote>
WM_DELETEITEM does not work as adverstised in MSDN - Sten Westerback
\(MVP SDK\)
16-Feb-07 12:23:07
Forgot to read related pages i would say. In description of DELETEITEMSTRUCT
one can read that...
Windows NT/2000/XP: The system sends a WM_DELETEITEM message only for items
deleted from an owner-drawn list box (with the LBS_OWNERDRAWFIXED or
LBS_OWNERDRAWVARIABLE style) or owner-drawn combo box (with the
CBS_OWNERDRAWFIXED or CBS_OWNERDRAWVARIABLE style).
I guess you don't have a owner drawn dialog.. and that someone at Microsoft
just didn't realize that the message description should also have that OS
specific comment... then again.. you shouldn't have got the message from
LB_DELETESTRING.
Anyway, normally you would know when you perform an action that would remove
one or all of the items in a listbox. If not you might want to subclass to
catch the messages and use them somehow.
- Sten
</quote>
Giovanni
AndersG
<giovanniDOTdica...@REMOVEMEgmail.com> wrote:
Ah.. Well, as long as it works this way, I am happy. I did change one
small thing, Instead of ASSERT() i used:
if (itemData == CB_ERR)
return;
The post you quoted confirms my suspicion that the code did once work,
in 9x and since the actual data leaked in our case was pretty small,
the issue was not a big one even on 2000/Xp
AndersG
Odd though.. The MSDN entry only says: "Windows NT/2000/XP: The system
sends a WM_DELETEITEM message only for items deleted from an owner-
drawn list box (with the LBS_OWNERDRAWFIXED or LBS_OWNERDRAWVARIABLE
style) or owner-drawn combo box (with the CBS_OWNERDRAWFIXED or
CBS_OWNERDRAWVARIABLE style)."
It does not explain why it is called for all but the last item?
Giovanni Dicanio
That MSDN documentation is not good, I would define it kind of a
"documentation bug".
Moreover, that control's behaviour is illogical to me; I mean: either
the WM_DELETEITEM is never sent, or it is sent for every item, but it is
an illogical thing that it is sent for every item excluding the last one.
Since we can't modify Windows source code for the combobox control :), I
would suggest you to just use the workaround I proposed (or something
similar).
Giovanni
Giovanni Dicanio
> AndersG wrote:
>> On 14 Dec, 14:12, AndersG <gustafsson.and...@gmail.com> wrote:
>>
>> Odd though.. The MSDN entry only says: "Windows NT/2000/XP: The system
>> sends a WM_DELETEITEM message only for items deleted from an owner-
>> drawn list box (with the LBS_OWNERDRAWFIXED or LBS_OWNERDRAWVARIABLE
>> style) or owner-drawn combo box (with the CBS_OWNERDRAWFIXED or
>> CBS_OWNERDRAWVARIABLE style)."
>>
>> It does not explain why it is called for all but the last item?
>
> That MSDN documentation is not good
I've added a comment about this issue in WM_DELETEITEM MSDN page here:
http://msdn.microsoft.com/en-us/library/bb761362.aspx
Giovanni
Joseph M. Newcomer
wrong thing for non-owner-draw combo boxes and list boxes. I'm exploring this and will
add a detailed explanation to my MSDN Errors page.
joe
Joseph M. Newcomer [MVP]
email: newc...@flounder.com
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm
Giovanni Dicanio
> The documentation is confusing, and lies, and the implementation is buggy and does the
> wrong thing for non-owner-draw combo boxes and list boxes. I'm exploring this and will
> add a detailed explanation to my MSDN Errors page.
Great, Joe!
Giovanni