AddGadgetItem image bug, all OS

[Keya]

I believe I have isolated and can confirm a bug which is preventing images being correctly added to Combo/List(etc) gadgets. The behavior is the same on Windows, Linux and Mac, although somewhat reversed in Windows which I'll explain shortly.

I first noticed it when creating a simple list of 256 icons in a Combogadget - some of the icons were previous icons, not the new one it should've been, and there was an element of randomness, and there was no logical reason for this. Here for example every icon is a grayscale shade from 0-255, but you can see what happens when handle id's get reused (even though they were newly CreateImage'd):


I isolated that problem - AddGadgetItem() only adds a new image to the list (comctl32's "ImageList" in case of Windows) if that ImageId() hasn't been used before. Even if it's a brand new CreateImage'd image, and with different content, if the handle ID has ever been used before then AddGadgetItem() will refer to cache, ignoring the new image. It seems like there is an internal list of previous handle id's to use as a cache.

I'm guessing this is a well-intended feature to save memory/time, but has turned out a big problem for me. But if it's an intended feature it would probably be also trivial to fix, I hope!

If this is fixed:
1) there would be no problems worrying about colliding handles (the image bug above wouldnt exist)
2) the same image could be used (eg. Box(newcolor)) rather than FreeImage/CreateImage'ing hundreds/thousands of times - it's a great SPEED boost too.

To work around this as it stands we would have to detect if a CreateImage'd handle has been used before (by keeping our own internal list), and if we ever encounter a used handle we'd have to essentially go into a CreateImage loop until a new handle was derived, and that might risk infinite looping... it all sounds a messy workaround anyway, and not a proper solution.

What was happening in my case is that sometimes, one or more of those 255 handles had already been used - even though they had been FreeImage'd and newly CreateImage'd. (whatever Windows uses for its "GetNewHandle" isn't a PRNG, and there is this chance of repeats). I tested on XP and Win7-64, both had the same problem -- every 2nd or 3rd run would exhibit handle collisions.

So I was wondering - is this just an OS limitation of the gadget itself? But using GDIView i confirmed that a list with 256 images is a Bitmap gdi object of height 4192 (width 16 of course), but only height 4032 when there were 7 colliding handles.

Using a debugger i then bpx'd on every ImageList_ api in comctl32 and confirmed that ImageList_Add is not being called if the image handle has been used before - but I see no reason for this, and if it called ImageList_Add there'd be no problem.

So that suggests to me its a PB bug and not OS, especially because the behavior is basically the same across all OS.

This limitation also means that every image added to an imagelist must essentially be CreateImage'd, but if you're creating for example color palettes all you want to do is repeatedly redraw the same image with Box(newcolor) - it's sooo much faster when theres thousands of images involved, but thats currently not a possibility because of this, so CreateImage must be called for each palette, but then that runs into the colliding handle issue. Stuck either way!

Simple example of trying to use the same image to add a Blue and Red icon. In this demo it uses the same image (just Box()'ing over the first image to draw the second image), but even if you change it to use FreeImage/CreateImage you then risk the colliding handle issue:

Code: Select all

#List1=1
If OpenWindow(0, 0, 0, 270, 80, "ComboBoxGadget", #PB_Window_SystemMenu | #PB_Window_ScreenCentered)
  ComboBoxGadget(#List1, 10, 10, 250, 21, #PB_ComboBox_Image)
  
  image = CreateImage(#PB_Any, 16,16)
  If image And StartDrawing(ImageOutput(image))  
    Box(0, 0, 15, 15, RGB(255,0,0)) ;red
    StopDrawing()
  EndIf
  AddGadgetItem(#List1, -1,"Red", ImageID(image))
  
  If image And StartDrawing(ImageOutput(image))  
    Box(0, 0, 15, 15, RGB(0,0,255)) ;blue
    StopDrawing()
  EndIf
  AddGadgetItem(#List1, -1,"Blue", ImageID(image))
  ;yes i've SaveImage'd both to ensure and confirm that they are indeed Red and Blue
  
  Repeat : Until WaitWindowEvent() = #PB_Event_CloseWindow
EndIf

Windows output (on Linux and Mac they're actually both Blue, but otherwise the same problem):



Here's the program i used to generate the 256 shades of grayscale (each image is FreeImage'd and the next newly CreateImage'd, so should have unique handle ID's but sometimes they collide). I usually only have to run it 2 or 3 times (tested XP/Win7) before i get collisions, which youll see in debug window, which demonstrates that even if you use newly CreateImage'd images there can still be problems due to this behavior:

Code: Select all

#MAXUNIQUEITEMS = 4096
Procedure.l Unique(lVal.l)
  Static arCnt.l
  Static Dim arItem.l(#MAXUNIQUEITEMS)
  If arCnt = 0
    Goto AddElement
  EndIf
  For i.l = 0 To arCnt-1
    If arItem(i) = lVal 
      ProcedureReturn i+1 ;'// Not added (already exists/isn't unique)
    EndIf
  Next i
  AddElement:
  arItem(arCnt) = lVal
  arCnt = arCnt + 1
  ProcedureReturn 0   ;'// Added (is unique)
EndProcedure

Procedure CreateSwatchImage(Color)
  Protected image = CreateImage(#PB_Any, 16,16)
  If image And StartDrawing(ImageOutput(image))  
    Box(0, 0, 15, 15, RGB(Color,Color,Color)) ;grayscale
    StopDrawing()
  EndIf
  ProcedureReturn image
EndProcedure

#List1=1
If OpenWindow(0, 0, 0, 270, 180, "ComboBoxGadget", #PB_Window_SystemMenu | #PB_Window_ScreenCentered)
  ComboBoxGadget(#List1, 10, 70, 250, 21, #PB_ComboBox_Image)
 
  For a = 0 To 255
    hImg = CreateSwatchImage(a)
    AddGadgetItem(#List1, -1,"Item #" + Str(a), ImageID(hImg))
    
    oldpos = Unique(ImageID(hImg))  
    If oldpos
      Debug(Str(a)+" exists as "+ Str(oldpos))
    EndIf
    
    If hImg
      FreeImage(hImg)
    EndIf
  Next
   
  Repeat : Until WaitWindowEvent() = #PB_Event_CloseWindow
EndIf

[#NULL]

i often saw code like this in the forums:

Code: Select all

AddGadgetItem(..., ImageID(img))
FreeImage(img)

and i always found it suspicious, but they probably knew about the caching or it just worked. but the gadget is not the owner of that image so how could it know when the image becomes invalid if you don't use RemoveGadgetItem() or something similar. or in other words don't free it if it's still referenced elsewhere. at least that's how it looks to me .

[User_Russian]

In my opinion this is not bug. This is necessary to reduce the memory usage by adding a plurality of identical images.
It is necessary to re-create the image.

Code: Select all

#List1=1
If OpenWindow(0, 0, 0, 270, 80, "ComboBoxGadget", #PB_Window_SystemMenu | #PB_Window_ScreenCentered)
  ComboBoxGadget(#List1, 10, 10, 250, 21, #PB_ComboBox_Image)
  
  CreateImage(0, 16,16)
  If StartDrawing(ImageOutput(0))  
    Box(0, 0, 15, 15, RGB(255,0,0)) ;red
    StopDrawing()
  EndIf
  AddGadgetItem(#List1, -1,"Red", ImageID(0))
  
  CreateImage(0, 16,16)
  If StartDrawing(ImageOutput(0))  
    Box(0, 0, 15, 15, RGB(0,0,255)) ;blue
    StopDrawing()
  EndIf
  AddGadgetItem(#List1, -1,"Blue", ImageID(0))
  ;yes i've SaveImage'd both to ensure and confirm that they are indeed Red and Blue
  
  Repeat : Until WaitWindowEvent() = #PB_Event_CloseWindow
EndIf

[Keya]

User_Russian, no, because the ImageList maintains its OWN painted/drawn list (a Bitmap gdi object (the ImageList structure from comctl32) which grows in height as icons are added), so it doesn't matter if you use one image to draw them or multiple.

And this is why you can FreeImage after adding to the list - it's a single, drawn bitmap, it's not a table of handles-to-bitmaps. You can easily see this with GDIView which i linked to in first post:
Here's the ImageList bitmap when all 256 images are ImageList_Add'd:

but when colliding handles occur (7 in this case):


But the OS DOES let you call ImageList_Add twice, with the same handle (but different image contents), and it will correctly add them - so no, it is not necessary to use different images as you say. But AddGadgetItem() refuses to call ImageList_Add if it's seen that handle before - even if the contents are new.

So how is that not a PB bug, if the OS _does_ allow for it, and the problem only exists because the PB command isn't calling it?

And what would be the purpose of maintaining a list of previously-used handles if it has a fully-drawn bitmap anyway? wouldn't make sense.

If PB was calling ImageList_Add each time then 1) the "colliding handle" issue wouldn't exist, and 2) it'd be a LOT faster, due to not having to CreateImage/FreeImage a thousand times for a thousand icons. And this thread wouldn't exist as there'd be no problems.

And if you still think it's "necessary, not a bug" (clearly it's not necessary because the Winapi doesn't necessitate it) then HOW would you prevent the colliding handles problem? keep your own list of used handles? That's basically what's already happening! And what would you do when you found a handle that had already been used, go into a potentially infinite loop calling CreateImage until you got a new one?!?

btw I can confirm your demo is not a solution - that is, using CreateImage(hardnumber) instead of CreateImage(#PB_Any) does NOT fix the colliding handle problem:

Code: Select all

Procedure CreateSwatchImage(Color)
  Protected image = CreateImage(0, 16,16)
  If image And StartDrawing(ImageOutput(0))  
    Box(0, 0, 15, 15, RGB(Color,Color,Color)) ;grayscale
    StopDrawing()
  EndIf
  ProcedureReturn image
EndProcedure

#List1=1
If OpenWindow(0, 0, 0, 270, 180, "ComboBoxGadget", #PB_Window_SystemMenu | #PB_Window_ScreenCentered)
  ComboBoxGadget(#List1, 10, 70, 250, 21, #PB_ComboBox_Image) 
  For a = 0 To 255*4    ;ie first four are 000000, last four are FFFFFF, makes it easier to get handle collision
    hImg = CreateSwatchImage(a/4)
    AddGadgetItem(#List1, -1,"Item #" + Str(a),hImg)
  Next   
  Repeat : Until WaitWindowEvent() = #PB_Event_CloseWindow
EndIf

[Keya]

Proof that YES you can call ImageList_Add with the same handle and it will correctly draw the new image contents, even if the handle has been used before (so no there isn't an OS limitation there - this has got to be a PB bug, especially given its consistent nature across all 3 OS)

Code: Select all

Procedure ReIndex(ListIconGadgetID.i, hImageList.i)
  n.i = ImageList_GetImageCount_(hImageList)  ;number of images in imagelist
  item.LVITEM
  ;set the iImage index for each item
  For i=0 To n-1
    item\Mask     = #LVIF_IMAGE
    item\iItem    = i
    item\iImage   = i
    SendMessage_(ListIconGadgetID, #LVM_SETITEM, 0, @item.LVITEM)
  Next
EndProcedure

;Create some images
zeroImg = CreateImage(0, 16, 16)
StartDrawing(ImageOutput(0))
Box(0, 0, 16, 16, RGB(0, 0, 0))
StopDrawing()

If OpenWindow(0,0,0,700,300,"Proof that DIFFERENT images with SAME handle id can be added to ImageList (no OS problem)",#PB_Window_SystemMenu|#PB_Window_ScreenCentered)
  ListIconGadget(1,10,10,680,300,"Title",600,#PB_ListIcon_GridLines|#PB_ListIcon_FullRowSelect)  
  SetGadgetAttribute(1,#PB_ListIcon_DisplayMode,#PB_ListIcon_Report)  
  For i=1 To 4
    AddGadgetItem(1,-1,"Testitem " + Str(i))
  Next
  
  ;create new ImageList
  IL_new.i = ImageList_Create_(16,16,#ILC_COLOR32,0,10)
  
  ImageList_Add_(IL_new, ImageID(0), 0)
  
;// NOW OVER-WRITE THE EXISTING IMAGE (same imageID/handle, just different color contents)
  StartDrawing(ImageOutput(0))
  Box(0, 0, 16, 16, RGB(255, 0, 0))
  StopDrawing()  
  ImageList_Add_(IL_new, ImageID(0), 0)
  
  StartDrawing(ImageOutput(0))
  Box(0, 0, 16, 16, RGB(0, 255, 0))
  StopDrawing()  
  ImageList_Add_(IL_new, ImageID(0), 0)
  
  StartDrawing(ImageOutput(0))
  Box(0, 0, 16, 16, RGB(0, 0, 255))
  StopDrawing()  
  ImageList_Add_(IL_new, ImageID(0), 0)
  
  SendMessage_(GadgetID(1), #LVM_SETIMAGELIST, #LVSIL_SMALL, IL_new)
  
  ;reindex the iImage member of LVITEM
  ReIndex(GadgetID(1),IL_new)
  
  Repeat : Until WaitWindowEvent() = #PB_Event_CloseWindow
EndIf

(thankyou to original author at http://forums.purebasic.com/english/vie ... 3&p=496962)

But although I might be able to use this as a solution in my Windows version it doesn't help with Linux or Mac, where the bug is the same.

But again, if this bug only exists because it was a well-intended 'feature' i'm guessing it should be trivial and quick to fix. I cannot move forward with my app at this stage so am not sure where to go

[freak]

You shouldn't free the image while it is still being used by the Gadget. If you don't free the image there is no collision possible. This is not a bug.

The ImageList APIs are not designed for large amounts of images (at least they weren't back when we implemented this in PB). The caching exists because re-using the same image over and over would kill performance if we called ImageList_Add() every time. Since the normal use-case is to have only a handful of unique symbols in the gadget but repeated in multiple lines (think file icons for example), the caching makes sense.

Sorry, but thousands of different icons are simply not the common use-case for this gadget. How is the user even supposed to see the difference between them?

You'll have to do this via API if you want the full control.

[Keya]

See but it's NOT "still being used" -- it's painted onto the ImageList Bitmap, which grows everytime ImageList_Add is called. I've already proven ImageList_Add can be used to paint different-content images that have the same image handle, and I've already confirmed in debugger that ImageList_Add does get called - but only when the handle id is different. But the OS doesn't impose that limitation. That's why FreeImage(img) is fine after AddGadgetItem(img) - it's (should've) been painted onto the ImageList's bitmap. I've spent the past 4-5 days on this problem and tested all avenues, even with debugger and winapi demo proof, and cannot see how this is anything but a PB bug.

[freak]

I elaborated a bit more above. Sorry for the edit.

[Keya]

You'll have to do this via API if you want the full control.

I see absolutely no reason why we can't do this with PB native API. All that's stopping it is a call to ImageList_Add - which it does! - but for whatever reason not if the image handle has been used before. But the OS doesn't impose any such restriction.

The ImageList is a fully-drawn bitmap who's height is extended by 16 every time ListImage_Add is called - it is not a table of handles to bitmaps.

And again, even if you FreeImage(img) and CreateImage(newimg), if the newimg has the same 'colliding' handle id then you get the problem, and on Windows that's quite often.

In the meantime, PB users can only use list/combo/etc/gadget with images if they keep all images open in memory -- even though there is absolutely no need for that because the ImageList maintains its own fully-drawn Bitmap. I need more than 10000 images, so I can't even do the 'currently recommended' PB approach.

So the current PB approach (which again seems like a well-intended "use cache to speed things up" feature has turned out to be a bug):
1) requires use of a lot of unnecessary memory (every list image must be held as its own (no longer needed) image, even though the ImageList has its own completely-independent fully-drawn version)
2) requires multiple images when 1 is suffice when dynamically drawing them
3) is therefore slower due to all the CreateImage calls
4) can risk running out of handles (ie. can only use 10000 handles by default on XP/Win7) -- yet ImageList's have absolutely no problem having hundreds of thousands of images painted on them,
5) therefore forces the "handle collision" issue on Windows, which I encounter even with less than 256 images every 3rd execution on average.

All of those problems are gone if AddGadgetItem() simply calls ImageList_Add like i'm asking it to / thought it would / have proven it can.

The OS allows for it, as already proven. PB can do this - as proven by it's calls to ImageList_Add. But it chooses not to if it's seen that handle before. I would be the first to apologise to Fred if i'm wrong, but after exhausting all avenues over the past 4-5 days I cannot see how this is anything but a PB bug in AddGadgetItem().

[freak]

The fact that there is an ImageList behind all this is an implementation detail. We could re-write the gadget tomorrow to a completely owner-drawn version and none of what you describe would apply anymore, yet it would still work as documented. This is why you have to keep the image allocated to be on the safe side.

As I said, your use-case is way out of the norm for this gadget. I'm not going to degrade the common use-case just so one guy can have his way. There are plenty of ways to do what you want (including Pure PB ways, for example via a CanvasGadget).

[Keya]

*sigh* this doesn't affect just me, but everyone using images in list/combo gadgets. It's very difficult to notice because it uses existing icons, so many users won't even know they have this bug.

And no there aren't "plenty of ways" to do this, it's either 1) list gadget, or 2) make my own list gadget.
I've already proven that there's nothing stopping PB's AddGadgetItem() from doing this, yet you want me to rewrite the wheel.

I will try patching it in a debugger now, as I've run out of other ways to prove this is a PB bug, and am going mental by nobody giving me the benefit of the doubt that just maybe this is a bug.

[Keya]

I found the main jump easy enough with run trace but as it turns out i can't just patch the exe (can't just jnz -> nop) because it still needs another call to SendMessage but 1) i don't have access to the ImageList handle from the PB code when it's used natively (as opposed to calling ImageList_Create myself), and 2) it's tricky inserting an api call in compiled assembly!, but looking through it made it easier to put together this example:

So here again is the current PB method, where for whatever reason it's not painting an image if the handle has been seen before, even if it's a new image:

Code: Select all

#List1=1: #Img1 =2
If OpenWindow(0,0,0,200,80,"Window",#PB_Window_SystemMenu|#PB_Window_ScreenCentered)
  
  ListIconGadget(#List1,10,10,200,80,"Column",180,#PB_ListIcon_GridLines|#PB_ListIcon_FullRowSelect)  
  img = CreateImage(#PB_Any, 16, 16)
    
  StartDrawing(ImageOutput(img))
  Box(0, 0, 16, 16, RGB(255, 0, 0))
  StopDrawing()
  AddGadgetItem(#List1,-1,"Red", ImageID(img))
  
  StartDrawing(ImageOutput(img))
  Box(0, 0, 16, 16, RGB(0, 255, 0))
  StopDrawing()
  AddGadgetItem(#List1,-1,"Blue", ImageID(img))  
  
  Repeat : Until WaitWindowEvent() = #PB_Event_CloseWindow
EndIf

=


And here is my (simplified for demo) version of AddGadgetItem(), which DOES always call ImageList_Add (which is exactly what PB's AddGadgetItem() internally calls). It's basically exactly the same but of course i had to call ImageList_Create myself to get the handle:

Code: Select all

Procedure MyAddGadgetItem(Gadget, Position, Text$, imagelist, ImageId=0)
  Define item.LVITEM
  item\Mask = #LVIF_IMAGE: item\iItem = Position: item\iImage = Position
  AddGadgetItem(Gadget,-1,Text$)
  ImageList_Add_(imagelist, ImageId, 0)
  SendMessage_(GadgetID(Gadget), #LVM_SETITEM, 0, @item.LVITEM)  
EndProcedure
  

#List1=1: #Img1 =2
If OpenWindow(0,0,0,200,80,"Window",#PB_Window_SystemMenu|#PB_Window_ScreenCentered)
  
  imagelist.i = ImageList_Create_(16,16,#ILC_COLOR32,0,10)
  CreateImage(#Img1, 16, 16)    
  ListIconGadget(#List1,10,10,200,80,"Column",180,#PB_ListIcon_GridLines|#PB_ListIcon_FullRowSelect)  
  SendMessage_(GadgetID(#List1), #LVM_SETIMAGELIST, #LVSIL_SMALL, imagelist)

  StartDrawing(ImageOutput(#Img1))
  Box(0, 0, 16, 16, RGB(255, 0, 0))
  StopDrawing()  
  MyAddGadgetItem(#List1, 0, "Red", imagelist, ImageID(#Img1))
 
  StartDrawing(ImageOutput(#Img1))
  Box(0, 0, 16, 16, RGB(0, 0, 255))
  StopDrawing()  
  MyAddGadgetItem(#List1, 1, "Blue", imagelist, ImageID(#Img1))
  
  Repeat: Until WaitWindowEvent() = #PB_Event_CloseWindow
EndIf

So I don't know what more i can do to prove that this is just PB bug/limitation and that the OS clearly allows for this
I also don't see how this "cache feature" is actually beneficial in any way (is it faster drawing an existing image? they're both already in memory), especially given the restrictions i've previously listed that it imposes, and I also don't see how fixing it would break or otherwise negatively impact any existing code, as it seems like it was just intended as a "turbo boost switch" kinda thing - functionally the same but intended to behave faster because of caching. I can use this fix as a workaround on Windows, but I can't see any reason why PB's native AddGadgetItem can't do this - mine can, and this fix won't work on Linux or Mac.

Thankyou for your patience with me on this frustrating issue.

[Dude]

So here again is the current PB method, where for whatever reason it's not painting an image if the handle has been seen before, even if it's a new image

Hmm, that example is pretty compelling to me. Seems like a bug, because viewing the image in the "Library Viewer" definitely shows it to NOT be red anymore, yet it's painted red when added. And it's only two items being added to the list, not hundreds, so it can't be a cache problem. FreeImage() is not used, either, so it's not that.

[Keya]

Dude (is it ok if i call you Dood? hehe), yes and you can see the ImageList bitmap grow (in pixel height by approx 16 pixels with every ImageList_Add call) in GDIView, and SaveImage() also confirms the image is correctly red or blue. And what would be the point of an ImageList having a table of handles to other images when it's already got the list drawn on its own internal bitmap? And we've already proven images can be FreeImage'd and remain in the list. The ImageList does afterall allow for hundreds of thousands (im guessing resource-limited) list entries with icons, yet Windows imposes 10000 GDI handle process limit, but that limit isn't a problem because of ImageList's internal bitmap. I've run out of avenues to test having spent 4-5 days on this, and too many energy drinks to let my mother know about hehe. Thankyou for giving my theory a chance

Proof/example of 20000 icons in list - again no OS limitation, and fast enough to add. Each is FreeImage'd before next is CreateImage'd, otherwise we'd reach Windows max 10000 handle problem. BUT by freeing each image we now get a lot of handle collisions, which is why you can see in this image the pixel height ISNT quite as big as it should be due to this AddGadgetItem() feature-bug where it has linked some LV_ITEM\ImageId's to point to previous images rather than ImageList_Add'ing them. Everytime i run it it's a different height because of this. (See previous gdiview screenshots for correct size when there's no collisions, ie 4192 pixels high for 256 16x16 images = 16.375 pixels high each, slightly more than 16 im guessing due to alignment when it dyn-reallocs). So, problems with either way because of PB AddGadgetItem(), yet these problems are not imposed by the OS

Code: Select all

#List1=1
If OpenWindow(0, 0, 0, 270, 80, "ComboBoxGadget", #PB_Window_SystemMenu | #PB_Window_ScreenCentered)
  ComboBoxGadget(#List1, 10, 10, 250, 21, #PB_ComboBox_Image)
  
  For i = 1 To 20000
    CreateImage(0, 16,16)
    If StartDrawing(ImageOutput(0)) 
      Box(0, 0, 15, 15, RGB(Random(255,0), Random(255,0), Random(255,0))) ;red
      StopDrawing()
    EndIf
    AddGadgetItem(#List1, -1,"Red #"+Str(i), ImageID(0))
    FreeImage(0)
  Next i

  Repeat : Until WaitWindowEvent() = #PB_Event_CloseWindow
EndIf

[Dude]

Even this example, with ONE use of StartDrawing(), just adds two BLACK boxes to the gadget

Code: Select all

#List1=1

If OpenWindow(0,0,0,200,80,"Window",#PB_Window_SystemMenu|#PB_Window_ScreenCentered)

  ListIconGadget(#List1,10,10,200,80,"Column",180,#PB_ListIcon_GridLines|#PB_ListIcon_FullRowSelect)

  img = CreateImage(1, 16, 16)

;   StartDrawing(ImageOutput(1))
;   Box(0, 0, 16, 16, #Red)
;   StopDrawing()

  AddGadgetItem(#List1,-1,"Red", ImageID(1))

  StartDrawing(ImageOutput(1))
  Box(0, 0, 16, 16, #Blue)
  StopDrawing()

  AddGadgetItem(#List1,-1,"Blue", ImageID(1))

  Repeat : Until WaitWindowEvent() = #PB_Event_CloseWindow

EndIf