Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 381075

Summary:

Unexpected layout result when using GridLayout and ToolbarLayout

Product:

[Tools] GEF

Reporter:

Andreas Scharf <andreas.scharf>

Component:

GEF-Legacy Draw2d

Assignee:

Alexander Nyßen <nyssen>

Status:

RESOLVED FIXED

QA Contact:

Severity:

normal

Priority:

CC:

nyssen

Version:

3.8

Target Milestone:

3.8.1 (Juno SR1)

Hardware:

OS:

Windows 7

Whiteboard:

Attachments:

Description	Flags
Expected layout	none
Result layout	none
Modified version of the Shapes example plugin	none
Shape diagram visualizing the problem	none

Description Andreas Scharf

2012-05-30 12:38:05 EDT

After updating to the master stream of GEF, I noticed some strange behavior of our GEF based editors. When using GridLayout in combination with ToolbarLayout (I'm not sure if the problem is restricted to this combination) and margins, I get unexpected layout results. I'll attach two screenshots visualizing the problem:

- Result.png shows the result layout
- Expected.png the (in my opinion correct layout)

To reproduce the problem, I modified the shapes example and attached the changed plugin. Basically I modifiy the visualization of the rectangle in the following way:

			RectangleFigure rectangleFigure = new RectangleFigure();
			rectangleFigure.setLayoutManager(new ToolbarLayout());

			Figure compartment = new Figure();
			compartment.setBorder(new MarginBorder(50));
			GridLayout manager = new GridLayout(2, false);
			manager.horizontalSpacing = 0;
			manager.verticalSpacing = 0;
			manager.marginHeight = 0;
			manager.marginWidth = 0;
			compartment.setOpaque(true);
			compartment.setBackgroundColor(ColorConstants.lightGray);
			compartment.setLayoutManager(manager);

			Figure first = new Figure();
			first.setPreferredSize(new Dimension(80, 20));
			first.setOpaque(true);
			first.setBackgroundColor(ColorConstants.blue);
			compartment
					.add(first, new GridData(SWT.FILL, SWT.FILL, true, true));

			Figure second = new Figure();
			second.setPreferredSize(new Dimension(20, 20));
			second.setOpaque(true);
			second.setBackgroundColor(ColorConstants.red);
			compartment.add(second,
					new GridData(SWT.FILL, SWT.FILL, true, true));
			rectangleFigure.add(compartment);
			
I also attached an example diagram which can shows the problem. So to easily reproduce the problem yourself do the following:

1. Import the attached ShapeExample_Plugin into your workspace. 
2. Start a runtime Eclipse with the ShapeExample_Plugin activated.
3. Import the attached ShapeExample_Project into the runtime workspace.
4. Open the diagram "shapesExample1.shapes"

Result: Result.png
Expected: Expected.png

This bug was introduced by #371084, however I'm not sure if fixing #371084 just revealed another bug.

Comment 1 Andreas Scharf

2012-05-30 12:38:37 EDT

Created attachment 216491 [details]
Expected layout

Comment 2 Andreas Scharf

2012-05-30 12:38:57 EDT

Created attachment 216492 [details]
Result layout

Comment 3 Andreas Scharf

2012-05-30 12:39:25 EDT

Created attachment 216493 [details]
Modified version of the Shapes example plugin

Comment 4 Andreas Scharf

2012-05-30 12:39:53 EDT

Created attachment 216494 [details]
Shape diagram visualizing the problem

Comment 5 Alexander Nyßen

2012-06-01 05:30:53 EDT

Ok, I have analyzed the problem. It is indeed a bug within GridLayout's calculatePreferredSize(IFigure, int, int) method that got "revealed" by the fix in bug #381075. In fact, before fixing bug #381075, the calculatePreferredSize(IFigure, int, int) within GridLayout always used a cached preferred size, even when being called with different width or height hints. As such, the wHints getting passed in from ToolbarLayout in your example have always been ignored after the first call (and the cached preferred size calculated with a wHint of -1 was used instead). Having fixed that (so that GridLayout now re-calculates the preferred size when different hints get passed in), the problem was thus revealed, that the border width and height does not properly get subtracted from the passed in hints (different to -1) before performing the layout calculations, but always added afterwards. 

Having said this, GridLayout's calculatePreferredSize(IFigure, int, int) will have to be adjusted as follows to fix the problem: 

protected Dimension calculatePreferredSize(IFigure container, int wHint,
			int hHint) {
		// Remove the size of the border from the wHint and hHint in case a size
		// different to the preferred size is used
		int borderWidth = container.getInsets().getWidth();
		int borderHeight = container.getInsets().getHeight();

		if (wHint != SWT.DEFAULT)
			wHint -= borderWidth;
		if (hHint != SWT.DEFAULT)
			hHint -= borderHeight;

		Dimension size = layout(container, false, 0, 0, wHint, hHint, /* flushCache */
				true);
		if (wHint != SWT.DEFAULT)
			size.width = wHint;
		if (hHint != SWT.DEFAULT)
			size.height = hHint;

		// Add the size of the border again before returning the calculated size
		size.expand(borderWidth, borderHeight);

		// the border preferred size might enlarge the size (in most cases it
		// should be empty)
		size.union(getBorderPreferredSize(container));

		return size;
	}

A second issue (which does however not have any effect here) is that GridData in fact also ignores the passed in flushCache value and always uses cached values. So GridData's computeSize(figure, flushCache) method should as well be adjusted to:

Dimension computeSize(IFigure figure, boolean flushCache) {
		if (flushCache) {
			flushCache();
		} else {
			if (cacheWidth != -1 && cacheHeight != -1) {
				return new Dimension(cacheWidth, cacheHeight);
			}
			for (int i = 0; i < cacheIndex + 1; i++) {
				if (cache[i][0] == widthHint && cache[i][1] == heightHint) {
					cacheWidth = cache[i][2];
					cacheHeight = cache[i][3];
					return new Dimension(cacheWidth, cacheHeight);
				}
			}
		}

		Dimension size = figure.getPreferredSize(widthHint, heightHint)
				.getCopy();
		if (widthHint != -1)
			size.width = widthHint;
		if (heightHint != -1)
			size.height = heightHint;

		if (cacheIndex < cache.length - 1)
			cacheIndex++;
		cache[cacheIndex][0] = widthHint;
		cache[cacheIndex][1] = heightHint;
		cacheWidth = cache[cacheIndex][2] = size.width;
		cacheHeight = cache[cacheIndex][3] = size.height;
		return size;
	}

However, as we are already passing the 3.8 RC3 gate today I doubt we will be able to fix this problem for 3.8 release, but it will probably have to go into 3.8 SR1 (Anthony, please correct me if we can still have that fix included).

Comment 6 Alexander Nyßen

2012-07-16 16:24:23 EDT

Fixed GridLayout and GridData. Pushed changes to master as well as R3_8_maintenance. Resolving as fixed.