Bug 341282

Comment 1 Felipe Heidrich 2011-03-29 16:41:27 EDT

I'm assigning this bug to Helena Halperin since she was the developer of setOrientation() for windows.
Please provide a fix ASAP.

Comment 2 Felipe Heidrich 2011-03-29 17:00:45 EDT

Created attachment 192140 [details]
sample patch for tabfolder

This is the fix for TabFolder, we believe the same pattern can be used to fix Table, Tree, Button, Toolbar (not sure about Menu, apparently image lists are only used in menu for wince).

Comment 3 Felipe Heidrich 2011-03-29 17:05:48 EDT

Created attachment 192142 [details]
sample patch for tabfolder

SSQ pointed out a problem on the first patch, we can't relie on the size of the first image in the array to init the new ImageList, we need to use the size o the old image list instead.

Comment 4 Felipe Heidrich 2011-03-30 09:40:21 EDT

Created attachment 192186 [details]
patch for tabfolder

My apologies, the last code has a error, the image list can't be set in the control (TCM_SETIMAGELIST) till the size is set (or till the first image is added). The patch changes ImageList to init the size in the constructor.

Comment 5 Felipe Heidrich 2011-03-30 09:42:54 EDT

I released the last patch so that Helena can use the new code to fix the other controls.

Comment 6 Helena Halperin 2011-03-30 10:48:45 EDT

Created attachment 192199 [details]
patch for button, tree and table

This is patch for Button, Tree and Table. Please review

Comment 7 Felipe Heidrich 2011-03-30 11:43:41 EDT

(In reply to comment #6)
> Created attachment 192199 [details]
> patch for button, tree and table
> 
> This is patch for Button, Tree and Table. Please review

For button, the code that recreate the image list in updateOrienatation and enableWidget are basically the same, you should be able to share that code by refactoring the code in a new method.


Table and Tree.
Mostly fine, except that in some cases the columns have the index to the image in the column struct (HDITEM#iImage). This index is not being update to the index of the image in the new image list.

Comment 8 Helena Halperin 2011-03-31 09:51:51 EDT

Created attachment 192271 [details]
a new patch for button, tree, table and toolbar

Button - added a new function "updateImageList" for shared code

Tree and Table - added code to update HDITEM

Toolbar, added in this patch. I used item.updateImages to resolve the problems with setOrientation for disabled toolbar

Comment 9 Felipe Heidrich 2011-04-11 15:27:03 EDT

Table and Tree are wrong.
- does not handle the case OS.COMCTL32_MAJOR >= 6 (Table)
- You should only set hdItem.iImage when hdItem.fmt == OS.HDF_IMAGE.
- int colIndex = indexOf (column); looks wrong, isn't "i" always equals to colIndex in your code ? See the implementation for indexOf (column)
- Likewise, "if (colIndex > -1)"  can that ever be false ?

- Table only, since the items array in Table can be compressed (see bug 129703) no code should be accessing the items in the array directly. Use this code instead:
int count = (int)/*64*/OS.SendMessage (handle, OS.LVM_GETITEMCOUNT, 0, 0);
for (int i=0; i<count; i++) {
	TableItem item = _getItem (i, false);
	if (item != null) {
...



ToolBar also looks wrong.
- Why do you call item.updateImages (item.isEnabled ()); ?
It seems you are doing the work twice. Besides, item.isEnabled() is wrong here.

-if (item == null) break; is wrong as you can have null in the middle of the array.

Comment 10 Felipe Heidrich 2011-04-11 15:34:03 EDT

fixed for Button, I changed the patch I bit, no need to save the size of the previous image list cause buttons can only have one image. The code in updateImageList() is exactly the code that was at end of enableWidget().

Comment 11 Helena Halperin 2011-04-17 13:00:33 EDT

Created attachment 193438 [details]
patch for tree, table and toolbar

changes for tree, table and toolbar

Comment 12 Felipe Heidrich 2011-04-19 12:16:38 EDT

Fixed in HEAD
Changes for Table, Tree:
1)
for (int i = 0; i < count; i++) {
	TableItem item = _getItem (i, false);
	if (item != null) image = item.image;
	if (image != null) {
		...
	}
}

This code is wrong, the value in the variable image is not reset for each iteration of the loop.
I fixed it for items and columns in Table  (in Tree you used the right code).

2)
I moved the "if (hwndHeader != 0) {" outside of the for statement over the columns array. No point doing anywork if hwndHeader==0.

3)
You were adding all column images to the imageList, the image should only be added to the imageList 
only when (hdItem.fmt & OS.HDF_IMAGE)!= 0.


Toolbar, I removed part of the code I think was not necessary (hopefully I didn't break anything).

Comment 13 Helena Halperin 2011-04-27 08:43:29 EDT

This version of toolbar  code does not work correct for disabled toolbar. To test, disable toolbar and switch orientation between RTL and LTR. Images disappeared. The reason is a desynchronization between imageList and disabledImageList. "Add" function, applied to imageList and disabledImageList,  returns different indexes in several  cases. This is because of  item.disabledImage2.dispose (), that   disposes the image in image list, and function "add" puts the image in this place. This is the reason for use "add" or "put" image in different cases.

Another issue , in TabFolder, this code looks incorrect
	for (int i = 0; i < items.length; i++) {
			TabItem item = items[0];
			if (item == null) break;
			Image image = item.image;
...

Comment 14 Felipe Heidrich 2011-05-02 10:28:19 EDT

(In reply to comment #13)
> This version of toolbar  code does not work correct for disabled toolbar. To
> test, disable toolbar and switch orientation between RTL and LTR. Images
> disappeared. The reason is a desynchronization between imageList and
> disabledImageList. "Add" function, applied to imageList and disabledImageList, 
> returns different indexes in several  cases. This is because of 
> item.disabledImage2.dispose (), that   disposes the image in image list, and
> function "add" puts the image in this place. This is the reason for use "add"
> or "put" image in different cases.

See Bug 344392

> 
> Another issue , in TabFolder, this code looks incorrect
>     for (int i = 0; i < items.length; i++) {
>             TabItem item = items[0];
>             if (item == null) break;
>             Image image = item.image;
> ...

I checked the code and that items array can only have null at the end. So I think the code is fine.
am I wrong, do you have case that shows a problem ?

Comment 15 Helena Halperin 2011-05-03 10:03:35 EDT

(In reply to comment #14)
> > Another issue , in TabFolder, this code looks incorrect
> >     for (int i = 0; i < items.length; i++) {
> >             TabItem item = items[0];
> >             if (item == null) break;
> >             Image image = item.image;
> > ...
> I checked the code and that items array can only have null at the end. So I
> think the code is fine.
> am I wrong, do you have case that shows a problem 

Sorry, I was not clear

Should not it be 
TabItem item = items[i];
instead of  
TabItem item = items[0]; 
?
After "setOrientation", the TabFolder with different images dispalys the image of the first tab in all tabs.

Comment 16 Felipe Heidrich 2011-05-03 10:42:16 EDT

(In reply to comment #15)
> Should not it be 
> TabItem item = items[i];
> instead of  
> TabItem item = items[0]; 
> ?

Yes, definitely.
Thank you for point this out. Released in HEAD.