Aleksey Bragin wrote:
Fix bitmap size calculation in IMAGELIST_InternalExpandBitmaps and remove unneded parameter. Memory requirements are greatly reduced after this fix.
The bug was an ambiguous meaning of a cx parameter, which was supposed to be a width of an individual image inside the list, but width of a whole bitmap was passed in thus making memory requirements huge (number of images * width of whole bitmap = required bitmap size). Furthermore, cx parameter is redundant because all checks are already there (that the image list can't be reduced), and besides of that there is no use for it anymore. Thus it's been removed.
That bug was introduced while attempting to change the size of the bitmaps for the toolbar. Looking at the code it's rely iffy if image list really should be able to do that at all. Or that toolbar should handle the resize itself (most likely).
The commit I'm talking about is: commit 19cef6ca1059140d00ac8b445ab3f9eb0491f239 Author: Gerard Patel g.patel@wanadoo.fr Date: Sat Jul 8 12:48:37 2000 +0000
Allow the size of bitmaps to be changed after toolbar buttons have been added.
With your patch you "disable" this ability (which I don't think is working anyway). So some tests are in order. At least a test program demonstrating what's the correct behavior.
Vitaliy
On Mar 27, 2009, at 3:39 AM, Vitaliy Margolen wrote:
Aleksey Bragin wrote:
Fix bitmap size calculation in IMAGELIST_InternalExpandBitmaps and remove unneded parameter. Memory requirements are greatly reduced after this fix.
The bug was an ambiguous meaning of a cx parameter, which was supposed to be a width of an individual image inside the list, but width of a whole bitmap was passed in thus making memory requirements huge (number of images * width of whole bitmap = required bitmap size). Furthermore, cx parameter is redundant because all checks are already there (that the image list can't be reduced), and besides of that there is no use for it anymore. Thus it's been removed.
That bug was introduced while attempting to change the size of the bitmaps for the toolbar. Looking at the code it's rely iffy if image list really should be able to do that at all. Or that toolbar should handle the resize itself (most likely).
The commit I'm talking about is: commit 19cef6ca1059140d00ac8b445ab3f9eb0491f239 Author: Gerard Patel g.patel@wanadoo.fr Date: Sat Jul 8 12:48:37 2000 +0000
Allow the size of bitmaps to be changed after toolbar buttons have been added.With your patch you "disable" this ability (which I don't think is working anyway). So some tests are in order. At least a test program demonstrating what's the correct behavior.
Vitaliy
Yes exactly, however I didn't provide any testcase, because I just removed non-used parameter and fixed existing code. If someone wants to implement image list having variable width of each image - he's free to proof that Windows does this (which I very much doubt).
Anyway, e.g. Sib Icon Edit required ~ 30 Mb (something like 27 images * 13440 width * 24 height * sizeof(each pixel)) of memory for every instance to run (due to this bug). Now it requires only about 300kb :-).
Aleksey Bragin wrote:
On Mar 27, 2009, at 3:39 AM, Vitaliy Margolen wrote: Yes exactly, however I didn't provide any testcase, because I just removed non-used parameter and fixed existing code. If someone wants to implement image list having variable width of each image - he's free to proof that Windows does this (which I very much doubt).
I'm afraid you are the one to prove the code in Wine is wrong. A simple test app should do. I don't think we need a wine test that demonstrates that something doesn't work.
Anyway, e.g. Sib Icon Edit required ~ 30 Mb (something like 27 images * 13440 width * 24 height * sizeof(each pixel)) of memory for every instance to run (due to this bug). Now it requires only about 300kb
Fixing one app doesn't mean it's correct. Yes, there is an obvious error in the image list, but you are not just fixing it. You also removing some functionality that tool bar depends on.
Vitaliy.
Anyway, e.g. Sib Icon Edit required ~ 30 Mb (something like 27 images * 13440 width * 24 height * sizeof(each pixel)) of memory for every instance to run (due to this bug). Now it requires only about 300kb
Fixing one app doesn't mean it's correct. Yes, there is an obvious error in the image list, but you are not just fixing it. You also removing some functionality that tool bar depends on.
The toolbar doesn't need it anymore - currently after a button size change, TOOLBAR_CheckImageListIconSize (called from TOOLBAR_Refresh) destroys the imagelist and creates a new one. It seems that even before, it was resizing the bitmap using ImageList_SetIconSize (however only when no images were added).
Mikołaj
On Mar 27, 2009, at 6:47 PM, Vitaliy Margolen wrote:
Anyway, e.g. Sib Icon Edit required ~ 30 Mb (something like 27 images * 13440 width * 24 height * sizeof(each pixel)) of memory for every instance to run (due to this bug). Now it requires only about 300kb
Fixing one app doesn't mean it's correct. Yes, there is an obvious error in the image list, but you are not just fixing it. You also removing some functionality that tool bar depends on.
I'm not fixing an app (not one, but every which uses this codepath, might be not many but in Wine you would not notice this due to different memory model than in a real OS), I'm fixing a wrong behavior when width of whole bitmap was confused with width of one item in the image list! Simple formula: NewTotalWidth = NewItemCount * WidthOfOneItem (correct one), and now in Wine: NewTotalWidth = NewItemCount * TotalWidth.
Then, after fixing, cx parameter becomes unused and thus gets deleted. There was no way of changing individual image list item's width with that cx parameter. The only possibility of its usage might have been checking if image list is going to be reduced instead of expanded, but that check is there, and is performed without using cx.
Am I wrong?
-
Aleksey Bragin -
Mikołaj Zalewski -
Vitaliy Margolen