Page 2 of 3

Re: AddGadgetItem image bug, all OS

Posted: Fri Feb 10, 2017 6:48 am
by Keya
This thread has gotten long and as I've exhausted all avenues and believe I've proven beyond doubt that this is a PB problem, but still have doubters, please allow me to conclude my bug report, and what i've learned since encountering this problem 4-5 days ago and having to get stuck into it.

THE PROBLEM: AddGadgetItem(), which internally calls ImageList_Add (using Windows for this example but problem is also consistent on Linux and Mac), fails to call it if it detects the handle has been used before, instead incorrectly linking to that previous handle's instance of the image, which is the wrong image. (I believe this is a well-meaningfully intended cache-style feature, but unfortunately has turned out nothing but problematic).

How I believe it can be fixed: Simply always get AddGadgetItem to call ImageList_Add. (Or something like a #PB_NoImageCache parameter).
I can only guess, but I'd imagine it would be quick and trivial to fix.
Positives: Fixes every issue in this thread.
Negatives: None - I cannot see how it could have any negative effect or 'break' any existing PB code, because all it would be doing is drawing the intended image instead of the one from cache. The intended purpose of reusing existing images is no longer there, but this issue suggests that if that is required it should and must be done by the programmer, not the compiler.

Who is affected: Everyone using a List/Combo gadget with icons added by PB's AddGadgetItem() (but not native ie winapi). Win+Linux+Mac. Seemingly all PB versions.

There are two types of use, and both are affected but in slightly different ways:

1) those who use CreateImage() with every icon/AddGadgetItem, but keep the image in memory (no FreeImage). (Max correct images = <10000, but then big problems).
Problem 1: You cannot create more than 10000 images in Windows due to the handle limit (although you won't experience handle collisions), even though ImageList supports hundreds of thousands+ images.
Problem 2: Because the ImageList only uses its own independent bitmap those images do nothing but waste memory and handles - if you have 10000 images open you can't even open a file let alone another image, yet because ImageList's bitmap is independent there is no reason to keep the handles open and images wasting memory.
Problem 3: Speed. Because you have to call CreateImage for every icon its a lot slower, especially when adding 10000+ images. When using dynamically-created images 1 image is usually enough, and that's a lot faster.

2) those who use CreateImage() with every icon/AddGadgetItem, but then FreeImage() after adding to the list. (Max correct images <256)
Problem: You'll experience handle collisions in Windows because of this AddGadgetItem() bug/feature/limitation. If using 256 images in your list you'll experience this approximately every 2nd-3rd execution with typically 1-10 collisions. If using more than 512 images you'll almost always experience this problem, and it's tricky to notice because it's still using valid (but wrong/old) icons. (Tested XP32 and Win7-64, handle collisions the same on both)

Both of these approaches have restrictions imposed on them by AddGadgetItem(), even though the OS imposes no such restrictions and supports 1000000+ images, yet in neither case can you correctly add 10,000 icons to your list, and in the 2nd/FreeImage approach you can rarely even add 256 icons before encountering the handle collision problem. (And please don't say "well you shouldn't be using 256 or 10000 images" (freak suggested that i'm a special case), because the OS is perfectly fine with it, and every app is different with its own needs as to what gadgets work best etc for the specific task at hand - Windows Explorer for example has no problem with a folder having tens of thousands of files each with their own icon in its SysListview, and if there was a better way to display it im sure MS would've done so - and Explorer clearly doesn't have thousands of image handles open).

Are there any significant benefits to this apparent feature?
All I can think of, is it was well-meaningfully intended to save memory by using the existing version of an image if it detects a previously-used handle. (The LV_ITEM structure for example lets you link to any index in the imagelist ie. to re-use images if needed, but because AddGadgetItem doesn't have a parameter for this/doesn't expose the ImageList itself i'm guessing Fred with his heart in the right place added his own implementation of that to support previous images - sounds good on paper!). However that logic is flawed as 1) handles can be the same even when FreeImage'd and CreateImage'd and different location in memory (that's how i first found the problem), and 2) existing images can be overdrawn with new content, which is perfectly valid, yet this feature doesn't recognise them as new images. Not only does it impose these problems, it also means we can't obtain maximum speed due to having to CreateImage with every icon, when in the case of dynamically-generated images 1 is enough (and i've timed it, it's a lot faster). It's only 16x16 anyway (an ImageList with 256 images = a bitmap of only 16x4192 pixels = 201216 bytes 24bit RGB), so there's not much saving anyway, and this (previous handles) is something the programmer can easily detect with a simple integer array + IsImage() if they wanted to implement their own handle-reuse system.

I believe I have proven beyond reasonable doubt:
1: PB's AddGadgetItem() internally calls ImageList_Add, but not when it detects a handle has previously been used. That's the problem.
2: ImageList_Add does however correctly add the new image even if it has a previously-used handle (just not when AddGadgetItem() doesn't call it!)
3: ImageList's bitmap is independent of the images it is fed. When you ImageList_Add it grows its internal bitmap in size by 16 pixels height, and paints your image into its bitmap.
4: CreateImage()'d images that are added can be FreeImage'd immediately after being added to the ImageList, because they're now drawn onto the ImageList's internal bitmap. Clearly one of the reasons why it has its own internal bitmap is so that you don't need to keep thousands of handles open, as also supported by the fact that it supports hundreds-of-thousands+ of images, and the fact apps like Windows Explorer with tens of thousands of icons in their lists don't have tens of thousands of handles open.
5: ImageList as i've tested supports hundreds of thousands of images (im guessing resource-limited or 32bit ceiling).
6: My version of AddGadgetItem shows it can easily be done, and not by doing anything special as we're basically doing exactly the same thing. There should be no need to reinvent the wheel or create my own custom Canvasgadget item. The only real difference between AddGadgetItem and MyAddGadgetItem is that mine always called ImageList_Add, whereas PB's AddGadgetItem() only calls it if it hasn't seen the handle id before.
7: While well-meaning, this feature doesn't really offer much anyway (just a tiny memory saving), yet creates all these problems.
8: This is a PB limitation/bug, one that seemingly doesn't need to or shouldn't be there, yet the OS does not impose any such restrictions/problems.

Thankyou for your time and giving me a chance.

Re: AddGadgetItem image bug, all OS

Posted: Fri Feb 10, 2017 9:49 am
by Fred
Fr34k explained it correctly, we use the ImageID() as parameter for the icon, which means than if you want to have different image, you need to create a new one and keep the handle to it. That's how it works for all 3 OSes (Linux and OS X doesn't have imagelist at all and we don't want to add such support). What you describe is very Windows specific and doesn't apply to other OSes and is not documented this way in the PB doc. We simply don't copy the image, we use it directly.

Re: AddGadgetItem image bug, all OS

Posted: Fri Feb 10, 2017 9:57 am
by Keya
we use the ImageID() as parameter for the icon, which means than if you want to have different image, you need to create a new one and keep the handle to it.
But what if the user updates the image and wants it redrawn!? Which is perfectly valid. And if you can't use the same image it just forces extra resource use that isn't required, plus the speed-loss and overhead of repeatedly calling CreateImage.

And again, even when i DO use FreeImage() and then call CreateImage() we get the handle collision problem, so no we can't just "create a new one".

And ImageList_Add DOES correctly draw the new image even if it's the same handle - you just have to call it! which AddGadgetItem does - but doesn't if its seen the handle before. Which causes problems.
We simply don't copy the image, we use it directly
In this case AddGadgetItem is not using it directly, it's linking to the old handle. My version of AddGadgetItem demonstrates that this is not an issue, and that this restriction is being imposed by AddGadgetItem seemingly for no reason other than to save memory, but at the cost of causing these problems.

It's preventing ImageList with list/combogadgets from being used as it was clearly intended, like Windows Explorer uses it. Only PB is limited to <256 images or <10000 images depending on method, both of which are unnecessarily restrictive and problematic.

My version of AddGadgetItem works, so why can't PB's? Afterall mine is just an attempted ripoff of yours - we're both simply just calling ImageList_Add to add icons, but PB's isn't if it's used the handle before.

That's the only problem - if you called ImageList_Add for every call to AddGadgetItem(), as my demo does, there'd be no problem.

So despite all these problems why are you still in favor of not calling ImageList_Add for each AddGadgetItem? what exactly is the benefit?

I don't understand how imposing restrictions that the OS doesn't is a beneficial feature?!? How is not being able to correctly use 256 icons good in any way? And is saving a few kilobytes of 16x16 icons really worth all these problems and unnecessary limitations?? I am absolutely roadblocked by this, so how much do I need to pay to have this bug/proven-unnecessary limitation fixed??

Re: AddGadgetItem image bug, all OS

Posted: Fri Feb 10, 2017 10:19 am
by netmaestro
Someone needing 256 separate and distinct icons is to say the least an outlier case. In 40 years of coding I've never run across an application that requires it. I mean no offense and I hope you work it out successfully but the reality is that when they move it to coding questions they aren't listening anymore. From here API is your way forward, and I know you will have no trouble doing it that way. I wish you luck :mrgreen:

Re: AddGadgetItem image bug, all OS

Posted: Fri Feb 10, 2017 10:21 am
by Keya
Adding 15000 icons, with just the 1 image, without the collision problem (as im always calling ImageList_Add), and without Windows 10000 handle limitation due to 1 image ... nothing special, just always calling ImageList_Add while AddGadgetItem doesn't:

Code: Select all

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

#List1=1: #Img1 =2
If OpenWindow(0,0,0,300,100,"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)
  
  For i = 0 To 15000 
    StartDrawing(ImageOutput(#Img1))
    color = RGB(Random(255,0), Random(255,0), Random(255,0))
    Box(0, 0, 16, 16, color)
    StopDrawing()  
    MyAddGadgetItem(#List1, i, "Item "+Str(i)+ "=#"+Hex(color), imagelist, ImageID(#Img1))
  Next i
  
  Repeat: Until WaitWindowEvent() = #PB_Event_CloseWindow
EndIf

Re: AddGadgetItem image bug, all OS

Posted: Fri Feb 10, 2017 10:23 am
by Keya
netmaestro wrote:Someone needing 256 separate and distinct icons is to say the least an outlier case. In 40 years of coding I've never run across an application that requires it.
Here's two simple examples: 1) a list of files with icons (like Windows Explorer, which has no problems with tens of thousands of files in its listview), and 2) a list of colors in a large palette. Please don't say I shouldn't just because you haven't, especially when the operating system allows for it - only AddGadgetItem() doesn't, which is a problem unique to PB, and a problem I've shown doesn't need to exist, and again being a problem unique to PB highlights that. In some cases YES a list with a lot of icons is required and is the best approach - it was fine for Microsoft, and yes that's what my I need to do in my app, and I don't believe that having 256 icons in a list makes my case special.

My 'MyAddGadgetItem' essentially does exactly what PB's should be doing, and it does nothing special other than doing what AddGadgetItem should be doing -- adding the gadget item by calling ImageList_Add.

Re: AddGadgetItem image bug, all OS

Posted: Fri Feb 10, 2017 10:28 am
by Fred
PureBasic is a crossplatform programming language, with unified commandset. Underlyings aren't taken in account when creating commands, what you describe is only valid on Windows because we use this API. But as Fr34k said, what will happen if we decide to go ownerdraw for ListIconGadget() ? ImageList will be removed and we would use the imageid() directly, like on 2 other OS. Also if we change it on Windows, we also need to change it on Linux and OSX (create our own imagelist ?!). Indeed you can still use API as you did if you want to handle this specific case.

Re: AddGadgetItem image bug, all OS

Posted: Fri Feb 10, 2017 10:31 am
by Keya
I don't want to take too much of your time Fred so please just answer this one question - why not call ImageList_Add for each call to AddGadgetItem() ? As i've proven works beautifully. None of these problems then exist, and we get to add more than 256 icons to a list without issue.

Re: AddGadgetItem image bug, all OS

Posted: Fri Feb 10, 2017 10:32 am
by Fred
Because it doesn't work like that on Linux and MacOS and will create inconsistency which we don't want.

Re: AddGadgetItem image bug, all OS

Posted: Fri Feb 10, 2017 10:36 am
by Keya
I can't see how it would create inconsistencies, just fix problems, and I still can't see any benefit to this approach, just problems and PB-unique restrictions. Im gutted you think these restrictions are good as its literally put my app on ice, but can you then please tell me which linux and mac API youre using to add an entry to the listbox? as I will obviously have to create my own fix to this restriction now. There's no two ways around it, i need more than 256 icons in a listbox and yes it's the best way to display this data, similar to Windows Explorer. I will of course make it available to everyone. Already solved Windows.

Re: AddGadgetItem image bug, all OS

Posted: Fri Feb 10, 2017 10:46 am
by Keya
netmaestro wrote:I mean no offense and I hope you work it out successfully but the reality is that when they move it to coding questions they aren't listening anymore.
Yep. My app just became Windows-only, and PB users will forever not be able to use AddGadgetItem with 256 icons (actually less but thats a good level where collisions start appearing) correctly in Windows, despite being purely a PB-only restriction.

It's not often a feature only causes problems and no benefits.

btw this PB limitation means that AddGadgetItem can NEVER be called reliably on Windows if FreeImage has EVER been called. (it also affects Linux and Mac - new images with the same handle wont be drawn, but they dont seem to reuse handle ids as much as Windows)

Re: AddGadgetItem image bug, all OS

Posted: Fri Feb 10, 2017 11:23 am
by Dude
netmaestro wrote:Someone needing 256 separate and distinct icons is to say the least an outlier case.
Nah, it depends on what you're doing. A file search app that shows a list of all files on your PC with their icons will go far beyond 256 separate and distinct icons easily. And no, that's not an unreasonable or unusual example at all. :)

Re: AddGadgetItem image bug, all OS

Posted: Fri Feb 10, 2017 11:24 am
by Keya
thankyou Dude, i feel i'm living in an alternative universe at the moment hehe :(

Re: AddGadgetItem image bug, all OS

Posted: Fri Feb 10, 2017 11:27 am
by Dude
I just want to know why my post below doesn't work, and what can be done to fix it. Surely you're not saying we need an API solution for it? That's not right at all. It's using all PureBasic native commands and it fails.

http://www.purebasic.fr/english/viewtop ... 00#p502300

Re: AddGadgetItem image bug, all OS

Posted: Fri Feb 10, 2017 11:34 am
by Keya
Because when you called AddGadgetItem the first time it was with an empty (all 00's, and therefore Black) icon.
When you then painted the box a color and called AddGadgetItem the image handle was the same, therefore ImageList_Add wasn't called and instead the old image was used, so the ImageList (its internal Bitmap, basically growing 16 pixels in height for every icon added) looks like this:

Code: Select all

 [first 16x16 bytes = black icon]  ;the one and only icon in the list
And the gadget's list looks like this:

Code: Select all

 Item[0]\imgindex=0 ;point to 1st image
 Item[1]\imgindex=0 ;also point to 1st image
Whereas if ImageList_Add was called for both icons (like in my demo) you'd get this:

Code: Select all

 [1st 16x16 bytes = black icon]  ;first icon
 [2nd 16x16 bytes = blue icon]  ;second icon

 Item[0]\imgindex=0 ;point to 1st image
 Item[1]\imgindex=1 ;point to 2nd image
Dude wrote:Surely you're not saying we need an API solution for it?
I believe that's what Fred is saying. Even though my API version of AddGadgetItem is essentially what i thought PB's AddGadgetItem was doing, but with my version always calling ImageList_Add. That's the only real difference, as i also verified in a debugger while pulling my hair out over the past 4-5 days

Yep it's going to be fun figuring out how to reinvent this wheel with Linux and Mac API. Very disheartened

Anyway I thank Fred for his valuable time.