Community
Participate
Working Groups
Created attachment 273156 [details] Sample Code demonstrating the SWT.Wrap problem Hi there! With https://bugs.eclipse.org/bugs/show_bug.cgi?id=196692 the LayoutComposite in Eclipse Forms was removed, thus the special computeSize() treatment of TableWrapLayout within forms. StyledText controls, which specify the SWT.WRAP style bit do not wrap their content anymore. The example code demonstrates the problem. It works on Neon, but it does not on Oxygen and the latest Photon build. Regards, Ralf
Did you test with Photon M6? http://download.eclipse.org/eclipse/downloads/drops4/S-4.8M6-201803080630/ Please adjust the summary to describe the problem and not the implementation detail.
I reproduce this regression with Eclipse Oxygen with the provided sample. I also confirm this issue didn't occur with Eclipse Neon libraries. Replacing only the Oxygen 'org.eclipse.ui.forms' jar with the Neon one is enough to make the issue disappear. Wrapping StyledText and TextForm don't wrap anymore, whatever GridLayout or TableWrapLyout is used. I can't find any workaround for this issue and this is a major problem for us. I will be happy if anyone could find a workaround. Please consider reopening this issue .
Issue reproduced also with 'org.eclipse.ui.forms_3.7.200.v20180220-2000.jar' extracted from Phonon M6 Platform Runtime Binary. (http://download.eclipse.org/eclipse/downloads/drops4/S-4.8M6-201803080630/download.php?dropFile=eclipse-platform-4.8M6-linux-gtk-ppc64.tar.gz).
Found a workaround : 1) Replace TableWrapLayout by GridLayout and TableWrapLayoutData by GridData 2) On wrapping widgets, set GridData.widthHint to something small (eg 20). This will be the widget minimal width under which the form will display a scroll bar.
(In reply to Ralf Grossklaus from comment #0) > Created attachment 273156 [details] > Sample Code demonstrating the SWT.Wrap problem Can you attach a complete example/project to demonstrate the issue?
Created attachment 273824 [details] Complete Sample Project Here is a complete sample project. Just run it as a java application with both: Neon and Oxygen. You will see different behaviour. Not exactly sure, if my first analysis (the missing Layout Composite) was correct about the actual cause of this different behaviour. I'm not that of a layout expert. Sorry.
Stefan, you broke this. Can you take a look?
I'll try to make some time for this some time in the next week.
(In reply to Stefan Xenos from comment #8) > I'll try to make some time for this some time in the next week. Thanks! Note that next week will already be RC1.
The Oomph configuration for platform UI seems to be broken at the moment. If someone could ping me when it's working again, I'll take a look at this at that time.
(In reply to Stefan Xenos from comment #10) > The Oomph configuration for platform UI seems to be broken at the moment. If > someone could ping me when it's working again, I'll take a look at this at > that time. Please open a specific bug for whatever specific problem you're seeing with the setup and make sure I'm assigned it and on the CC list. I'll try to have a look during my current travels. I'm trying with a committer photon package to provision just the Platform UI setup, but my internet is slow here so it will take a while to see if I can reproduce any problem at all. Note that before my trip I did test that this full scenario worked: https://wiki.eclipse.org/Eclipse_Platform_SDK_Provisioning
Note that the setup has gotten as far as downloading the artifacts for a photon target platform, so it's not clear to me exactly what failed in the setup. I have to run now, but will look at the result later. Specific details of the setup failure are likely needed.
The Oomph setup for Platform UI works for me.
Stefan if you can't get it to work with Oomph simply clone the corresponding repository. Not that hard ;-).
(In reply to Dani Megert from comment #14) > Stefan if you can't get it to work with Oomph simply clone the corresponding > repository. Not that hard ;-). And for the build to use, you can download http://download.eclipse.org/eclipse/downloads/drops4/I20180516-2000/
Ping! Any chance for a fix for 4.8?
> Ping! Any chance for a fix for 4.8? I expect to have some more time to investigate this on the weekend. I'll post an update then.
> Stefan if you can't get it to work with Oomph simply clone the > corresponding repository. Not that hard ;-). Prior to Oomph, it used to take me about a day or two to get an unfamiliar Eclipse git repository compiling without compiler errors. Sadly, that's the entirety of the time I currently have to work on Eclipse stuff per month. I've heard claims that others can do it significantly faster, but I've never personally replicated or witnessed this. In the cases were I was watching, they were either ignoring compilation errors in their workspace, removing a lot of code from their workspace, or were using the same workspace they had used for years and were just estimating the time it had taken them to set it up originally. It's certainly plausible that some folks can set up an error-free workspace in less than the time it takes me, but that doesn't reduce my overhead. Unless I find myself working on this full-time again (where I can burn a day or two on occasion with such tasks) it's preferable to time my code contributions with moments when the Oomph configs are working perfectly so I can spend my time budget on the desired bugfix. That said, (climbing off soap box) I'll still resort to a manual setup this weekend if I'm unable to get Oomph working, since I understand the need for urgency on this fix.
Ed: Thanks so much for looking at this (and for your continued work on this awesome tool). I'll post further updates when I take another stab at this on the weekend.
(In reply to Stefan Xenos from comment #18) > > Stefan if you can't get it to work with Oomph simply clone the > > corresponding repository. Not that hard ;-). > > Prior to Oomph, it used to take me about a day or two to get an unfamiliar > Eclipse git repository compiling without compiler errors. Sadly, that's the > entirety of the time I currently have to work on Eclipse stuff per month. > Yes, the developers of the project often overlook this fact because they maintain the one workspace they use and they don't know all the problems that happen when you start from scratch and don't document all the magical/special things they know to do but of course no one else knows to do. > I've heard claims that others can do it significantly faster, but I've never > personally replicated or witnessed this. Yes, I doubt it too. I believe it's a bogus claim and is only theoretically possible for some very simple projects. > In the cases were I was watching, > they were either ignoring compilation errors in their workspace, removing a > lot of code from their workspace, or were using the same workspace they had > used for years and were just estimating the time it had taken them to set it > up originally. Exactly. Most people don't create a fresh new environment often (or ever!). And sadly, even when Oomph can do that for them, they don't really see the point or need for trying that so don't test it and don't maintain it. :-( > It's certainly plausible that some folks can set up an > error-free workspace in less than the time it takes me, but that doesn't > reduce my overhead. > It's not even theoretically possible to do it faster manually than having it be done for you automatically, assuming of course that automatically actually works its necessary faster than driving exactly the same steps manually... > Unless I find myself working on this full-time again (where I can burn a day > or two on occasion with such tasks) it's preferable to time my code > contributions with moments when the Oomph configs are working perfectly so I > can spend my time budget on the desired bugfix. > Of course this is true for any and all casual contributors. > That said, (climbing off soap box) I'll still resort to a manual setup this > weekend if I'm unable to get Oomph working, since I understand the need for > urgency on this fix. If you can't get something working with Oomph, please report a specific bug with specific details so I can modify the platform's setups with anything that might be necessary. Please be sure its assigned to me so that I notice. Note however that I expect this will work because I spent a great deal of time ensuring that the setups for all the project that comprise the Eclipse SDK actually work to produce an error free workspace, and I spent time documenting how anyone in the world can do that: https://wiki.eclipse.org/Eclipse_Platform_SDK_Provisioning
> please report a specific bug with specific Will do. First I'll try a complete reinstall by deleting my ~/.p2 folder. It's running now.
> First I'll try a complete reinstall by deleting my ~/.p2 folder. It's running now. All good! Sorry for troubling you, Ed. Whatever problem I hit last week was either transient or due to something in my .p2 folder. Either way it's gone now.
I did encounter an error when I tried to import PDE UI. I filed bug 535497 in case the logs are helpful.
I've modified both attached examples to run as an Eclipse plugin, so I can inspect them using LayoutSpy. I've reproduced the described behavior in Eclipse 4.8. Can confirm that on Eclipse 4.8, the controls in this example are returning their unwrapped width from computeSize(SWT.DEFAULT, SWT.DEFAULT, false). Relevant values (Eclipse 4.8): ScrolledForm: computeSize(SWT.DEFAULT, SWT.DEFAULT, false) = (3203, 72) computeSize(224 - widthAdjustment, SWT.DEFAULT, false) = (224, 72) computeSize(SWT.DEFAULT, 151 - heightAdjustment, false) = (3203, 151) From: computeSize(SWT.DEFAULT, SWT.DEFAULT, false) = (3186, 55) computeSize(3186 - widthAdjustment, SWT.DEFAULT, false) = (3186, 55) computeSize(SWT.DEFAULT, 134 - heightAdjustment, false) = (3186, 134) StyledText: computeSize(SWT.DEFAULT, SWT.DEFAULT, false) = (3166, 15) computeSize(3166 - widthAdjustment, SWT.DEFAULT, false) = (3167, 15) computeSize(SWT.DEFAULT, 15 - heightAdjustment, false) = (3166, 15) I'll repeat the test using Neon to determine what the behavior used to be.
Created attachment 274314 [details] Contains the attached examples, as a plugin Run this as an Eclipse Application. Use ctrl-3, then run "Layout Test" While the test dialog is open, use Ctrl-Alt-Shift-F9 to open the layout inspector, to view the layout properties.
Relevant values (Eclipse 4.6): ScrolledForm: computeSize(SWT.DEFAULT, SWT.DEFAULT, false) = (3203, 342) computeSize(224 - widthAdjustment, SWT.DEFAULT, false) = (224, 342) computeSize(SWT.DEFAULT, 151 - heightAdjustment, false) = (3203, 151) Form: computeSize(SWT.DEFAULT, SWT.DEFAULT, false) = (3186, 55) computeSize(207 - widthAdjustment, SWT.DEFAULT, false) = (207, 325) computeSize(SWT.DEFAULT, 325 - heightAdjustment, false) = (3186, 55) StyledText: computeSize(SWT.DEFAULT, SWT.DEFAULT, false) = (3166, 15) computeSize(187 - widthAdjustment, SWT.DEFAULT, false) = (188, 285) computeSize(SWT.DEFAULT, 285 - heightAdjustment, false) = (3166, 285) This seems to rule out a regression in computeSize as the cause of the change in behavior. In both cases, all the controls in the layout were telling their parent that that they wanted to be really wide. In 4.6, the Form itself wanted to be a lot taller but that is almost certainly unrelated to this change (and the new shorter value seems correct to me). This is all consistent with Ralf's initial hypothesis that 4.6 contained a special case in LayoutComposite that was removed. Next step: understand the special case and determine if there's a way to reintroduce it in a way that doesn't break any SWT or Forms API contracts, and that works with all 4 combinations of SWT layout inside Forms, Forms inside SWT layouts, etc.
The change in behavior appears to be in ScrolledForm. In Eclipse 4.6, it was setting its children (and the size of its scroll region) to match the width of the ScrolledForm itself. In 4.8, it sets the scroll region size to the preferred width of its children.
The bug appears to be in SharedScrollComposite.reflow. The 4.8 version is not handling the expandHorizontal=true case correctly - or at least not in the same way that 4.6 did. The trouble with defining "correctly": 4.6 wasn't behaving as documented, either (it wasn't using the setMinSize arguments in the way it claims to, and the subclass is overwriting the values passed to the public setMinSize method). Next step: write a bunch of test cases to figure out what each combination of setExpandHorizontal/setExpandVertical was actually meant to do on 4.6 for each combination of forms and non-forms children. Then figure out how to reimplement that behavior in the new SharedScrollComposite.reflow. It may even be possible to make setExpandHorizontal and setMinSize behave as documented while I'm in there.
The fact that LayoutComposite is missing is a red herring.
New Gerrit change created: https://git.eclipse.org/r/123913
I've attached a fix here, along with new regression tests. https://git.eclipse.org/r/#/c/123913/ This behavior makes wrapping controls behave more like the 4.6 did when embedded within a ScrolledForm with expandHorizontal=true. It's not exactly the same since 4.6 had a bug that would sometimes cause it to overestimate the vertical space for controls and insert unnecessary whitespace. The non-bug cases are now identical, though - and the attached sample code now works as intended. I've intentionally changed the expandHorizontal=false case. In Eclipse 4.6 the behavior was inconsistent due to some internal caching bugs, so it's unlikely anyone was relying on it. The control would sometimes expand (meaning the setting would do nothing) and sometimes be left with its preferred size. I've chosen the "preferred size" interpretation as being correct in all cases, but this differs from the 4.6 behavior in most situations. If this is deemed undesirable, we can choose the "always expand" interpretation, but doing so would cause the setting to have no effect. I've also updated SizeCache.computeMinimumWidth() to work correctly for wrapping SWT controls, which is also necessary for the attached examples. My definition of "correct" is the same as what LayoutGenerator uses -- ie: wrapping controls now report that they can be safely wrapped at some point their unwrapped size grows larger than this. This does produce some differences from the 4.6 behavior, but they should be good ones. Previously, if you put a wrapping control inside a horizontally-scrolling form and then made the form extremely narrow, you would get a very narrow control with a lot of wrapping. Now you'll get a horizontal scroll bar before the wrapping gets excessive. You can see this if you reduce the window size in the attached example. Could I trouble someone for a code review?
Note: Although I mused, above, about making changes to setMinSize, I didn't make any changes to it in this patch.
Hi Stefan! Thank you for fixing this. Really appreciate this! Sorry for the red herring :( Regards, Ralf
Dani's comments from the code review: "However, we are past RC4, which means you would have to bring this bug forward on the PMC mailing list with a risk assessment. The PMC then decides whether a respin after RC4 is warranted. See https://www.eclipse.org/eclipse/development/plans/freeze_plan_4_8.php for all the details. https://bugs.eclipse.org/bugs/show_bug.cgi?id=532530#c4 mentions a possible workaround, so, we might just deliver it to 4.9/master. I leave that up to you." Here's what you need to know to make the call: If we omit this fix, there are easy workarounds but it will be hard to build layouts that work across a variety of Eclipse versions. This may be problematic for plugin authors that support multiple Eclipse versions. The consequence of such a breakage would be an ugly and hard-to-read layout. The fix itself is fairly low risk. In order to ease the suffering of such plugin authors, I would suggest that we cherry-pick this fix into every currently-supported Eclipse version that we intend to rebuild (both 4.7 and 4.8), but I would consider this bug to be low enough in severity that it's probably not worth doing a rebuild just for this.
(In reply to Stefan Xenos from comment #34) > In order to ease the suffering of such plugin authors, I would suggest that > we cherry-pick this fix into every currently-supported Eclipse version that > we intend to rebuild (both 4.7 and 4.8). The PMC did not approve to put it into Photon (4.8). There is no longer a supported/service release with the new rolling release model, so, will just be in master/4.9.
Gerrit change https://git.eclipse.org/r/123913 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=d68fca2eadc0dfcf92dde45d4da661da96d050dd
Fixed in 4.9.
Stefan, would you please add license headers to the new files? And while on it please also fix the http://download.eclipse.org/eclipse/downloads/drops4/I20180705-2000/compilelogs/plugins/org.eclipse.ui.tests.forms_3.7.300.v20180705-1651/@dot.html#OTHER_WARNINGS - if it was only it I would have fixed it but I can't add license headers on your behalf.
Will do! Thanks for the catch, Alex.
New Gerrit change created: https://git.eclipse.org/r/125760
Gerrit change https://git.eclipse.org/r/125760 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=1ba7275967583e200f7d505d92642610c850c129
