STR #2231: Browser doesn't recalculate column width when item has an image

STR #2231

Application:	FLTK Library
Status:	5 - New
Priority:	3 - Moderate, e.g. unable to compile the software
Scope:	3 - Applies to all machines and operating systems
Subsystem:	Unassigned
Summary:	Browser doesn't recalculate column width when item has an image
Version:	2.0-feature
Created By:	isaque
Assigned To:	Unassigned
Fix Version:	Unassigned
Update Notification:	Receive EMails Don't Receive EMails

Trouble Report Files:

Post File

#1	isaque 21:25 Jul 30, 2009

column_shift.diff
2k

#2	isaque 19:49 Aug 19, 2009

column_shift_v2.diff
2k

Trouble Report Comments:

Post Text

#1	isaque 21:25 Jul 30, 2009

Whenever you have a multi column browser and you add an image to an item, like this: Browser b(...); Widget* item=b.add(...); item->image(an_image); The image shifts the whole browser line and make them skewed. I've created a patch that fixes that.

#2	isaque 12:39 Aug 03, 2009

Please use test/browser.cxx to test it.

#3	isaque 12:34 Aug 18, 2009

This patch seems to be working fine for quite some time. Anyone can apply it to the code? Thanks.

#4	greg.ercolano 15:53 Aug 18, 2009

Hmm, I'm just looking over this patch, and am wondering about the last bit of code: + + // Restore column width + if (cols) cols[0] = saved_colw; It /seems/ like the conditions that that initialize 'saved_colw' are possibly different than the condition that restores the value. ie. saved_colw is effectively set on the conditions of: if ( img && !(flags & ALIGN_CENTER) && (flags & ALIGN_LEFT) && cols) ..but saved_colw is saved back into cols[0] without any of those conditions needing to be met, meaning cols[0] might end up as zero. Is that correct?

#5	greg.ercolano 16:01 Aug 18, 2009

Or in other words: ---- snip void Widget::draw_label(const Rectangle& ir, Flags flags) const { int saved_colw = 0; // VALUE INIT TO ZERO .. if (img) { .. if (flags & ALIGN_CENTER) { .. } else { .. if (flags & ALIGN_LEFT && cols) { saved_colw = cols[0]; // ACTUAL VALUE SAVED HERE ONLY .. } } } .. if (cols) cols[0] = saved_colw; // RESTORE USES DIFFERENT LOGIC ---- snip

#6	isaque 08:33 Aug 19, 2009

Yeah, it's correct. I'm fixing that and I'll provide a new patch. Thanks for catching that.

#7	isaque 19:49 Aug 19, 2009

Fixed the issue pointed by Greg - thanks again man!

#8	AlbrechtS 04:23 Aug 20, 2009

Honestly said: I don't think that the provided patch is acceptable. You change Widget::draw_label() to achieve a better layout for a special case in the browser widget. What else may this affect? It's not enough to verify that it solves _your_ problem... Also, I don't understand these lines: // Shift first column width, so labels after 1st column are lined up correctly. if (flags & ALIGN_LEFT && cols) { cols[0] -= ir.w(); if (cols[0]==0) cols[0]--; } First point: what, if ir.w() > cols[] ? Are negative cols values acceptable? Second point: I can see why there is the special case "if (cols[0]==0)", but why do you decrement (and not e.g. increment) the cols value, resulting in -1 (again a negative value). That said, I'm not a FLTK 2 developer, I only read the code, and I may be wrong with this...

#9	AlbrechtS 04:26 Aug 20, 2009

Correction: "First point" should read: "what, if ir.w() > cols[0] ? ..."

#10	isaque 06:58 Aug 20, 2009

I used the same code that's used by Browser code to draw the plus sign and lines in the a treeview browser. The idea for cols[0]-- is to make it negative and make the column resizable. The code was used only when the image is on the left of the browser line because it was where the problem happens. I had to make the change on Widget_draw.cxx because it's there where the problem is. This code is responsible for all label draw routine, including the image draw.

#11	isaque 07:02 Aug 20, 2009

The way this patch was coded, it only affects this case where it's a multicolumn label (browser case) with an image on the left. No other place was impacted. The issue pointed by Greg was fixed and I can't see any other impact besides that, so if you find any other issue, please let me know and I'll try to fix that as well.

#12	AlbrechtS 07:50 Aug 20, 2009

What about this? After your patch, in line #246, there is: if (!img) return; // don't call pop_clip if push_clip was not called And in line #252: if (cols) cols[0] = saved_colw;

#13	AlbrechtS 07:59 Aug 20, 2009

Oh, sorry, I didn't see that cols[0] is modified only if img is true, thus my last point is moot. But I still don't know if your code can't have side effects on other widgets (label drawing), where your special alignment is *not* wanted. Maybe some FLTK 2 developers should check that.