Bugzilla – Full Text Bug Listing
| Summary: | [Layout] Issues with algorithm in GridLayout.layout and Composite.computeSize | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Nick Edgar <n.a.edgar> | ||||
| Component: | SWT | Assignee: | Carolyn MacLeod <carolynmacleod4> | ||||
| Status: | RESOLVED FIXED | QA Contact: | Carolyn MacLeod <carolynmacleod4> | ||||
| Severity: | normal | ||||||
| Priority: | P2 | CC: | Andreas.Hoegger, daniel.kruegler, deepakazad, Enygma2002_ro, jason.roberts, keith.chong.ca, lshanmug, markus.kell.r, Silenio_Quarti, snorthov | ||||
| Version: | 3.2 | ||||||
| Target Milestone: | 3.7 M4 | ||||||
| Hardware: | PC | ||||||
| OS: | Windows 2000 | ||||||
| Whiteboard: | |||||||
| Bug Depends on: | |||||||
| Bug Blocks: | 85899 | ||||||
| Attachments: |
|
||||||
|
Description
Nick Edgar
I was expecting to get (0, 0) back. Actually, I get (64, -5) back from Composite.computeSize due to the following logic in Composite: if (size.x == 0) size.x = DEFAULT_WIDTH; if (size.y == 0) size.y = DEFAULT_HEIGHT; For my case, I really want it to return (0, 0). I would expect that the default width and height are only used when a layout is not specified. See bug 85899 for more background. There are actually three problems here: 1. Incorrect result from GridLayout.computeSize. 2. Implementation issues in GridLayout.computeSize that may or may not be relevant. 3. Composite.computeSize should not do the 64@64 default size thing if a layout is specified (and maybe not at all). Note that computeSize() should never return a negative value. The correct result is not (0,0) but (64, 64). public static void main(String[] args) {
Display display = new Display();
Shell shell = new Shell(display);
shell.setLayout(new FillLayout());
{
Composite c = new Composite(shell, SWT.NONE);
GridLayout layout = new GridLayout();
layout.marginWidth = layout.marginHeight = 0;
c.setLayout(layout);
System.out.println("GridLayout = "+c.computeSize(SWT.DEFAULT, SWT.DEFAULT));
}
{
Composite c = new Composite(shell, SWT.NONE);
FillLayout layout = new FillLayout();
layout.marginWidth = layout.marginHeight = 0;
c.setLayout(layout);
System.out.println("FillLayout = "+c.computeSize(SWT.DEFAULT, SWT.DEFAULT));
}
{
Composite c = new Composite(shell, SWT.NONE);
FormLayout layout = new FormLayout();
layout.marginWidth = layout.marginHeight = 0;
c.setLayout(layout);
System.out.println("FormLayout = "+c.computeSize(SWT.DEFAULT, SWT.DEFAULT));
}
{
Composite c = new Composite(shell, SWT.NONE);
RowLayout layout = new RowLayout();
layout.marginWidth = layout.marginHeight = 0;
c.setLayout(layout);
System.out.println("RowLayout = "+c.computeSize(SWT.DEFAULT, SWT.DEFAULT));
}
{
Composite c = new Composite(shell, SWT.NONE);
StackLayout layout = new StackLayout();
layout.marginWidth = layout.marginHeight = 0;
c.setLayout(layout);
System.out.println("StackLayout = "+c.computeSize(SWT.DEFAULT, SWT.DEFAULT));
}
}
GridLayout = Point {64, -5}
FillLayout = Point {64, 64}
FormLayout = Point {64, 64}
RowLayout = Point {6, 6}
StackLayout = Point {64, 64}
RowLayout and GridLayout need to be corrected to be consistant Note that if you give negative values for the margins, all SWT layouts will return negative results for computeSize. Fixed RowLayout and GridLayout to handle the case where there are no children in the same manner as the other layouts. > The correct result is not (0,0) but (64, 64). But I want the correct result to be (0,0) <g>. See bug 85899 for details. Basically, I have a placeholder composite representing the view stack, but in this case it's a standalone view with no title. It's an empty composite with no children, and I want it's preferred size of (0,0) to come through. This gets defeated by the DEFAULT_WIDTH/HEIGHT handling. I would have thought that if I gave it an explicit layout (whether GridLayout or other), that it would not do the DEFAULT_WIDTH/HEIGHT handling, since that's essentially overriding what the layout gave. That is why I haven't closed this bug yet :D I will talk with Steve when he gets back about whether we will change Composite. BTW a margin value of 1 will give almost what you want (2,2) - a good reason why a margin value of 0 should give (0,0). I concur. The case in bug 85899 needs to handle both: it's got a flag for whether there's a 1-pixel border or not, so it would be nice to have consistent treatment. CAR was looking into changing the defaults for all widgets. In any case, a composite that gives a layout that computes (0, 0) should have use that value. I can't see why we shouldn't just fix this case at least. VI? Ran into this today, any chance of this getting fixed? I should mention that I ran into this while working on bug 315772. Would really help if this gets fixed. Ran into this too a couple of days ago. An empty composite should have size (0,0) and not (64,64). I had a composite to which I added and removed children, and each time I would remove all children, the composite, instead of having a height of 0, was occupying 64 pixels in height leaving me with a blank spot. My workaround was to manually assign a height hint of 0 to the layout when the last control is removed and reassigning the default height when a control was added again, but this is unnatural. It defeats the purpose of a layout manager which should be in charge of these computations. It seems that this bug is over 5 years old now. Are people still aware of it? I too have recently ran into this bug. I decided to use margin values of 1 to get around the problem....not an ideal solution. I stumbled across this issue as well - it would be great to have a fix for it Is there anybody posting comments with SWT commiter rights? I cannot believe several people are annoyed about this 64*64 default size and nothing gets fixed for 5 years. For business applications pixel faults are P1! Therefore I would suggest to remove the following two lines of the computeSize(int,int,boolean) method in Composite: if (size.x == 0) size.x = DEFAULT_WIDTH; if (size.y == 0) size.y = DEFAULT_HEIGHT; Carolyn and SSQ to investigate this. Note that changing the default values could break anyone that expects this behavior. Things are never as simple as they appear. <g>
At this point (and even back in 2005), changing the default size of a Composite would potentially break people who do not even realize that they are counting on the behavior. Examples are Composites (and all of its many subclasses, such as Canvas, Shell, etc) whose contents are drawn using graphics calls. These composites often have no children.
We discussed this with Steve several times in the past. I will discuss it again with SSQ.
SSQ, should we consider enforcing default size for subclasses of Composite only, and not for instances of Composite itself? This seems to be the place where a "default size" feels like unexpected behavior. I do not know how many people would be broken by the change, but I think it would be very few? Any future "I got the latest SWT and my custom-drawn Composite disappeared!" bugs could be duped to this one, where the work-around would be to set the size of the Composite to something, i.e. composite.setLayoutData(new GridData(64, 64));
By the way, for those of you who can use a work-around to the unwanted default size behavior, the following line of code will work, if you know when your composite has no children:
composite.setLayoutData(new GridData(0, 0));
Note that if you use this, you will need to change the layout data to something else if/when you add a child.
(In reply to comment #22) > Carolyn and SSQ to investigate this. Note that changing the default values > could break anyone that expects this behavior. I don't deny that, and it is true, that usually changing defaults is a bad idea - unless it turns out that the chosen default is badly chosen ;-) Seriously: If that argument is the show-stopper, why has this bug not been closed around 2005? I dont think the default needs to be changed. I think I like the following solution.. (In reply to comment #15) > CAR was looking into changing the defaults for all widgets. In any case, a > composite that gives a layout that computes (0, 0) should have use that value. > I can't see why we shouldn't just fix this case at least. VI? public Point computeSize (int wHint, int hHint, boolean changed) { checkWidget (); display.runSkin (); Point size; if (layout != null) { if (wHint == SWT.DEFAULT || hHint == SWT.DEFAULT) { changed |= (state & LAYOUT_CHANGED) != 0; state &= ~LAYOUT_CHANGED; size = layout.computeSize (this, wHint, hHint, changed); } else { size = new Point (wHint, hHint); } } else { size = minimumSize (wHint, hHint, changed); if (size.x == 0) size.x = DEFAULT_WIDTH; // Do this only when layout is null if (size.y == 0) size.y = DEFAULT_HEIGHT; } if (wHint != SWT.DEFAULT) size.x = wHint; if (hHint != SWT.DEFAULT) size.y = hHint; Rectangle trim = computeTrim (0, 0, size.x, size.y); return new Point (trim.width, trim.height); } (In reply to comment #25) +1 for Deepak's fix. The snippet from comment 6 then prints Point {0, 0} for all layouts (except RowLayout, but that's because RowLayout defaults to marginLeft = 3, etc). I looked around in the SDK, and I also didn't find any problem with this fix there. To find problematic cases, I've put the following in place of the original "if (size.x == 0) ..." lines, but in all cases where the breakpoint was hit, the size was either irrelevant (composite not shown) or updated later: if (size.x == 0 && wHint == SWT.DEFAULT || size.y == 0 && hHint == SWT.DEFAULT) System.out.print("bug 118659"); // breakpoint My painted components needs to be 16*16 pixels, can anybody change the default size to that one ;) Sorry I really do not understand people hanging on this default size! People building their applications on the composites default size 64*64 have definitely misunderstood something serious in UI programming. Therefore please do not fix a bug with another one – By another bug I mean to find 64*64 somehow in the next release. <a href="show_bug.cgi?id=118659#c25">Comment 25</a>: Is a way to hide 64*64 in almost every normal program flow. Isn’t the better way to remove unnecessary code? Or would anybody like to explain in future why people should set any layout on their empty composites to get rid of the default size? I do not see any problem in making the change proposed in comment#25. If the layout is set it should determine the size of the composite. But there are many other places where DEFAULT_WIDTH and DEFAULT_HEIGHT are used in the hierarchy. We need to investigate them. (In reply to comment #27) > My painted components needs to be 16*16 pixels, can anybody change the default > size to that one ;) > > Sorry I really do not understand people hanging on this default size! People > building their applications on the composites default size 64*64 have > definitely misunderstood something serious in UI programming. > Therefore please do not fix a bug with another one – By another bug I mean to > find 64*64 somehow in the next release. > > <a href="show_bug.cgi?id=118659#c25">Comment 25</a>: Is a way to hide 64*64 in > almost every normal program flow. Isn’t the better way to remove unnecessary > code? Or would anybody like to explain in future why people should set any > layout on their empty composites to get rid of the default size? I really agree with this comment. People that built their applications relying on the default size of an empty composite just ended up with bad code. The platform needs to be firm and fix this. An empty composite needs to be empty, not take up 64x64 pixels. On the other hand, there are lots of people, like me and other above, that had to overcome this problem by faking with size 0 (or margin 1)... so changes in the platform will not affect their code (since they coded with an empty composite in mind). Please fix this... it's been 5 years. The more this goes on, the more people are encouraged to rely on it and will make it harder and harder to fix. I'm still in favor of fixing this for composites with a layout (the full fix would also be OK for me). After running with the full fix for a few days, I only ran into bug 331550, where an intermediate height of 0 lead to collateral damage. I've fixed our code that made bad assumptions. When you fix this bug, please announce the fix at least in the porting guide, so that other clients with similarly broken code can find the cause more easily. Created attachment 184393 [details] patch to make empty composites with a layout be 0 x 0 Sorry to take so long to reply to this - I was away for a few days. I released the "composites with a layout" fix suggested by Deepak in comment 25. Here is the patch (for all platforms). It is now in HEAD, and it will be in M4, and I do not anticipate too many problems, if any. Thank-you, Deepak. We will consider implementing the "full fix" right after M4, but we do not want to break anybody right before M4. We will announce any changes on the eclipse.org-committers mailing list, as well as listing it in the "What's New" section of the Platform Plug-in Developer Guide (Markus, is this the "porting guide" that you mentioned?) so that people can test their stuff before M5. Markus, when you say you have been "running with the full fix", I am not sure whether you mean: 1) delete 2 lines in Composite.computeSize() (i.e. if size == 0 set size to defaults) or 2) set DEFAULT_HEIGHT and DEFAULT_WIDTH to 0 in Widget The "full fix" actually involves more than just composites. DEFAULT_HEIGHT and DEFAULT_WIDTH are used throughout the toolkit, including by simple Controls. We need to look at each control (on each platform) and decide what its preferred "empty" size is... and we will be trying for 0 x 0 (plus trim) everywhere. This includes thinking about a List, Table, or Tree with no items; or a Label, Text or StyledText with no text. Currently Combo, DateTime and Spinner have unused code that checks for size 0, but this will never happen. Similarly CTabFolder doesn't need default width because it always adds 3 pixels. So there are a bunch of controls & platforms to test. :) The things that most likely will be broken are the simple code, like snippets. Our ControlExample would be broken for Canvas and Group. I might add an "empty" feature to ControlExample to test "empty" state. (In reply to comment #31) > We will announce any changes on the > eclipse.org-committers mailing list, as well as listing it in the "What's New" > section of the Platform Plug-in Developer Guide (Markus, is this the "porting > guide" that you mentioned?) so that people can test their stuff before M5. No, I meant /org.eclipse.platform.doc.isv/porting/3.7/incompatibilities.html (see the same file for previous versions for a template to copy). > Markus, when you say you have been "running with the full fix", I am not sure > whether you mean: > 1) delete 2 lines in Composite.computeSize() > (i.e. if size == 0 set size to defaults) > or > 2) set DEFAULT_HEIGHT and DEFAULT_WIDTH to 0 in Widget I only did (1). I'm actually not sure whether changing DEFAULT_HEIGHT and DEFAULT_WIDTH is of much value. Having non-zero defaults is actually helpful when you're not using a layout. That way, you immediately see the controls for which you forgot to set a size. 0-by-0 controls are not visible, so this change would make debugging harder, but would not have a big advantage in practice. Clients that need a 0-by-0 Composite can just call setSize(0, 0), and they're done. Since I haven't seen a compelling reason to change the defaults after so many years, I would just leave them as they are. Just to be clear: Things are completely different for Composites with a layout (comment 0). There, we just had an implementation bug where the default size wrongly showed up in a corner case. This is fixed in HEAD with comment 31. > 0-by-0 controls are not visible, so this change would make debugging harder This is what I have been afraid of... <g> > Clients that need a 0-by-0 Composite can just call setSize(0, 0), > and they're done. Unfortunately this does not work because in Composite.computeSize(), if the Composite does not have a layout and the size is 0 x 0, then it is set to 64 x 64... <grin>. Perhaps this is the real bug all along. Perhaps we just need to keep track of whether the size was explicitly set (we currently don't... setSize just sets the size of the control in the OS, which of course would be 0 x 0 when queried). That way, if the Composite (or Canvas, etc) has no children (i.e. if the calculated minimumSize ends up being 0 x 0) AND IF we know that the size was intentionally set, we would use the size that was set, and not override to the default size. (If there are no children and the size was never set, then the defaults would still be used). > Perhaps we just need to keep track of whether the size was explicitly set...
Actually, this would be bogus because the *parent layout* explicitly sets the size of the control after asking what its preferred size is - that's how layouts work. So querying the last setSize is the wrong thing to do.
There are several ways to get a Composite that is being used as a placeholder to have 0 x 0 size when there are 0 children:
1) the Composite must have a layout (if the layout calculates 0 x 0, this is now honored)
or 2) the subclass of Composite must define its own computeSize() (if you are subclassing a Composite, you really ought to be writing your own computeSize anyhow) or define its own layout (which has a computeSize)
or 3) the Composite, in a parent with a layout, sets its layout data's width hint and height hint to 0 when it detects that it has 0 children, and sets the data back to "what it was before" when children are present.
I am going to mark this fixed. The fix (see comment 31) has been in since 3.7M4. See comment 34 for several ways to get a Composite with 0 children to have 0 x 0 size. If anyone still has a specific problem, please reopen with a snippet that shows the problem. I think I agree with this fix...seems to make sense.
But, for the record, this caused a change in behaviour of SashFormLayout layout method.
In my scenario, previously, for my horizontal orientation, the width was 64, and the height was 'some positive number'. Now, the width is 0, and the height is the same.
protected void layout(Composite composite, boolean flushCache) {
SashForm sashForm = (SashForm)composite;
Rectangle area = sashForm.getClientArea();
if (area.width <= 1 || area.height <= 1) return;
.....
// keep just the right number of sashes
So, it returns now, and it doesn't proceed with creating the Sash(es) as it did before in 3.6.
For now, we will react to this change to account for the Sash not being created.
|