Opened 5 years ago

Closed 5 years ago

#10687 closed defect (fixed)

Misterious "ListView_SetItem failed" now with assertion (msw)

Reported by: nitram.cero Owned by:
Priority: normal Milestone:
Component: wxMSW Version: stable-latest
Keywords: Cc:
Blocked By: Blocking:
Patch: no

Description

Where do I begin?...

ClearAll() is confusing. I used it as if it were a "DeleteAllItems()".

Then when I wanted to SetItem() with a column > 0 on a new item, it failed (as ClearAll also counter-intuitively removes the columns).

Windows' GetLastError() returned nothing at all. It gave me a headache.

I think ClearAll() should be deprecated to give place to ClearItems()/ClearColumns().
Because "clear" in programming usually means a complete removal of data, not the structure that holds it.
It's similar to say that to "clear a grid control" would have to make the columns and rows equal to 1.
It's confusing.


On to the fix:

I added an assertion in wxMSW's "wxConvertToMSWListItem" to check if the info parameter column is in a valid range acording to ctrl parameter's column count.

I attached the patch (made from SVN 60049)

Also commented out a "item.cchTextMax = 0;" before the SetItem call that was absurd. If needed for any reason, it should be at wxConvertToMSWListItem() instead of being in SetItem function like a hack.

Regards
-Martín

Attachments (1)

ListCtrl_SetItem.patch download (1.2 KB) - added by nitram.cero 5 years ago.
the fix

Download all attachments as: .zip

Change History (10)

Changed 5 years ago by nitram.cero

the fix

comment:1 Changed 5 years ago by VZ

(In [60055]) remove "item.cchTextMax=0" line from SetItem() as it doesn't seem to make any sense (see #10687)

comment:2 Changed 5 years ago by VZ

(In [60056]) added assert checking the column index validity (see #10687)

comment:3 Changed 5 years ago by vadz

  • Priority changed from high to low
  • Resolution set to fixed
  • Status changed from new to closed

I agree that ClearAll() may be confusing but it's probably not confusing enough to remove it. And after all chances of not noticing that it doesn't do what you expect are rather low.

Thanks for the assert and other fixes!

comment:4 Changed 5 years ago by nitram.cero

Hehe, no problem.

That was probably my anger talking, because of wasting half an hour tracing a bug on a damn list item!
Probably now that the assertion raises it's less common to get that error.
(I found about 4 topics asking about the "ListView_SetItem() failed" error on wx lists and wxpython lists, so go figure)

Regards
-Martín

comment:5 Changed 5 years ago by rk

  • Patch unset
  • Priority changed from low to normal
  • Resolution fixed deleted
  • Status changed from closed to reopened

The new assert added in r60056 makes it impossible to use wxListCtrl in wxLC_LIST mode. In this mode you are not required to add any columns. When I try to insert an item now the assert will wrongly detect this as an error. You can see this faulty behavior in the listctrl sample by switching it to the "List View" mode. So either the documentation is now wrong in telling me that in wxLC_LIST mode the columns are created automatically or the assert should only be valid in report mode.

comment:6 Changed 5 years ago by nitram.cero

Probably GetColumnCount() is returning 0 instead of 1 on non-report listviews.

I can't check it up now, I'm filled with work... is either that or the column not being added as you say.

Sorry for de inconvenience, as it's my patch :p

comment:7 Changed 5 years ago by rk

GetColumnCount() is in fact returning 0 in list mode. So maybe the assert is not the problem after all but only revealed a problem in the ListCtrl itself.

comment:8 Changed 5 years ago by VZ

(In [60277]) correct the assert added by r60056 to not trigger in non-report modes (see #10687)

comment:9 Changed 5 years ago by vadz

  • Resolution set to fixed
  • Status changed from reopened to closed

Thanks for noticing this, rk, should be fixed by the above commit.

Note: See TracTickets for help on using tickets.