Bug 215997 - [JFace] Table grows on reflow when using TableColumnLayout in a Forms Section
Summary: [JFace] Table grows on reflow when using TableColumnLayout in a Forms Section
Status: CLOSED WONTFIX
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.3.1   Edit
Hardware: PC Windows XP
: P3 normal with 27 votes (vote)
Target Milestone: ---   Edit
Assignee: Platform UI Triaged CLA
QA Contact:
URL:
Whiteboard: stalebug
Keywords: helpwanted
Depends on:
Blocks:
 
Reported: 2008-01-21 10:49 EST by Will Horn CLA
Modified: 2020-07-31 02:00 EDT (History)
36 users (show)

See Also:


Attachments
zipped project testcase (10.45 KB, application/zip)
2008-01-25 16:07 EST, Adam Archer CLA
no flags Details
take scrollbars into account (1.01 KB, patch)
2008-08-25 13:34 EDT, Thomas Schindl CLA
no flags Details | Diff
mylyn/context/zip (7.95 KB, application/octet-stream)
2008-08-25 13:35 EDT, Thomas Schindl CLA
no flags Details
Screenshot showing used area with patch applied (21.00 KB, image/png)
2008-08-25 13:38 EDT, Thomas Schindl CLA
no flags Details
mylyn/context/zip (7.95 KB, application/octet-stream)
2008-08-25 13:38 EDT, Thomas Schindl CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Will Horn CLA 2008-01-21 10:49:25 EST
Build ID: 3.3.1.1

Steps To Reproduce:
See http://dev.eclipse.org/newslists/news.eclipse.platform.swt/msg36119.html

The constructor in Snippet016TableLayout can be modified as follows to demonstrate this bug whenever a key is pressed.

public Snippet016TableLayout(Composite comp)
    {
        FormToolkit toolkit = new FormToolkit(comp.getDisplay());
        final ScrolledForm form = toolkit.createScrolledForm(comp);
        form.getBody().setLayout(new FillLayout());
        Section section = toolkit.createSection(form.getBody(), Section.EXPANDED |
                                                                Section.TITLE_BAR);
        section.setLayout(new FillLayout());

        final Composite client = toolkit.createComposite(section, SWT.NO_FOCUS);
        section.setClient(client);

        final TableViewer v = new TableViewer(new Table(client, SWT.BORDER));
        v.setLabelProvider(new MyLabelProvider());
        v.setContentProvider(new MyContentProvider());
        v.getTable().setHeaderVisible(true);

        TableColumnLayout ad = new TableColumnLayout();
        client.setLayout(ad);

        TableColumn column = new TableColumn(v.getTable(), SWT.NONE);
        column.setText("Column 1");
        column.setMoveable(true);
        ad.setColumnData(column, new ColumnWeightData(90, 290));

        column = new TableColumn(v.getTable(), SWT.NONE);
        column.setText("Column 2");
        column.setMoveable(true);
        ad.setColumnData(column, new ColumnWeightData(10, 200));

        MyModel[] model = createModel();
        v.setInput(model);
        v.getTable().setLinesVisible(true);

        v.getTable().addKeyListener(new KeyAdapter()
        {
            @Override
            public void keyPressed(KeyEvent e)
            {
                form.reflow(true);
            }
        });
    }

More information:
Comment 1 Thomas Schindl CLA 2008-01-21 11:02:27 EST
I think this is some problem when interacting between Forms and the layout. I think we should ask UA because we are using standard SWT-features here.
Comment 2 Adam Archer CLA 2008-01-25 16:07:59 EST
Created attachment 87914 [details]
zipped project testcase

I simplified the sample slightly to eliminate some of the complexity when debugging. I have attached a project containing the new sample.

I think the issue here is that the table is requesting more room than the parent has to offer. When this happens in a Composite, the composite does not listen to the table and gives it only the room available. When children of a ScrolledForm ask for more room than there is available, the form will accommodate them and add a horizontal scrollbar. The reason it continues to grow is because on the second reflow the form reports its current width as larger than before because it was grown to make room for the table. This pattern recurses endlessly.

To see the problem, open the view in the attached project (Sample Category > Sample View) and put a breakpoint in Table#computeSize(int, int, boolean). You will see that the width returned is always 20px larger than the width implied by the rect variable (rect.right - rect.left). On each subsequent key press the rect width grows by 16px resulting in a larger preferred width being reported by the table each time.

I debugged and recorded the values I found in Table#computeSize for three reflows. Here's the data:

Reflow#	rect delta	width result
1	743		763
2	759		779
3	775		795

where rect delta = rect.right - rect.left
Comment 3 Adam Archer CLA 2008-01-25 16:10:37 EST
Sending to SWT for consideration.
Comment 4 Eric Rizzo CLA 2008-01-26 18:19:38 EST
An additional observation that might help:
I've been able to work around this issue by introducing another Composite between the form body and the table, giving the form body a GridLayout, and then assigning a widthHint of 1 to the GridData of the new Composite (Table's parent). This eliminates the endless growing but in real form pages sometimes produces an unnecessary horizontal scroll bar on the table (it's not consistent, but it is frequent; I've not been able to determine the exact conditions that trigger it).
I think this supports the analysis in Comment 2, but I'm not 100% sure.
Here's the part of the code from the attachment that I modified to demonstrate:

public void createPartControl(Composite parent) {
	toolkit = new FormToolkit(parent.getDisplay());
    form = toolkit.createScrolledForm(parent);

    Composite container = toolkit.createComposite(form.getBody());
    form.getBody().setLayout(new GridLayout());
    GridData containerLD = new GridData(SWT.FILL, SWT.FILL, true, true);

    // This keeps the table from growing every time the form is reflow()ed
    containerLD.widthHint = 1;

    container.setLayoutData(containerLD);

    final TableViewer v = new TableViewer(new Table(container, SWT.BORDER));
    v.setLabelProvider(new MyLabelProvider());
    v.setContentProvider(new MyContentProvider());
    v.getTable().setHeaderVisible(true);

    TableColumnLayout ad = new TableColumnLayout();
    container.setLayout(ad);

    TableColumn column = new TableColumn(v.getTable(), SWT.NONE);
    column.setText("Column 1");
    column.setMoveable(true);
//	    ad.setColumnData(column, new ColumnPixelData(290));
    ad.setColumnData(column, new ColumnWeightData(90, 290));

    column = new TableColumn(v.getTable(), SWT.NONE);
    column.setText("Column 2");
    column.setMoveable(true);
//	    ad.setColumnData(column, new ColumnPixelData(290));
    ad.setColumnData(column, new ColumnWeightData(90, 200));

    MyModel[] model = createModel();
    v.setInput(model);
    v.getTable().setLinesVisible(true);

    v.getTable().addKeyListener(new KeyAdapter()
    {
        @Override
        public void keyPressed(KeyEvent e)
        {
            form.reflow(true);
        }
    });
}
Comment 5 Steve Northover CLA 2008-04-21 14:42:06 EDT
Duong to investigate.
Comment 6 Duong Nguyen CLA 2008-05-02 14:47:22 EDT
The problem is in AbstractColumnLayout layout methods. The layoutTable tree method will adjust the columns in table based on the current table width. The width of the table is the sum of the size of the columns + scrollbar width + borders. The table is computed in the layout code as:

	int tableWidth = table.getSize().x;
	int trim = computeTrim(area, table, tableWidth);
	int width = Math.max(0, area.width - trim);

This code adjusts the table width to take into account of the border (trim) but does not take into account of the scrollbars. So as the size of the columns are adjusted each time, they grow by the amount equivalent to the width of the scrollbar.

To fix this, the code should either compute the width by adding up the widths of the columns, or take the table width and subtract the trim width and the scrollbar width.

Moving to UA.
Comment 7 Adam Archer CLA 2008-05-06 09:25:17 EDT
AbstractColumnLayout is jface. Moving to Platform UI.
Comment 8 Trevor CLA 2008-07-25 22:32:56 EDT
Am I correct in understanding that this bug's root cause has been discovered is simply awaiting a solution? It appears a couple of possible solutions have already been suggested. What else (short of a patch) would it take to have this solved?
Comment 9 Boris Bokowski CLA 2008-07-25 23:02:01 EDT
Tom is away on vacation right now. It would help if he came back. ;)
Comment 10 Thomas Schindl CLA 2008-08-04 04:34:46 EDT
I'm going to look into this for 3.5 but because I'm just back from holiday I can't promise anything for now.
Comment 11 Thomas Schindl CLA 2008-08-04 04:37:25 EDT
by the way patches to implement the suggestion from comment #6 are highly appreciated.
Comment 12 Thomas Schindl CLA 2008-08-25 13:34:53 EDT
Created attachment 110824 [details]
take scrollbars into account

this patch takes the scrollbars into account when visible but I'm not sure this is correct because on OS-X it seems that the scrollbars are already part of the trim-calculation. Is that possible
Comment 13 Thomas Schindl CLA 2008-08-25 13:35:04 EDT
Created attachment 110825 [details]
mylyn/context/zip
Comment 14 Thomas Schindl CLA 2008-08-25 13:38:39 EDT
Created attachment 110826 [details]
Screenshot showing used area with patch applied
Comment 15 Thomas Schindl CLA 2008-08-25 13:38:50 EDT
Created attachment 110827 [details]
mylyn/context/zip
Comment 16 Wang Yizhuo CLA 2008-08-26 06:14:03 EDT
Thanks Tom for your patch.

I am facing the same problem. 

I tried the work around as comment #4 mentioned, but no luck. In order to use Tom's patch, I have to check out the source code of elipse itself, and redistribute and put it in my plugins folder rite, or there is an easier way that i am not aware of. 

Thanks.
Comment 17 Thomas Schindl CLA 2008-08-26 06:24:28 EDT
Duong, Steve could please look at the patch tell me what I'm doing wrong? I think the patch does what Duong suggested in comment #6. I don't think adding up the columns to calculate the appropriate width doesn't make any sense because that's exactly what this layout is supposed to calculate (the width of the columns :-).

I'm sorry that I this will not get fixed for 3.4.1 (without a small wonder) but I didn't had much free time to work on JFace lately because of my day job. I'm now assigning to 3.4.2.
Comment 18 Tobias Schwarz CLA 2008-08-27 04:17:40 EDT
hi all,

i observed a similar problem a while ago when using ColumnWeightData in a table inside a tab layout (in an eclipse launch dialog).
my problem was, that every time when i switched to another launch config, the dialogs gets wider.
i "workarounded" the problem by using a fixed layout size, thats computed each time when the layout was done the first time.

the problem i found out is, that the table calculates the size and adds the with of the scrollbar!! but in my oppinion this is something that should be done by the scrollable - the base class of a table.
so the table always uses the whole width including the scrollable width for the columns and than adds the scrollable width another time - so the table gets wider and wider.
the table should only use the custom area of the scrollable for the columns instead of the full client rect.
the table itself should not know anything about the scrollbars! this should all be done by the scrollable!

so i think this problem should not be workarounded another time! it should be fixed within the table and the scrollable widget.

i don't know if there are other widgets to, that have the scrollable as its base class and calculates their size in a wrong way?

best regards
tobias
Comment 19 Maciej Datka CLA 2008-10-17 03:03:27 EDT
I am also experiencing the problem. 
I brefly looked at the patch code and I don't think that is will help in my case. In my case the table is not showing the vertical scroll.

I also noticed that computeSize method of Table class always adds vertical scroll bar size to the total computed width - not sure if it's OK.
Well to be more precise it adds only when table style has SWT.V_SCROLL flag, but this flag is always added in checkStyle.
Anyway - it does it regardless if the scroll is visible or not.
(In my case it's not visible)
Comment 20 Steve Northover CLA 2008-10-17 10:25:40 EDT
Space for the scroll bars is added in even when they are not visible.  When items are added, on Windows, scroll bars are dynmically hidden and shown.  Even if we wanted to change the behavior of Table.computeSize(), we couldn't without breaking the universe.
Comment 21 Maciej Datka CLA 2008-10-17 10:33:22 EDT
Ok I understand. Something else is causing the growth then.
I also noticed similar growth on reflows - when org.eclipse.swt.browser.Browser is placed on form.

The workaround worked fine for me in both cases. 
(It case of Browser the GridData with widthHint=1 was assigned to it directly, not to the parent)
Comment 22 Thomas Schindl CLA 2008-12-17 16:22:51 EST
removeing 3.4.2 target - i recognize the high vote count but I don't have time to work on it :-(
Comment 23 Konstantin Komissarchik CLA 2009-06-10 21:20:30 EDT
Seeing the same problem. The error in the width calculation is causing a horizontal scrollbar to appear on my form section when I started experimenting with TableColumnLayout. Eric's workaround in Comment #4 worked for me.

Noticed something else that I think are artifacts of the same issue. Both of these went away when the workaround was applied.

1. Dragging the column separator to resize columns produced a high-frequency flickier of TABLE's horizontal scrollbar while the separator was being dragged. Once you stopped resizing, TABLE's scrollbar went away.

2. When the table transitions from not having a vertical scrollbar to having one (you've added a few items), the TableColumnLayout is not picking up on this and adjusting column widths accordingly. You get an unnecessary horizontal scrollbar in the TABLE.

In am running Vista.
Comment 24 Eric Rizzo CLA 2009-07-23 20:24:50 EDT
What is the current status of committer attention on this? It's quite old, has a snippet to reproduce it, as well as some seemingly detailed analysis already. Yes, I've found a work-around but it is ugly and it seems that plenty of people/apps have bumped into it.
Comment 25 Chris Goldthorpe CLA 2009-07-27 16:22:50 EDT
Assigning to Platform UI to evaluate the patch to org.eclipse.jface titled "take scrollbars into account".
Comment 26 Boris Bokowski CLA 2009-07-27 18:24:32 EDT
(In reply to comment #25)
> Assigning to Platform UI to evaluate the patch to org.eclipse.jface titled
> "take scrollbars into account".

Tom is a committer on Platform UI, and he attached the patch.
Comment 27 Thomas Schindl CLA 2009-07-27 19:13:58 EDT
Looking at the history of this bug - I'm very uncertain my patch is fixing the problem but I'm going to investigate it once more.
Comment 28 Mickael GAUVIN CLA 2009-07-29 04:04:25 EDT
Hello Tom, 
In my source code based on 3.5 I have a table without vertical bar so 
"width = Math.max(0, area.width - trim)"
is executed and the problem is still already here. 

---------------------------------------------------------------------------
Scrollable table = getControl(composite);
 		int tableWidth = table.getSize().x;
 		int trim = computeTrim(area, table, tableWidth);

		int width;
		
		if( table.getVerticalBar().isVisible() ) {
			width = Math.max(0, area.width - trim - table.getVerticalBar().getSize().x);
		} else {
			width = Math.max(0, area.width - trim);
		}
 
 		if (width > 1)
 			layoutTableTree(table, width, area, tableWidth < area.width);
--------------------------------------------------------------------------

Good luck, I hope you will find the solution.

Best regards.

Mickaël.
Comment 29 Thomas Schindl CLA 2010-01-28 17:13:09 EST
multi change because of intenion of stepping back as platform-ui committer
Comment 30 Nikolay Kasyanov CLA 2011-11-22 06:40:33 EST
any news here?
Comment 31 Susan McCourt CLA 2011-11-22 11:30:02 EST
(In reply to comment #30)
> any news here?

Sorry, there is no news.  There is no one actively working in this area right now.  The bug is marked "helpwanted" and we would make time to look at a patch.  As best as I can tell reading the history, Tom was unsure of his patch and did not think it would work on Mac.
Comment 32 David Smith-Uchida CLA 2013-05-22 12:29:52 EDT
I dug into this a bit.  I believe that the root of the problem is that Table.computeSize(int wHint, int hHint, boolean changed) add in the trim with a call to computeTrim().

I don't understand Table well enough to figure out if this is a bug or the "right" thing to do.

I did write up a nasty little kludge to computeTableTreeSize that removes the trim. I can't figure out how to get the trim values by themselves so I wound up iterating to get the solution.

Here's my version of AbstractColumnLayout with the kludge marked

	private Point computeTableTreeSize(Scrollable scrollable, int wHint,
			int hHint) {
		Point result = scrollable.computeSize(wHint, hHint);
		
		// Begin kludge to compensate for Table always adding in the trim margins
		// to the value returned from computeSize()
		int trueWidth = result.x;
		int trueHeight = result.y;
		
		while (true)
		{
			Rectangle trim = scrollable.computeTrim(0, 0, trueWidth, trueHeight);
			if (trim.height > result.y || trim.width > result.x)
			{
				if (trim.height > trueHeight)
					trueHeight --;
				if (trim.width > trueWidth)
					trueWidth --;
			}
			else
			{
				break;
			}
		}
		
		result.x = trueWidth;
		result.y = trueHeight;
		// End kludge
		
		int width = 0;
		int size = getColumnCount(scrollable);
		for (int i = 0; i < size; ++i) {
			ColumnLayoutData layoutData = getLayoutData(scrollable, i);
			if (layoutData instanceof ColumnPixelData) {
				ColumnPixelData col = (ColumnPixelData) layoutData;
				width += col.width;
				if (col.addTrim) {
					width += getColumnTrim();
				}
			} else if (layoutData instanceof ColumnWeightData) {
				ColumnWeightData col = (ColumnWeightData) layoutData;
				width += col.minimumWidth;
			} else {
				Assert.isTrue(false, "Unknown column layout data"); //$NON-NLS-1$
			}
		}
		if (width > result.x)
			result.x = width;

		return result;
	}
Comment 33 Tiburon T CLA 2015-04-15 06:03:59 EDT
Ran into this today. Is there still no news and no workaround?
Comment 34 Sebastian Westemeyer CLA 2015-04-15 07:05:39 EDT
There is a workaround. This is what I do for all my FormToolkit tables:

// BEGIN workaround for eclipse bug with column layout https://bugs.eclipse.org/bugs/show_bug.cgi?id=215997
Composite bugWorkaround = toolkit.createComposite(container, SWT.NONE);
GridLayout gridLayout = new GridLayout();
gridLayout.marginWidth = gridLayout.horizontalSpacing = gridLayout.marginBottom = gridLayout.marginHeight = gridLayout.marginLeft = gridLayout.marginRight = gridLayout.marginTop = 0;
bugWorkaround.setLayout(gridLayout);
bugWorkaround.setLayoutData(new TableWrapData(TableWrapData.FILL_GRAB));
// if not for the bug, this composite would have instanceNameTableComposite as a parent and the
// TableWrap data as layoutData
Composite tableComposite = toolkit.createComposite(bugWorkaround, SWT.NONE);

// This keeps the table from growing every time the form is reflow()ed
tableComposite.setLayoutData(new GridData(SWT.FILL, SWT.NONE, true, false));
// END workaround for eclipse bug https://bugs.eclipse.org/bugs/show_bug.cgi?id=215997

// Jface layout for table object!! Table HAS TO BE in its own Composite!!
TableColumnLayout layout = new TableColumnLayout();
tableComposite.setLayout(layout);

final Table table = toolkit.createTable(tableComposite, SWT.FLAT | SWT.CHECK | SWT.SINGLE | SWT.H_SCROLL
    | SWT.V_SCROLL | SWT.FULL_SELECTION | SWT.BORDER);
table.setHeaderVisible(false);
tableViewerFolderTypes = new CheckboxTableViewer(table);
table.setLinesVisible(true);
tableViewerFolderTypes.setComparator(new ViewerComparator(new ComparableComparator<IniParameterSetting>()));
TableViewerColumn col = new TableViewerColumn(tableViewerFolderTypes, SWT.LEFT);
layout.setColumnData(col.getColumn(), new ColumnWeightData(100));
Comment 35 Fedor Malyshev CLA 2015-11-16 08:22:43 EST
An easy way to reproduce in eclipse RCP preference pages using org.eclipse.ui.preferencePages extension point:

public class SomePage implements IWorkbenchPreferencePage {

	private Composite composite;

	@Override
	public void createControl(Composite parent) {
		composite = new Composite(parent, SWT.NONE);

		Table table  = new Table(composite, SWT.BORDER);
		TableColumn column = new TableColumn(table, SWT.CENTER);
		column.setText("Some Column");
		TableColumnLayout tableColumnLayout = new TableColumnLayout();
		tableColumnLayout.setColumnData(column, new ColumnWeightData(100));
		composite.setLayout(tableColumnLayout);

		table.setHeaderVisible(true);
	}

	@Override
	public Point computeSize() {
		return composite.computeSize(SWT.DEFAULT, SWT.DEFAULT);
	}

	@Override
	public void setSize(Point size) {
		composite.setSize(size);
	}

	@Override
	public Control getControl() {
		return composite;
	}

....

the key to the bug is wrong size computation. The reason is that org.eclipse.swt.widgets.Table.computeSize() adds a scroll width to sum of column's widths even in case 
the scroll bar is not visible:

int width = 0;
int count = (int)/*64*/OS.SendMessage (hwndHeader, OS.HDM_GETITEMCOUNT, 0, 0);
for (int i=0; i<count; i++) {
	width += OS.SendMessage (handle, OS.LVM_GETCOLUMNWIDTH, i, 0);
}

...

if ((style & SWT.V_SCROLL) != 0) {  //check if scroll bar is actially shown on windows??
	width += OS.GetSystemMetrics (OS.SM_CXVSCROLL);
}

The only reason bug is not reproduced on every eclipse preference page with a Table is that computed size is cached in org.eclipse.jface.preference.PreferencePage size property.
Comment 36 Eclipse Genie CLA 2020-07-31 02:00:04 EDT
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. As such, we're closing this bug.

If you have further information on the current state of the bug, please add it and reopen this bug. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.

--
The automated Eclipse Genie.