Bugzilla – Full Text Bug Listing
|
Description
Stefan Xenos
Created attachment 73871 [details]
Example code to demonstrate the problem
This example contains two Sections. The upper section contains a wrapping gridlayout with two columns. Notice that:
- The section does not reserve the correct amount of vertical space for the layout initially
- When you resize the dialog, the upper section does not expand as the text wraps.
Created attachment 73877 [details]
Fix for this defect
The problem here is that FormUtil.isWrapControl is erring on the side of treating controls as non-wrapping.
This is unsafe, since it will cause undetected wrapping controls to return the wrong height. It is, however, safe to treat a non-wrapping control as wrapping. This patch removes the optimization and treats all controls as wrapping.
This makes less efficient use of the layout caches since cache misses for wrapping controls are more common, but it will always result in correct layouts.
If we wish to bring back the optimization, we should do so selectively for particular controls that are known to be non-wrapping, and we should always default to the wrapping behavior for unknown controls.
Adam, does this look like the same problem that was fixed in Bug 171276? No, it's a different problem. I can see the issue with the sample provided in comment 1 using the changes from bug 171276. I'm very hesitant, however, to arbitrarily call every control wrappable. This is going to impose a significant overhead in computing the size and laying out of non-wrappable controls. I would also be hesitant to treat all controls as wrappable. Anything that makes forms slower should be avoided. With the most recent forms code I do not see the first problem listed in comment 1. The second problem can be corrected by instantiating the Composite with the SWT.WRAP style as follows: Composite theComposite = new Composite(section1, SWT.WRAP); Re: comment 5 > I would also be hesitant to treat all controls as wrappable. Anything > that makes forms slower should be avoided. There *is* a performance cost to passing in correct width hints, but it is negligable (when I contributed the fix to bug 73716, I did a lot of profiling on the forms layouts and understand the performance costs well). As long as there is no more than 1 call to computeSize with a non-SWT.DEFAULT width hint per layout, most of the calls will be cached by SizeCache. This remains the case with this patch. However even if the speed cost were significant, it wouldn't matter. The speed of an algorithm is not important if that algorithm produces the wrong answer. If we were comparing two correct algorithms, the faster one would be better. However, in this case we're comparing an incorrect algorithm with a (marginally) slower correct one. If the code didn't need to be correct, then the natural consequence would be to make all of our layouts produce 0-size controls that are off the screen, since this is the fastest possible layout. Comment 6 observes that the library will pass in the correct width hint if the Composite makes use of the SWT.WRAP constant. This is true, but it is also a restatement of the bug. In order for this to be a permissible optimization, Composite (or computeSize) would need to guarantee that the width hint would be unused unless the control sets the SWT.WRAP flag. This is not the case in general, so the caller remains responsible for supplying a correct hint. True, you can argue that the code that instantiates the Composite may know that it will be used within a Section and know that it needs the correct width hint and so set the SWT.WRAP flag. There are several problems with this: 1. SWT does not specify SWT.WRAP as a valid flag on Composite. This means that Composite (or its subclasses) may add incompatible semantics to the SWT.WRAP flag in the future. For example, if I wrote a subclass of Composite that used the width hint in all cases and that used the SWT.WRAP flag to indicate whether it should wrap its contents, it would be impossible to insert a non-wrapping example of my control into a Section. In general, the meaning of the flags are determined by the Control itself. If you want to give the enclosing layout some meta-data about the control, that meta-data can go in the control's setLayoutData, its setData, or in the enclosing layout itself. The enclosing layout is not allowed to make assumptions about what one of its children uses its flags for unless the child's JavaDoc explicitly says so. 2. The code that instantiates the Composite may not know that it will be used inside a Section, as would happen if the Composite was being instantiated by plugged-in code. 3. The code that instantiates a Composite may not know whether or not the width hint is needed, since it is the layout that uses the width hint and not the composite itself. This would happen in the (very common) case where the code that instantiates the Composite is also instantiating a generic layout (like GridLayout or FormLayout), and the API for the generic layout does not specify the conditions under which the width hint will be used. This is a perfectly valid optimization to do with certain common controls (like Labels), but Section still needs to respect the API contracts on everything else. Created attachment 74193 [details]
Example code to demonstrate problem 1
Re: comment 6 In the original example I attached, it was hard to see the extra space reserved for the upper Section unless you used a very large font size (as I do). I've attached another example which should demonstrate the problem in pretty much any situation. Created attachment 75257 [details] patch Thanks for the new sample code. It helped clarify the problem. This fix widens the number of controls that are flagged as wrappable, but not nearly as inclusively as the patch in comment 2. This treats any Composite that is a child (direct or indirect) of an ExpandableComposite as wrappable. This means that we do not lose the optimization for many lower level controls and for Composites that actually aren't wrappable. Stefan, can you think of any cases that this behaviour would not account for? Dejan, can you review? These changes are far reaching and make me nervious. Have we done any tests on existing clients (i.e. PDE etc.) and some performance numbers? I agree with you entirely. Broadening the scope of this makes me nervous too. That's why I'm trying to reduce it from Stefan's suggested fix in comment 2. My own preliminary tests on PDE revealed no problems. With respect to performance, I have not done any tests, but Stefan makes a good point in comment 7. Performance is irrelevant if the algorithm is not correct. ;) I was not implying that performance matters with incorrect results. However, there is a number of scenarios where the layout does work well, and if they become slower, it would matter. Ideally we want to arrive at a situation where: 1) Scenarios that work well today are not noticeably slower 2) Scenarios that do not work today work (irrespective of performance) > Stefan, can you think of any cases that this behaviour would not account for?
This is an improvement. However, the patch will not address cases where:
1. The control does not have an ExpandableComposite ancestor.
2. There is a non-composite that requires correct width hints.
In general, if it is possible to construct any layout where the optimization will be applied to a control that requires a correct width hint, the bug remains. I would suggest the following heuristics:
Apply the optimization if:
- The contol is an instance of Toolbar, Label, Text, or CLabel and it does not have the SWT.WRAP flag set.
- The control is an instance of Combo
- The control is an instance of Composite (and not a subclass), it has a GridLayout or StackLayout, none of its children are composites, and all of its children can be proven to be safe to apply the optimization using the above heuristics.
... otherwise, don't use the optimization
I believe that these are all safe and relatively common situations. If you discover a performance regression with a particular layout, you can always add heuristics later.
However, I believe the code will still run pretty fast without this optimization.
1. The layout cache used everywhere in the forms library caches one the result of computeSize for one default and one non-default width hint.
2. Width hints tend to be repeated (easy to demonstrate experimentally).
3. Most of the standard layout implementations only make one call to computeSize with a non-default hint per child per layout (that includes GridLayout, FormLayout, and AFAIK all of the Forms layouts).
...which means you'd expect most of the computeSize calls to be cached, even without the optimization. This is still a bit of hand-waving - but when I first wrote the caching code I worked out the expected runtime of the layout algorithm based on the probability of cache hits... I'll see if I can dig up my old notes and copy them here.
As long as the algorithm runs in O(n) - where n is the number of controls in the layout, you're doing fine. The old uncached forms layouts from Eclipse 3.0 were really slow because they ran in O(2^d), where d was the maximum depth of the widget hierarchy.
*** Bug 482024 has been marked as a duplicate of this bug. *** (In reply to Stefan Xenos from comment #16) > *** Bug 482024 has been marked as a duplicate of this bug. *** You know I have a patch for it sitting on gerrit for months... https://git.eclipse.org/r/#/c/60264/ Yes... thank you for looking into it. I've added myself as a reviewer to your patch and have added a bunch of comments. I'd suggest you change the bug number on your gerrit patch to point it here. FYI, your patch is much better than my original one, since you've added tests and have done some elegant cleanup. > You know I have a patch for it sitting on gerrit for months...
Don't feel too bad. My patch for this bug has been sitting here for almost 9 years.
...but we'll get yours in. No worries. Email me directly if you update your patch and I miss it. I agree this is an important issue.
*** Bug 356596 has been marked as a duplicate of this bug. *** We should test our fix against the test case in bug 356596. (In reply to Stefan Xenos from comment #21) > We should test our fix against the test case in bug 356596. I looked at it and made it invalid since it uses shared grid data object in tests. Thanks. Okay, well I think I have a bunch of open items still on the code review. Let me know when you've sorted them out and I'll do another round. New Gerrit change created: https://git.eclipse.org/r/69355 Elena, I've broken my changes and yours into separate changes and I've pushed fixes for all the test failures in HintAdjustmentTest. There's still some failures in your original test suite which I'll look into next and we need a few more interactive tests. I've fixed all the remaining failures in the unit tests. Gerrit seems to be down right now. I'll push the changes when it's back up. Unfortunately, there seem to be some regressions in some of the PDE editors. Namely: they're compressing their contents rather than scrolling when they are compressed to their preferred size. I'll investigate that next. I've finished building the layout inspector in bug 498030, and I've investigated the problem with the forms editors further. The problem is that the editor content is becoming too narrow and no horizontal scrollbars are appearing. The cause seems to be how ScrolledForm computes the size of its immediate child. With the patch: org.eclipse.ui.forms.widgets.ScrolledForm getBounds() = Rectangle {0, 0, 382, 602} computeSize(SWT.DEFAULT, SWT.DEFAULT, false) = (1125, 1068) org.eclipse.ui.forms.widgets.Form getBounds() = Rectangle {0, 0, 365, 1051} computeSize(SWT.DEFAULT, SWT.DEFAULT, false) = (1108, 565) Without the patch: org.eclipse.ui.forms.widgets.ScrolledForm getBounds() = Rectangle {0, 0, 382, 602} computeSize(SWT.DEFAULT, SWT.DEFAULT, false) = (1125, 693) org.eclipse.ui.forms.widgets.Form getBounds() = Rectangle {0, 0, 666, 676} computeSize(SWT.DEFAULT, SWT.DEFAULT, false) = (1108, 565) Stefan/Elena, any progress here? The patches are not relatively long in the queue. The regression is still present in the forms-based editors, and I've been swamped with JDT index work for a very long time. Ralf, are you available to help here? looks complicated, but i put it on my list. Mass move. Please move to a concrete milestone if you plan to work on this item. I'm revisiting this tonight. Let's get this thing in finally... I found the cause of the regressions. Most of the implementations of ILayoutExtension.computeMinimumWidth are just delegating to computeSize with a hardcoded small width hint (0 or 5 usually). This was "working" previously because a lot of the layouts would ignore their width hint if it was too small and just return their minimum width instead. Now that we are respecting the width hints in all cases (for compatibility with every other SWT control), this no longer works and the implementations of ILayoutExtension will need correct implementations of computeMinimumWidth. Before doing this, I'll need to write some regression tests for all 8 implementations of ILayoutExtension.computeMinimumWidth (there are no unit tests at the moment). That will give us some confidence that the new implementation is equivalent to the old one. Gerrit change https://git.eclipse.org/r/69355 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=1e645458b2ef61bd7d83b1f5c7fb28dcde7ace58 I run all tests again watched them and checked PDE forms manually and seems fine (except toggle is not centered vertically, but I am not sure it support to be) Not sure it counts verified, but I set it to verified. Created attachment 268178 [details]
Image showing the problem
In preference go to Plug-in Development -> API errors/warning and then select "API Use" tab or "API Compatibility" tab to see the problem
The UI now looks ugly. See image I attached. (In reply to Vikas Chandra from comment #38) > The UI now looks ugly. See image I attached. +1. This either needs to be fixed or the two changes have to be reverted. Also note that the last change introduces a compile warning in our official build: http://download.eclipse.org/eclipse/downloads/drops4/I20170504-2000/compilelogs/plugins/org.eclipse.ui.tests.forms_3.7.100.v20170504-2002/@dot.html New Gerrit change created: https://git.eclipse.org/r/96456 Gerrit change https://git.eclipse.org/r/96456 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=aad2395cd78f3e285e3fc02a7c01712301a61222 > +1. This either needs to be fixed or the two changes have to be reverted.
I'm investigating now and will either fix or revert within 12 hours of this comment.
(In reply to Stefan Xenos from comment #42) > > +1. This either needs to be fixed or the two changes have to be reverted. > > I'm investigating now and will either fix or revert within 12 hours of this > comment. Thanks Stefan. FWIW, I haven't seen any other issue so far. Using the layout spy I've confirmed that, post-patch, the twistie (the first child of ExpandableComposite) appears to be top-aligned whereas it was center-aligned pre-patch. This results in it being 3 pixels too high. I don't expect it to be hard to fix. I'll attach a regression test along with the fix. I have a fix ready to push but git appears to be down at the moment. New Gerrit change created: https://git.eclipse.org/r/96510 Elena, my new patch removes a check you added which would top-align the dropdown if there was text to the right of it. This fixes the preference page, but I don't really understand why the condition was there to begin with. Can you explain? Gerrit change https://git.eclipse.org/r/96510 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=10d206d1b845d370971b710b96a81750f263f778 (In reply to Eclipse Genie from comment #48) > Gerrit change https://git.eclipse.org/r/96510 was merged to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=10d206d1b845d370971b710b96a81750f263f778 > It's still not centered and too high. I will revert all this on Monday evening if it is not matching M6. (In reply to Dani Megert from comment #49) > (In reply to Eclipse Genie from comment #48) > > Gerrit change https://git.eclipse.org/r/96510 was merged to [master]. > > Commit: > > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=10d206d1b845d370971b710b96a81750f263f778 > > > > It's still not centered and too high. I will revert all this on Monday > evening if it is not matching M6. Also not that we now have 4 test failing on Mac and 1 on Windows. I suggest to revert this for 4.7. It was 2 years ago I don't remember anything now, if I did something on purpose I probably added a test to cover it, if there is no test you can change it. The PDE tests failing or platform? There where are they? Did anybody check if test were correct on a first place? (In reply to Elena Laskavaia from comment #51) > It was 2 years ago I don't remember anything now, if I did something > on purpose I probably added a test to cover it, if there is no test > you can change it. > > The PDE tests failing or platform? There where are they? Did anybody check > if test were correct > on a first place? We have explicit tests for UI Forms, see e.g. http://download.eclipse.org/eclipse/downloads/drops4/I20170507-2000/testresults/html/org.eclipse.ui.tests.forms_ep47I-unit-mac64_macosx.cocoa.x86_64_8.0.html > It's still not centered and too high. I will revert all this on > Monday evening if it is not matching M6. M6 was aligning it one pixel below the center. It's perfectly centered now, which is why it's higher than M6 (you can look at it in the layout spy to confirm that it really is centered now and wasn't in M6). If you want it to match the M6 position, I'll reimplement the same fudge factor that M6 used. Patch to follow shortly. > Also not that we now have 4 test failing on Mac and 1 on Windows. > I suggest to revert this for 4.7. Please don't. I don't believe any of these test failures are likely to degrade the user experience in any way, and the patch does offer a lot of value. I'll add back the M6 fudge factor shortly and will investigate the test failures immediately afterward. (In reply to Stefan Xenos from comment #53) > > It's still not centered and too high. I will revert all this on > > Monday evening if it is not matching M6. > > M6 was aligning it one pixel below the center. I disagree - at least for Windows 7. See upcoming screenshots. (In reply to Dani Megert from comment #55) > (In reply to Stefan Xenos from comment #53) > > > It's still not centered and too high. I will revert all this on > > > Monday evening if it is not matching M6. > > > > M6 was aligning it one pixel below the center. > > I disagree - at least for Windows 7. See upcoming screenshots. Especially when it's collapsed. Created attachment 268216 [details]
How it should look
Created attachment 268217 [details]
Currently: too high
All the test failures are in newly-added tests. The testButton and testWrapHyperlink tests are really tests of the underlying SWT controls. These failures tell us that there's a minor bug in the Hyperlink and Button class in the osx implementation of SWT (specifically, SWT is not adding and subtracting the trim from the hints correctly like it does on the other platforms). Reverting the patch would remove the test case but wouldn't fix the bug. Suggest just disabling the test to make the failure go away and then re-enabling it in 4.8 when there's time to fix the bug. The testVerticallyCentered bug looks real. I'll include a fix for it when I make the vertical offset fix (patch forthcoming). Still need to investigate testLabelLongAndTextClientCompFixed. New Gerrit change created: https://git.eclipse.org/r/96609 Gerrit change https://git.eclipse.org/r/96609 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=60aa693b09fc8a755cb794816233cc4a87bc936c I've pushed a change that lowers the twistie to match the screenshot you posted (note: this may look centered since lowercase letters tend to be located near the bottom of the glyph, but it's actually 1 pixel below center). I've also suppressed the tests which are failing due to the SWT issues on osx. The last test failure - the one in testLabelLongAndTextClientCompFixed - appears to be due to a difference in the glyph size on the different platforms. I've replaced the label with a control returned by ControlFactory which uses hardcoded pixel sizes. This should eliminate the platform-specific behavior and ensure the test behaves the same on all platforms -- and it's also more appropriate, since the purpose of that test is to test ExpandableComposite -- not Label. In the latter case, we'll only know for sure if it addresses the test failure after the next nightly build... but I expect things to be sorted out now. Created attachment 268234 [details] Still too high (In reply to Stefan Xenos from comment #62) > I've pushed a change that lowers the twistie to match the screenshot you > posted (note: this may look centered since lowercase letters tend to be > located near the bottom of the glyph, but it's actually 1 pixel below > center). In http://download.eclipse.org/eclipse/downloads/drops4/I20170508-2000/download.php?dropFile=eclipse-SDK-I20170508-2000-win32-x86_64.zip it's still too high by 2 pixels (see screenshot). Please adjust to match 4.6. The tests are OK so far (p47I-unit-cen64-gtk3 linux.gtk.x86_64 8.0 pending). Created attachment 268252 [details]
Chevron alignment
Two screenshots, showing I20170308-2000 on the left and today's code in master on the right.
As you can see, I can draw two horizontal lines through the two images and they line up perfectly. The upper red line lines up with the horizontal separator on the top of the dialog box to confirm that both images align perfectly, and the lower red line passes directly through both the twistie and the center bar in the "e" on the word "General".
I captured these screenshots on Windows 10.
Dani, I'm not sure what your previous attachment was meant to show. Could you please explain it and also explain how you concluded that something is still misaligned?
(In reply to Stefan Xenos from comment #64) > Dani, I'm not sure what your previous attachment was meant to show. Could > you please explain it and also explain how you concluded that something is > still misaligned? Left is I201705082000 and right is 4.7 M6. I added some explorer windows to provide the horizontal references. As you can see, the twisties on the left are higher placed. I can't speak about Windows 10 but for Window 7 it's not OK (yet). I just tried I20170508-2000 and it doesn't look like your screenshot on either Windows 10 or GTK. However, I20170508-0800 looks like your screenshot. Are you sure you weren't accidentally using I20170508-0800 when you captured that screenshot? (In reply to Stefan Xenos from comment #66) > I just tried I20170508-2000 and it doesn't look like your screenshot on > either Windows 10 or GTK. > > However, I20170508-0800 looks like your screenshot. Are you sure you weren't > accidentally using I20170508-0800 when you captured that screenshot? 100 %. I don't have the 0800 build installed. > 100 %. I don't have the 0800 build installed.
Hmm. In that case, I'd guess that the difference between our setups is probably the size of the font. It may be that for one of us our default font is an even number of pixels high and for the other it's an odd number of pixels high. If there's a round-off error somewhere, that would explain why I'm not seeing it when using the same build and platform as you.
Looking at the code, the previous version would have had more opportunities for round-off errors (since it did division quite early) which would shift the twisty up or down a pixel based on the font size. This would have caused the vertical alignment to be a bit inconsistent between the twisty and the text label, based on the font size.
I've changed the math so that - in the presence of rounding errors - it will push the twisty down and the label up. If my hypothesis is right about the rounding errors, this should make the alignment look the way you want it on your system without breaking it on any systems where it was already correct.
New Gerrit change created: https://git.eclipse.org/r/96678 Dani, if you have a moment could you let me know if the attached patch fixes your alignment issues? On my system it makes no visual difference, but it should cause the twisty to move down by 1 more pixel if you had previously been experiencing rounding errors. > it's still too high by 2 pixels (see screenshot)
Note that in your screenshot the twisty is misaligned by 1 pixel, not 2. The second pixel is due to the fact that the left and right images are shifted by 1 pixel, which you can see by the spacing above the horizontal separator at the top of the image.
Gerrit change https://git.eclipse.org/r/96678 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=bf8238acb94c23742196f20574d19bf29f6018bb (In reply to Eclipse Genie from comment #72) > Gerrit change https://git.eclipse.org/r/96678 was merged to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=bf8238acb94c23742196f20574d19bf29f6018bb > I verified that this works in eclipse-SDK-I20170509-2000-win32-x86_64 using Tahoma 9. Thanks Stefan. I've been testing my plugin (WireframeSketcher) with RC version of Eclipse and I've encountered a problem that I've tracked down to the change in this bug. I've been using a feature of Form that allows to ignore the body in layout calculation. This feature is enabled as follows:
scrolledForm.getForm().setData(FormUtil.IGNORE_BODY, Boolean.TRUE);
Unfortunately the changes in this bug break this feature and now the size of the body is always taken in account, regardless of the IGNORE_BODY flag. I've narrowed down this change to Form$FormLayout$computeMinimumWidth. This method now looks like this:
public int computeMinimumWidth(Composite composite, boolean flushCache) {
initCaches(flushCache);
return Math.max(headCache.computeMinimumWidth(), bodyCache.computeMinimumWidth());
}
This has changed from before:
public int computeMinimumWidth(Composite composite, boolean flushCache) {
return computeSize(composite, 5, SWT.DEFAULT, flushCache).x;
}
Note that the minimum width calculation now always takes the body into account.
Can this be corrected?
The fix can be as simple as this:
public int computeMinimumWidth(Composite composite, boolean flushCache) {
initCaches(flushCache);
boolean ignoreBody = form.getData(FormUtil.IGNORE_BODY) != null;
return Math.max(headCache.computeMinimumWidth(), ignoreBody ? 0 : bodyCache.computeMinimumWidth());
}
(In reply to Peter Severin from comment #74) > Can this be corrected? Not for 4.7. Please file a new bug report with steps to reproduce. I've created Bug 518142. Created attachment 273142 [details]
Sample
Hi there,
i was just wondering why the class LayoutComposite and thus the special computeSize() for TableWrapLayouts in EclispeForms was removed with the fix of this issue.
This broke a dialog in my application, as a StyledText does not wrap it's text since migrating to Oxygen.
I added some sample code which demonstrates the Problem i have. In Neon, this code works fine (as the StyledText wraps) in Oxygen it fails to do so. Any ideas how i could fix this?
Regards Ralf
PS: The classes of following Plugins are required in the class path to run this example:
- org.eclipse.swt
- org.eclipse.ui.forms
- org.eclipse.jface
- org.eclipse.core.runtime
- com.ibm.icu
(In reply to Ralf Grossklaus from comment #78) Please try with latest Photon build: http://download.eclipse.org/eclipse/downloads/drops4/S-4.8M6-201803080630/ If the problem is still there, file a new bug report. Thanks for the quick answer. I filed https://bugs.eclipse.org/bugs/show_bug.cgi?id=532530 as the problem still exists on Photon. Regards, Ralf |