Community
Participate
Working Groups
Created attachment 213307 [details] screenshot build id: 4.2 I-20120321-0610 Steps to reproduce: 1. Using Open Type dialog, open a file with long lines. One or more long lines should be somewhere in the beginning (first "page") of the file. 2. The file opens in the editor but it has been scrolled horizontally to the beginning of the Class name. The attached screenshot shows the problem.
This works fine in 3.x. Looks like some broken/changed event flow when opening editors.
Test case: 1. paste: public class Bug { // Bug Bug Bug Bug Bug Bug Bug Bug Bug Bug Bug Bug Bug Bug Bug Bug Bug Bug Bug Bug Bug Bug } 2. close the editor 3. use 'Open Type' to open 'Bug'
This is caused by extensive requests to do a layout with SWT.DEFER (even on the shell). As a result, the part which is being opened, is not yet realized and hence all size and position computations are doomed to fail. When executing this code: takeAPartsControl.isLayoutDeferred(); in partOpened(...) or partActivated(...) 4.2 returns 'true', which is wrong. This is a critical bug since it affects every part that wants to initialize itself based on its controls sizes and positions.
Maybe this could be fixed within SWT by always forcing the layout when a client calls methods which rely on the layout. Tricky part with this is to distinguish calls from clients vs. internal ones.
I tried to force a layout as a workaround but there doesn't seem to be a way to force the layout once it's in deferred mode.
The only way to force a layout right now is by calling display.readAndDispatch(). You could call setLayoutDefered(false) while isLayoutDeffered() is true on the appropriate composite but I believe this will leave the deffered count unbalanced since next display.readAndDispatch() will also call setLayoutDeferred(false). SWT could add API, another layout flag -> NOW which means the opposite of SWT.DEFER. I am not sure that makes sense given that you still need to know how many times to call layout(..., SWT.NOW). Maybe SWT.NOW means call setlayoutDeferred(false) as many times as necessary. But wouldn't this just defeat SWT.DEFER? What is the point. Anyway, the problem only happens when there are no editors open. It should be possible to handle this above SWT.
Comment above is mine.
(In reply to comment #6) > But wouldn't this just defeat SWT.DEFER? What is the point. Right. I guess it's just overused in this case. > Anyway, the problem only happens when there are no editors open. No, it also happens when editors are open. Every part which gets opened and wants to do computations based on the layout is affected.
OK, adding the following code (I put it in CompatibilityEditor at the end of the 'createPartControl' method): Display display = parent.getDisplay(); while (display.readAndDispatch()) ; parent.layout(true, true); While this appears to fix the issue by first flushing any DEFER'd layouts and then explicitly laying out the parent I'm concerned that spinning the event loop while opening *all* editors may introduce other regressions. If there are no other cases then it would be better if this code was put into the editor itself rather than potentially affecting currently working editors. Dani, what's your take ?
(In reply to comment #9) > OK, adding the following code (I put it in CompatibilityEditor at the end of > the 'createPartControl' method): > > Display display = parent.getDisplay(); > while (display.readAndDispatch()) > ; > parent.layout(true, true); > > While this appears to fix the issue by first flushing any DEFER'd layouts and > then explicitly laying out the parent I'm concerned that spinning the event > loop while opening *all* editors may introduce other regressions. > > If there are no other cases then it would be better if this code was put into > the editor itself rather than potentially affecting currently working editors. > > Dani, what's your take ? I suspect that driving the event loop might also have other side-effects. So, I'd neither add this for all parts nor for just the textual editors.
Silenio just pointed out that we only need a *single* call to 'readAndDispatch()' in order to flush the DEFER'd layouts. I've verified that this works and while I'm still a bit concerned about affecting other editors I'll take the initial hit. To be clear, if this introduces *any* regressions in currently working editors I'll revert the change and the code will have to go into the editor itself.
(In reply to comment #11) > Silenio just pointed out that we only need a *single* call to > 'readAndDispatch()' in order to flush the DEFER'd layouts. > > I've verified that this works and while I'm still a bit concerned about > affecting other editors I'll take the initial hit. > > To be clear, if this introduces *any* regressions in currently working editors > I'll revert the change and the code will have to go into the editor itself. Note, that not only editors are affected, but also any parts that rely on the layout. The effect is incorrectly laid out parts. There might already be other bugs complaining about this.
Dani, I know of no other instances where the current code pattern is causing similar issues, do you ? I'm not about to put this code (ok, hack) into more general use unless we can clearly identify that it's needed. commit 26e36800be05a142d83b0dae6f37d885be5073d0 Pushed the changes with the single 'readAndDispatch()'... I'll mark this as FIXED...Dani would you please get this and test whether you agree that it is indeed fixing the issue ?
(In reply to comment #13) > Dani, I know of no other instances where the current code pattern is causing > similar issues, do you ? No, but I guess that graphic editors like GEF might be affected. > I'll mark this as FIXED...Dani would you please get this and test whether you > agree that it is indeed fixing the issue ? No, it is not, because after your fix is executed many other setLayoutDeferred(true) calls are made (e.g. when updating the action bars [1]). You need to make sure that the layout is done just before partOpened/Activated is sent. [1] Thread [main] (Suspended (breakpoint at line 1082 in Composite)) Shell(Composite).setLayoutDeferred(boolean) line: 1082 WorkbenchWindow.updateActionBars() line: 1795 WorkbenchWindow.updateActionSets() line: 1874 WorkbenchPage$ActionSwitcher.updateActionSets(ArrayList) line: 777 WorkbenchPage$ActionSwitcher.updateTopEditor(IEditorPart) line: 671 WorkbenchPage.updateActiveEditorSources(MPart) line: 353 WorkbenchPage.updateBroughtToTop(MPart) line: 397 WorkbenchPage.access$16(WorkbenchPage, MPart) line: 396 WorkbenchPage$E4PartListener.partBroughtToTop(MPart) line: 173 PartServiceImpl$6.run() line: 246 SafeRunner.run(ISafeRunnable) line: 42 PartServiceImpl.firePartBroughtToTop(MPart) line: 244 PartServiceImpl.access$4(PartServiceImpl, MPart) line: 242 PartServiceImpl$1.handleEvent(Event) line: 92 UIEventHandler$1.run() line: 41 UISynchronizer(Synchronizer).syncExec(Runnable) line: 180 UISynchronizer.syncExec(Runnable) line: 150 Display.syncExec(Runnable) line: 4683 E4Application$1.syncExec(Runnable) line: 184 UIEventHandler.handleEvent(Event) line: 38 EventHandlerWrapper.handleEvent(Event, Permission) line: 197 EventHandlerTracker.dispatchEvent(EventHandlerWrapper, Permission, int, Event) line: 197 EventHandlerTracker.dispatchEvent(Object, Object, int, Object) line: 1 EventManager.dispatchEvent(Set, EventDispatcher, int, Object) line: 230 ListenerQueue.dispatchEventSynchronous(int, Object) line: 148 EventAdminImpl.dispatchEvent(Event, boolean) line: 135 EventAdminImpl.sendEvent(Event) line: 78 EventComponent.sendEvent(Event) line: 39 EventBroker.send(String, Object) line: 81 UIEventPublisher.notifyChanged(Notification) line: 57 PartStackImpl(BasicNotifierImpl).eNotify(Notification) line: 374 PartStackImpl(ElementContainerImpl<T>).setSelectedElement(T) line: 171 ModelServiceImpl.showElementInWindow(MWindow, MUIElement) line: 418 ModelServiceImpl.bringToTop(MUIElement) line: 385 PartServiceImpl.delegateBringToTop(MPart) line: 578 PartServiceImpl.bringToTop(MPart) line: 314 PartServiceImpl.showPart(MPart, EPartService$PartState) line: 962 WorkbenchPage.busyOpenEditor(IEditorInput, String, boolean, int, IMemento, boolean) line: 3066 WorkbenchPage.access$22(WorkbenchPage, IEditorInput, String, boolean, int, IMemento, boolean) line: 2991 WorkbenchPage$8.run() line: 2973 BusyIndicator.showWhile(Display, Runnable) line: 70 WorkbenchPage.openEditor(IEditorInput, String, boolean, int, IMemento, boolean) line: 2969 WorkbenchPage.openEditor(IEditorInput, String, boolean, int) line: 2928 WorkbenchPage.openEditor(IEditorInput, String, boolean) line: 2919 EditorUtility.openInEditor(IEditorInput, String, boolean) line: 373 EditorUtility.openInEditor(Object, boolean) line: 179 OpenAction.run(Object[]) line: 249 OpenAction.run(IStructuredSelection) line: 228 OpenAction(SelectionDispatchAction).dispatchRun(ISelection) line: 275 OpenAction(SelectionDispatchAction).run() line: 251 PackageExplorerActionGroup.handleOpen(ISelection, boolean) line: 376 PackageExplorerPart$4.open(ISelection, boolean) line: 538 OpenAndLinkWithEditorHelper$InternalListener.open(OpenEvent) line: 48 StructuredViewer$2.run() line: 866 SafeRunner.run(ISafeRunnable) line: 42 JFaceUtil$1.run(ISafeRunnable) line: 49 SafeRunnable.run(ISafeRunnable) line: 175 PackageExplorerPart$PackageExplorerProblemTreeViewer(StructuredViewer).fireOpen(OpenEvent) line: 864 PackageExplorerPart$PackageExplorerProblemTreeViewer(StructuredViewer).handleOpen(SelectionEvent) line: 1152 StructuredViewer$6.handleOpen(SelectionEvent) line: 1256 OpenStrategy.fireOpenEvent(SelectionEvent) line: 275 OpenStrategy.access$2(OpenStrategy, SelectionEvent) line: 269 OpenStrategy$1.handleEvent(Event) line: 309 EventTable.sendEvent(Event) line: 84 Tree(Widget).sendEvent(Event) line: 1053 Display.runDeferredEvents() line: 4165 Display.readAndDispatch() line: 3754 PartRenderingEngine$9.run() line: 1016 Realm.runWithDefault(Realm, Runnable) line: 332 PartRenderingEngine.run(MApplicationElement, IEclipseContext) line: 910 E4Workbench.createAndRunUI(MApplicationElement) line: 85 Workbench$4.run() line: 580 Realm.runWithDefault(Realm, Runnable) line: 332 Workbench.createAndRunWorkbench(Display, WorkbenchAdvisor) line: 535 PlatformUI.createAndRunWorkbench(Display, WorkbenchAdvisor) line: 149 IDEApplication.start(IApplicationContext) line: 124 EclipseAppHandle.run(Object) line: 196 EclipseAppLauncher.runApplication(Object) line: 110 EclipseAppLauncher.start(Object) line: 79 EclipseStarter.run(Object) line: 353 EclipseStarter.run(String[], Runnable) line: 180 NativeMethodAccessorImpl.invoke0(Method, Object, Object[]) line: not available [native method] NativeMethodAccessorImpl.invoke(Object, Object[]) line: 57 DelegatingMethodAccessorImpl.invoke(Object, Object[]) line: 43 Method.invoke(Object, Object...) line: 601 Main.invokeFramework(String[], URL[]) line: 629 Main.basicRun(String[]) line: 584 Main.run(String[]) line: 1438 Main.main(String[]) line: 1414
Dani, not sure I understand...can you give me a repro scenario that ends up wrong with the new code ?
(In reply to comment #15) > Dani, not sure I understand...can you give me a repro scenario that ends up > wrong with the new code ? The problem is that it sometimes indeed works but sometimes not (depending on whether other calls to setLayoutDeferred(true) happen before calling partOpened/Activated (seec comment 14). I can easily reproduce (master and I20120410-0633) on Windows 7 with these steps: 0. start new workspace 1. close Welcome 2. paste this into Package Explorer: public class Bug { // Bug Bug Bug Bug Bug Bug Bug Bug Bug Bug Bug Bug Bug Bug Bug Bug Bug Bug Bug } 3. Window > New Window 4. Open Type 'Bug' ==> now the bug might appear. If not, repeat step 3 and 4 until it appears. For me this normally happens in less than 3 attempts.
Thanks Dani, I now see that the current 'fix' won't cut it...
Dani, if you move the same 'fix' that I made in the CompatibilityEditor just before firing the event can you verify that this will fix the issue ? I had some trouble finding this API, is it the one from PartService ?
Adding the fix just at one place won't do, because lots of setLayoutDeferred(true) calls happen between partOpened, partBroughtToTop and partActivated. Therefore, we either have to fix the underlying issue of calling setLayoutDeferred(true) so many times, or we need to patch 3 places. I guess at this point, the latter is the way to go. I've removed the changes made to CompatibilityEditor and added a fix to WorkbenchPage. The fix is less intrusive than the first attempt since I only drive the event loop once and don't force a layout. This is enough to flush the deferred layouts. Fixed in master: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=8d91da76608af4522ddd141b43c059e2448078de Eric, please review the fix. Thanks.
Dani, I'm seeing a strange NPE now....I'm running N20120420-2153 and don't see this in there but I do when ever I run an inner (debug) session when clicking on any view that has not been instantiated (i.e. gets lazily rendered). java.lang.NullPointerException at org.eclipse.e4.ui.workbench.renderers.swt.StackRenderer$8.mouseUp(StackRenderer.java:878) at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:220) at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:84) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1053) at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:4169) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3758) at org.eclipse.ui.internal.WorkbenchPage.firePartOpened(WorkbenchPage.java:4365) at org.eclipse.ui.internal.e4.compatibility.CompatibilityPart$2.handleEvent(CompatibilityPart.java:110) at org.eclipse.e4.ui.services.internal.events.UIEventHandler$1.run(UIEventHandler.java:41)
(In reply to comment #20) > Dani, I'm seeing a strange NPE now....I'm running N20120420-2153 and don't see > this in there but I do when ever I run an inner (debug) session when clicking > on any view that has not been instantiated (i.e. gets lazily rendered). > > java.lang.NullPointerException > at > Did you revert my change to verify it's form that change? If so, please reopen this bug.
I put a fix in for the NPE in bug 377837. Eric, can you verify whether that fixes your NPEs too? (Though maybe it's papering over a different issue; everything seemed to work though.)
I haven't reverted it yet but I do think I have a possible fix... If you look at the trace where firePartOpened is called it's currently sent when the 'object' for the part goes non-null. This should likely just be moved to when the 'widget' is going non-null (the code for firing partClosed is already there...
This defect scares me...here's the analysis: 1) we add a model check on 'object' and firePartOpened 2) in that code we ultimately spin the event loop 3) the 'mouseUp' happens to be the top event on the queue 4) the StackRenderer's 'mouseUp' handler fires and NPE's To be that sensitive to the timing of a single event is an indication that our life cycle handling is not very robust. Adding the NPE check is a good idea but in 4.3 we should take a closer look at this... I've added Bug 377906 to track this...
OK, the preliminary testing shows that moving the 'firePartOpened' up to when the 'widget' is set seems to solve this issue. Now I have to test whether bug 337488 has regressed...other than that it looks OK. I'm going to get Bogdan to help me go over Dani's code just to be sure...
OK... a) The change to firing off 'partOpened' to when the widget goes non-null did not introduce a regression. b) After checking it appears that the only part of Dani's commit that is needed to fix this issue is the code that was added to 'firePartActivated' (makes sense as this is the last event to fire so there are no opportunities for more deferred layouts to happen). As per comment #24 we have to avoid this hack whenever possible so my preference is to revert the changes in the 'firePartOpened' and 'fireBroughtToTop' methods. Since we have no idea *what* event is on the queue any regressions (like the one we found) could be completely random and thus hard to fix. If we find that similar defects start appearing we can revisit this but for now I have to lean towards the least change possible. Dani, I'd still prefer to remove this altogether...now that I've changed where 'partOpened' gets called to after the widget is created could you re-check whether you can add this code in your 'partActivated' implementation instead of in WorkbenchPage ? That would be the minimal use of the 'readAndDispatch' hack.
commit 740d35da1cb0feda4ecc9bda7d855a8045886e19 This commit moves 'firePartOpened' to when the part's 'widget' goes non-null (as opposed tot eh 'object' as it was). This looks symmetrical now since we fire part closed when the widget goes null. It also reverts some of Dani's original commit, leaving the only place we do the 'readAndDispatch' hack is in the firePartActivated method. This is about the least intrusive I can figure out how to make it (for now). Dani, I've run the scenario multiple times but a second check would be useful... Marking FIXED (again)
(In reply to comment #27) > commit 740d35da1cb0feda4ecc9bda7d855a8045886e19 > > This commit moves 'firePartOpened' to when the part's 'widget' goes non-null > (as opposed tot eh 'object' as it was). This looks symmetrical now since we > fire part closed when the widget goes null. > > It also reverts some of Dani's original commit, leaving the only place we do > the 'readAndDispatch' hack is in the firePartActivated method. This is about > the least intrusive I can figure out how to make it (for now). > > Dani, I've run the scenario multiple times but a second check would be > useful... > > Marking FIXED (again) Sorry, but that change made things *way* worse, e.g. partOpened(...) is now no longer fired at all. I reverted your commit to fix this and to fix this original bug (again ;-). If you make further changes please test them the following way before committing: 1. open the source for ResourceNavigator.java 2. add the following code: if (viewer.getTree().isLayoutDeferred()) System.out.println("bug!"); else System.out.println("works!); to each part listener method: partListener.new IPartListener() {...}.partActivated(IWorkbenchPart) partListener.new IPartListener() {...}.partBroughtToTop(IWorkbenchPart) partListener.new IPartListener() {...}.partClosed(IWorkbenchPart) partListener.new IPartListener() {...}.partDeactivated(IWorkbenchPart) partListener.new IPartListener() {...}.partOpened(IWorkbenchPart) 3. add a breakpoint on the line with the "if" in each listener method 4. start the target 5. verify that each method is triggered when you open, activate, deactivate, bring-to-top and close views, and that the code goes through the "works!" code path Thanks.
Sheesh Dani, sorry...;-( You are correct that I completely broke partOpened...back to the drawing board <sigh> To be clear my goal here (for M7) is to get the original test case (Bug Bug Bug Bug ...) to work correctly, not to match the old behavior exactly. However I'll see how close I can get. BTW, does the code that I committed fix the 'bug bug' test case ?
(In reply to comment #29) > Sheesh Dani, sorry...;-( You are correct that I completely broke > partOpened...back to the drawing board <sigh> > > To be clear my goal here (for M7) is to get the original test case (Bug Bug Bug > Bug ...) to work correctly, not to match the old behavior exactly. We not only have to make partActivated work but also partOpened. That's what my fix did and now does again. > However I'll > see how close I can get. > > BTW, does the code that I committed fix the 'bug bug' test case ? I didn't bother spending further time on this, given the breakage it caused.
OK, I'll leave this code in even with my concerns as outlined in comment #24... Should we start seeing issues we can come back then, otherwise we'll fix this properly in 4.3. Note that the partOpened event is still generated when the control is null, if this is going to be an issue please let me know right away...if we hadn't added the null check the StackRenderer would still be throwing NPE's...
Verified in I20120503-1800.
*** Bug 378842 has been marked as a duplicate of this bug. ***
We've had to revert the part of this work that was calling 'readAdnDispatch' because it was causing serious reversions in the CTabFolder handling on the Mac. I'll re-open this for another look during 4.2.1.
(In reply to comment #34) > We've had to revert the part of this work that was calling 'readAdnDispatch' > because it was causing serious reversions in the CTabFolder handling on the > Mac. What's the bug number?
Bug 379128 PW
The real fix should really get rid of all the deferred layout requests.
*** Bug 383820 has been marked as a duplicate of this bug. ***
(In reply to comment #38) > *** Bug 383820 has been marked as a duplicate of this bug. *** I see that this bug is marked as Critical, which is good, but I want to reinforce how serious this bug is for Mac users. It really compromises our ability to jump around in the Java IDE through following references.
*** Bug 384112 has been marked as a duplicate of this bug. ***
*** Bug 384156 has been marked as a duplicate of this bug. ***
*** Bug 385716 has been marked as a duplicate of this bug. ***
Dani, there may be a quick fix for this on your side...before you do your layout call 'setLayoutDeferred(false)' on the shell. I'm not saying that we shouldn't then work with SWT to better understand the interactions between the two modes going forward but this is likely the quickest path to a working solution...
Hmm, just re-read the whole defect and it seems this may not work but it may still be worth a shot if we actually haven't attempted it yet.
(In reply to comment #44) > Hmm, just re-read the whole defect and it seems this may not work but it may > still be worth a shot if we actually haven't attempted it yet. There are several issues with that suggestion: 1. One setDeferredLayout(false) call won't work because the framework excessively makes deferred=true calls. Clients would have to call the method until the layout isn't deferred anymore. 2. The client has many places where it expects the stuff to be layouted and asks for some values, e.g. StyledText.getHorizontalPixel(). We can't go and find/hack all those places to workaround a severe framework bug. 3. All other editors would still suffer from the problem.
Dani, I think that you are right in that the correct solution to this will have to come from SWT. It looks like a non-deferred layout should first run the deferred layouts. However, I don't think that this will happen in the service releases (certainly not SR1 and likely not SR2). Silenio, correct me if I'm wrong here... The platform may indeed have to drop its current use of DEFER but this also is unlikely to happen during the SR cycles. For now it might be best to address this issue in the particular instance we have of the Java Editor. Dani, I'm begging dude, can you not see if you can fix this on your side ? I realize that I'm trying to fix the symptom not the disease here but I see no other solution and agree that this is a serious regression. Should other defects of a similar kind come in then we can address them separately.
(In reply to comment #46) > Dani, I'm begging dude, can you not see if you can fix this on your side ? Sure. Fixed with http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=d40eeb927576256d757aeded761266f550b22ef2 This adds a workaround that should cover all textual editors. Once we see it's working fine in 4.3, I'll backport it.
(In reply to comment #47) > http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/ > ?id=d40eeb927576256d757aeded761266f550b22ef2 This fix looks wrong, since it just calls shell.setLayoutDeferred(false), but doesn't undo its action by calling shell.setLayoutDeferred(true) the same number of times. It effectively leaves the layout undeferred. Since all correct earlier callers of setLayoutDeferred(true) will eventually call setLayoutDeferred(false), the layoutCount will end up negative. In the end, simple one-level usages of setLayoutDeferred will be ineffective after this.
Thanks Dani... Markus, you've hit on one of the things that Silenio pointed out when we were discussing this issue. The real fear is that as the other folks call 'setDeferredLayouts(false)' themselves to 'unwind' the deferred stack they'll end up making the count go < 0 (a known bug in SWT). His suggestion was to place the layout into an 'asynchExec', thus guaranteeing that it would be run after the event loop has spun (and purged the pending layouts). It sounds questionable to me since it changes the timing of the layout but if it works... BTW, I just checked and even calling 'Display.getCurrent.update()' will *not* run the deferred layouts, lending more weight to the idea that SWT may have to take another look at the various interactions taking place here...
Created attachment 220072 [details] TextViewer workaround 2 Here's the "restoring" I had in mind (patch on top of master).
(In reply to comment #50) > Created attachment 220072 [details] [diff] > TextViewer workaround 2 > > Here's the "restoring" I had in mind (patch on top of master). I see your point and like your patch ==> http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=dec0a5b7df19eade00ea3afb775606885be3f17a See bug 387669 for the backport.
Thanks guys, hopefully before 4.3 goes out we'll have a better solution for this at the SWT level...
*** Bug 388653 has been marked as a duplicate of this bug. ***
*** Bug 389128 has been marked as a duplicate of this bug. ***
*** Bug 389453 has been marked as a duplicate of this bug. ***
*** Bug 389559 has been marked as a duplicate of this bug. ***
*** Bug 391351 has been marked as a duplicate of this bug. ***
Eric, can this be moved to 4.3 now? PW
OK, this appears fixed to me. I'm going to mark this as FIXED, feel free to re-open if you still have concerns...
(In reply to comment #59) It works for me.
(In reply to comment #59) > OK, this appears fixed to me. I'm going to mark this as FIXED, feel free to > re-open if you still have concerns... You already had that "feeling" several times now. Let me repeat two easy test cases that you can try before closing it next time ;-). Test Case 1: ------------ 1. get 'org.eclipse.jface.text' as source in your dev workspace 2. open TextViewer 3. replace the two calls to internalRevealRangeWithWorkaround with calls to internalRevealRange 4. save 5. start new target workbench and execute next steps in it 6. close Welcome 7. paste this into Package Explorer: public class Bug { // Bug Bug Bug Bug Bug Bug Bug Bug Bug Bug Bug Bug Bug Bug Bug Bug Bug Bug Bug } 8. Window > New Window 9. Open Type 'Bug' ==> now the bug might appear: the editor is wrongly horizontally scrolled. If not, repeat step 3 and 4 until it appears. For me this normally happens in less than 3 attempts. Test Case 2: ------------ 1. open the source for ResourceNavigator.java 2. add the following code: if (part == ResourceNavigator.this && viewer.getTree().isLayoutDeferred()) System.out.println("bug!"); to those part listener methods: partListener.new IPartListener() {...}.partActivated(IWorkbenchPart) partListener.new IPartListener() {...}.partOpened(IWorkbenchPart) 3. start a target workbench 4. close Welcome 5. Window > Show View > Navigator ==> bug! bug! Both test cases pass fine on 3.x.
Dani, I'm moving this to 4.4... We know what the issue(s) are: 1) The new 4.x code makes extensive use of SWT.DEFER -and- 2) SWT's handling of this type of layout has serious issues (i.e. effectively blocks layouts which are not expected to be deferred). The only two possibilities for a proper fix require either that we rewrite all the current layout code in the workbench to avoid the use of SWT.DEFER or to have a fix from SWT that fixes the current blocking behavior. It's too late to change the workbench and I strongly suspect that it's also too late to provide the necessary SWT fix as well (Silenio ?). At this point it seems that the user community at large is not seeing the effects of this defect thanks to your workaround so it shouldn't appear as a regression to our users...
*** Bug 415546 has been marked as a duplicate of this bug. ***
M4 is done...
Reducing severity based on comment 62.
Assigning to myself for investigation. We definitely shouldn't be setting scroll bars to the wrong spot, but we also shouldn't be doing synchronous layouts due to the performance cost. Going to see if I can find a solution which is both correct and fast.
(In reply to Stefan Xenos from comment #66) > Assigning to myself for investigation. We definitely shouldn't be setting > scroll bars to the wrong spot, but we also shouldn't be doing synchronous > layouts due to the performance cost. > > Going to see if I can find a solution which is both correct and fast. The first thing to do here is writing tests that assert correct event sequences.
> The first thing to do here is writing tests that assert correct > event sequences. I disagree with the claim that performing a synchronous layout prior to firing the event is the only way to fix this, so I wouldn't want to write a test asserting that this is the case. Without having dug into this yet, the following general strategies often work for this type of problem: A. Lazily perform a partial synchronous layout on demand in order to supply the requested information. B. Defer the operation until after the layout has completed rather than performing it synchronously. C. Move some code out of listeners and into custom layout implementations such that it executes as part of the layout pass D. Some combination of the above None of these would require forcing a synchronous layout prior to firing the event and I'd like to rule them out before considering a solution with a heavy performance cost.
(In reply to Stefan Xenos from comment #68) > > The first thing to do here is writing tests that assert correct > > event sequences. > > I disagree with the claim that performing a synchronous layout prior to > firing the event is the only way to fix this, so I wouldn't want to write a > test asserting that this is the case. When a client gets the listener notification that the part is open/on top, then he can assume to safely call methods on SWT. Those methods will only work if no deferred layouts are pending. That's all what the test should do. Anything that would require the client to do something prior to calling the SWT methods would be wrong.
> When a client gets the listener notification that the part is open/on top, > then he can assume to safely call methods on SWT. I'll respond to this once I've had a chance to investigate further.
Created attachment 257885 [details] Bug tested (case1) (In reply to Dani Megert from comment #69) > (In reply to Stefan Xenos from comment #68) > > > The first thing to do here is writing tests that assert correct > > > event sequences. > > > > I disagree with the claim that performing a synchronous layout prior to > > firing the event is the only way to fix this, so I wouldn't want to write a > > test asserting that this is the case. > > > When a client gets the listener notification that the part is open/on top, > then he can assume to safely call methods on SWT. Those methods will only > work if no deferred layouts are pending. That's all what the test should do. > Anything that would require the client to do something prior to calling the > SWT methods would be wrong. Dani, according comment #61, I'm able to reproduce the error in both Test Case 1 (see image) and 2 (Navigator is now deprecated). According Eric's comment #62, we should better understand the extensive use of SWT.DEFER, and ask the SWT guys to point us to the "best practices" for having propert SWT layouting. At this point we could follow Stefan's comment #69, by choosing to: (a) perform a sync layout request; (b) defer the layout operation and (c) implement a custom layout. As test, I could think to add a plugin contributing a view, to be displayed by a command (like in Navigator). Then in the test listen to to partOpened(), partBroughtToTop() and partActivated(), to verify the layout is NOT deferred. In the worst case, see Eric's comment #62, we could ask SWT guys to provide us an hook (API) to get synced (wait?) for a deferred layout :) To better understand, I'd like to ask you some questions: 1. Could you please give me a reference on "best practices" for "proper" layouting? 2. How would you suggest to sync with deferred layouts? 3. Do you know whether there is already a test, similar to the one I described, that I can reuse to verify the bug? Kind Regards.
> ask the SWT guys to point us to the "best practices" for having proper SWT layouting I'm probably the guy you want to ask. I did a lot of work on the SWT layout system and wrote the pseudocode that SWT.DEFER was based on. Sorry, I've been planning (and really looking forward) to fix this one, but my users have been keeping me so busy with JDT core stuff that I haven't had a chance to come back to it. Why are deferred events desirable? When you do something like switch perspectives, there will be a whole bunch of part events fired and a whole bunch of views and editors realized. If each one forces a synchronous layout, you end up doing a bunch of layouts when you could have done exactly one. You basically multiply the cost of the layout by the number of views. The SWT.DEFER thing marks all the controls that need to be recomputed and lays them out in a single pass. This is massively more efficient. However, in order to work properly, anything which positions a control needs to happen as part of the layout pass. Actually, that's not just due to deferred layouts. It's actually true all the time, but when using deferred layouts the consequences are much more visible. Even without deferred layouts, methods like getBounds() return the result from the most recent layout pass, not the currently-running layout pass... so if you're making a decision about a widget position based on it, your calculations are going to be slightly out of date. The results are particularly noticable when using deferred layouts, though, because the previous layout pass may have never happened so you'll get answers that are wildly off rather than just a few pixels off. I was planning to address this using a custom layout implementation which bases its decisions on the results of computeSize (which returns a hypothetical size that is always up-to-date), not the result of getBounds() or getSize() (which is always slightly out-of-date). If you're stumped and you want a rich set of layout examples, check out the Android source code. All the layouts are open-source and the layout algorithm is very similar to SWT's. However, in Android all layouts are deferred - there is no option to layout synchronously, so the source code is full of useful idioms for this sort of thing. An Android RecyclerView also faces this kind of problem -- the view gets instantiated synchronously and its owner may ask it to scroll to a particular row immediately after it is created, before the first asynchronous layout pass runs and the dimensions of the view or its contents are known. The code won't port directly, but it may be a useful source of inspiration since it has so many similarities to SWT. (Instead of a computeSize(...) that returns a Point, they have a method called measure(...) which returns stores its result in the receiver. You can access it by calling getMeasuredWidth/Height()...). In the case of Android, most application developers are unaware that the layouts are all asynchronous since they have useful helper methods like "scrollToPosition" that take a row number and figure out the pixel position on the next layout pass. IMO, SWT could offer similar helper methods so that each view and editor doesn't need to write a custom layout to make everything work correctly. Here's the code for it (LinearLayoutManager.java does the asynchronous magic which turns a synchronous method called by client code into something asynchronous that works with the deferred layout system): http://grepcode.com/file/repository.grepcode.com/java/ext/com.google.android/android/5.0.0_r1/android/support/v7/widget/LinearLayoutManager.java Notice that scrollToPosition remembers a "pending" scroll position independently of the actual control size and triggers an asynchronous layout. During the asynchronous layout (onLayoutChildren is very similar to SWT's "layout" method), it then figures out the actual pixel position for the scroll as the children are being layed out. iOS, QT, and every other layout system I've used does something very similar, but the code isn't as easy to read if you're used to SWT. I believe the same general approach will work here. Anyway. I'm lurking in the #eclipse-dev IRC channel most days if you'd like to discuss this with me... and if you don't end up fixing this, I promise I will - eventually - as soon as I'm done fixing all my critical JDT core bugs. :-)
Patrik, I wanted to follow up on our discussion on IRC. There was a suggestion to use readAndDispatch to force the deferred layout to happen. It will probably do that, but it will also execute unknown code that was scheduled via asyncExec and will eventually cause a hard-to-reproduce crash when one of those asyncExecs tries to close the very part you're opening or corrupts the workbench state by activating a different part. Don't do this. There was another suggestion to defer the work using asyncExec. Deferring the work is a good idea, but doing so with asyncExec won't work 100% of the time. It would be a race condition since there's no guarantee that your asyncExec will run after the deferred layout. Don't do this either. If I'm reading the above comments correctly, it looks like someone tried this and hit exactly this race. Instead, defer the work using a custom layout's Layout#layout method. This will always run at exactly the right time (as part of the next layout pass)... and running as part of the layout pass rather than after it is even better since - if you do it right - you'll only reposition the child widgets once rather than twice.
Taking the bug, I plan to solve this using a custom Layout#layout
> Taking the bug, I plan to solve this using a custom Layout#layout Good to hear. Let me know what you come up with. Once we understand the problem better, it may make sense to push the solution up to SWT so that others who face similar problems won't have to do such an elaborate fix.
Patrik: Are you still working on this? Anything I can do to help?
Stefan, I re-started working on this after a long pause. - I reproduced the bug, by get+editing org.eclipse.jface.text.TextViewer class (eclipse.platform.text\.git) As we agreed in IRC, I should create a new custom layout and attach the custom layout to TextViewer. I wasn't very clear how to create and assign the custom layout, so I logged the layout responsible of this bug, that is : org.eclipse.e4.ui.workbench.renderers.swt.TrimmedPartLayout (eclipse.platform.ui\.git) This layout is instantiated once, at startup, for each workbench window, in: WBWRenderer.createWidget(MUIElement, Object) line: 376 Construction and assignment are as you can see: TrimmedPartLayout tl = new TrimmedPartLayout(wbwShell); // .. wbwShell.setLayout(tl); So, according what we agreed, I should create a new layout (#1, #2) then replace the TrimmedPartLayout, unless we want to just "edit" it, instead of creating a new one. To be honest, I'm more for editing the existing TrimmedPartLayout, instead of creating a new one. Thanks for giving your time. Any suggestion is welcome. #1 http://www.eclipse.org/articles/article.php?file=Article-Understanding-Layouts/index.html #2 http://www.java2s.com/Tutorial/Java/0280__SWT/CreatingYourOwnLayouts.htm
Mass move to 4.7 as M7 is approaching. Please move back in case you are planning to fix it for Neon.
Moving target milestone to 4.9 for all bugs that are major or above.
Moving target milestone to 4.11 for all bugs that are major or above.
Mass change, please reset target if you still planning to fix this for 4.11.
Please set the target milestone back to 4.12 if you still intend to fix this for 4.12.
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.
