Community
Participate
Working Groups
Created attachment 267410 [details] Editor lagging while scrolling Dragging the scrollbar does not work properly anymore on 4.7 M6. The editor's content is updated very slowly and fast scrolling is almost impossible. With 4.7 M5 everything works as usual. I've attached a video which shows the lag. For editable files (.java) the lag is much worse. Steps: 1. create a new workspace and use the dark theme (the normal theme seems to work fine) 3. create a java project and open a file (f. e. String.class) 4. Try scrolling via dragging the mouse bar -> The lag should occur.
Created attachment 267411 [details] Editor lagging while scrolling
I can't reproduce on Linux, seem to be Windows only. Probably side effect from the fix for bug 511596 or bug 511597, and probably related to "themed" scrollbars (see bug 430278), which explains why only the Dark theme is affected (and only on Windows). Fabio, any ideas?
I must say that on 4.6.1 on Windows I can't reproduce it either (well, the report is really some change from 4.7M5 to 4.7M6 and as far as I know, nothing really changed in the themed scrollbar). On wild guesses I'd say something else became asynchronous or the drawing of some other component became slower -- from the bugs you linked, https://bugs.eclipse.org/bugs/show_bug.cgi?id=511596 seems the most likely to be the culprit here, and it should be reasonably easy to revert that locally to see if things work with it reverted (unfortunately I don't really have the time to actually check it right now).
As a note, if someone will be able to take a look at this, the themed scrollbar does: StyledText.setTopPixel(pixel); ScrollBar.notifyListeners(SWT.Selection, e); This happens in: com.brainwy.liclipse.theme.styled_text.StyledTextThemedScrollBarAdapter.StyledTextHorizontalScrollHandler.setPixel(Scrollable, int) and com.brainwy.liclipse.theme.styled_text.AbstractScrollHandler.notifyScrollbarSelectionChanged(Scrollable, int) and expects things to be updated with that... so, if any of that became asynchronous, this would explain the behavior. Maybe a forced draw to the StyledText is now required after StyledText.setTopPixel/setHorizontalPixel (in case of a horizontal scroll)?
Oops, the paths should actually be: org.eclipse.e4.ui.internal.css.swt.dom.scrollbar.StyledTextThemedScrollBarAdapter.StyledTextVerticalScrollHandler.setPixel(Scrollable, int) and org.eclipse.e4.ui.internal.css.swt.dom.scrollbar.AbstractScrollHandler.notifyScrollbarSelectionChanged(Scrollable, int)
(In reply to Fabio Zadrozny from comment #3) > On wild guesses I'd say something else became asynchronous or the drawing of > some other component became slower -- from the bugs you linked, > https://bugs.eclipse.org/bugs/show_bug.cgi?id=511596 seems the most likely > to be the culprit here, and it should be reasonably easy to revert that > locally to see if things work with it reverted (unfortunately I don't really > have the time to actually check it right now). Stefan, can you take a look?
I should have a windows machine at my disposal to investigate in the next week or so.
New Gerrit change created: https://git.eclipse.org/r/96058
Ok, just took a look at this and fixed it by forcing an update when scrolling. The setPixel call does call a redraw, but the SWT Control#redraw docs says: It is not necessary for the caller to explicitly call {@link #update()} after calling this method, but depending on the platform, the automatic repaints may be delayed considerably. So, the patch waiting for someone to review/integrate calls StyledText#update when the user does an interaction that changes the scrollbar selection from the themed scrollbar. As it's only during the user interaction, it shouldn't have any other side effects and doesn't appear to slow anything down for me.
I guess it works, but I think we may be able to do better. Forcing a repaint inside an event handler has the possibility of backing up the event queue if the events arrive at faster rate than we can paint them. We've seen this problem frequently on gtk and osx. It may also happen on slower win32 machines. I'd suggest fixing SWT so that paint events are never delayed by too much on any platform. I'd suggest this algorithm (implemented in SWT - not the application): someoneCalledRedraw() { if (!aPaintIsQueued) { aPaintIsQueued = true; asyncExec(() -> { aPaintIsQueued = false; performAPaintNow(); }); } } That would ensure that neither input events nor paints could starve one another. As long as there is a dirtied control, there would be a paint enqueued in the event queue and subsequent events couldn't delay that paint. However, there would never be more than one paint enqueued so there would be no way for paints to cause the event queue to back up if the paints are too slow. By putting the algorithm in SWT, nobody would ever need to force an update on any platform. We could then delete the code that's currently forcing repaints in the ruler (and we should -- painting the ruler out of sync with the editor body will look strange and forcing a paint in the ruler's event handler can would cause the queue to back up and force the editor body to paint less frequently than it would otherwise). It's possible that the forced paints (whatever is making the ruler paint so smoothly in the video) is already causing such an event backlog. This may be the reason the rest of the paint events are getting delayed so much. If that's the case, forcing repaints in the editor is exactly the wrong thing to do (it will look smooth but introduce input lag). It might be interesting to see how smoothly the editor would scroll if the ruler wasn't forcing repaints. It might turn out that deleting the calls to update() in both places would be sufficient to fix the bug.
Hi Stefan, I'm not 100% sure that in this particular case it's the best approach, since anything asynchronous during the scroll is going to be a bit odd to the user (the native scroll does a repaint which appears synchronous in response to each mouse move during the scroll, and I think that the themed scroll should follow the same pattern -- the only difference here is that it's governed by the system, which is not the case on the themed scrollbar)... even if he's on a slower machine, it may be better getting more delay due to the painting as that probably also happens during a native scroll in such a machine -- than getting the scroll unsynchronized with the editor. As a note, in this case what I'm seeing is that there's no other other update being called (I see a redraw for each mouse move being requested, but no paint event up to some time later). Note that I don't disagree with you that in general the approach you outlined is a good approach, but in this particular case, I think the scroll and the editor should be synchronized... ideally the theming would be in SWT and this would all be there not made from the outside, but SWT devs don't think that's a good idea either, so, I still think the proposed approach is better in this specific situation given the limitations... and I think that Display.asyncExec could be already appear unsynchronized to the user depending on the cpu load. Although I may be wrong, so, if you think the solution you outlined would be the preferred approach here, I think it'd be better for an actual SWT dev to do it, so that such a change can be supported by someone with more authority on SWT than me as I'm not sure what are going to be the implications of doing the changes you outlined.
As a second note, I didn't really see anyone else call a Control#update... The ruler has a different approach which on redraw creates a GC and paints over the ruler... I took a look at it and found a curious comment saying: On Windows, we can't switch to direct-paint mode, because calls to update() have been removed, and now repaints would only happen with huge delays when scrolling, see <a href="https://bugs.eclipse.org/511596#c26">bug 511596 comment 26</a>. So, it seems the delay for paint is true in Windows not only on the case for the StyledText... creating a new GC and do a manual paint as it was solved on the VerticalRuler doesn't seem a good approach though -- I think calling #update() is less worse than that ;)
> The ruler has a different approach which on redraw creates a GC and > paints over the ruler... That's just another way to force a synchronous paint. I guess I was advocating for never using synchronous paints. :-) > Display.asyncExec could be already appear unsynchronized to the > user depending on the cpu load. Yeah. True, but that's because SWT executes actual asyncExecs at a lower priority than it should. However, it still could use some sort of asynchronous timing, even if it isn't an actual asyncExec. I'm not necessarily against your original approach. I'm just trying to offer some alternative suggestions to consider. Your original patch is certainly better than the status quo, so if we don't have time to test any of my alternatives yet we should use your patch in the meantime.
I really like Stefan's suggestions for an improvement in SWT. Given that M7 is around the corner I suggest to merge Fabio's patch and open a follow-up bug for investigation of Stefan's suggestions.
(In reply to Lars Vogel from comment #14) > I really like Stefan's suggestions for an improvement in SWT. > > Given that M7 is around the corner I suggest to merge Fabio's patch and open > a follow-up bug for investigation of Stefan's suggestions. Since you suggest to merge the patch, did you review and test it?
(In reply to Dani Megert from comment #15) > (In reply to Lars Vogel from comment #14) > > I really like Stefan's suggestions for an improvement in SWT. > > > > Given that M7 is around the corner I suggest to merge Fabio's patch and open > > a follow-up bug for investigation of Stefan's suggestions. > > Since you suggest to merge the patch, did you review and test it? In this case I would have merged it.
I can't reproduce this on Windows 7 with http://download.eclipse.org/eclipse/downloads/drops4/I20170430-2000/download.php?dropFile=eclipse-SDK-I20170430-2000-win32-x86_64.zip Calling StyledText#update while scrolling is not a good idea and badly affects bug 366471.
(In reply to Dani Megert from comment #17) > I can't reproduce this on Windows 7 with > http://download.eclipse.org/eclipse/downloads/drops4/I20170430-2000/download. > php?dropFile=eclipse-SDK-I20170430-2000-win32-x86_64.zip > > > Calling StyledText#update while scrolling is not a good idea and badly > affects bug 366471. If you scroll really fast, you should be able to reproduce it even on a fast machine (compare the native and themed versions: the themed version is always a bit off)... also, the issues on #update are only MacOS/Linux, this code is Windows only (so, it shouldn't be an issue) -- see comment from Markus Keller at: https://bugs.eclipse.org/511596#c26 (if you want I can add a check just to guarantee, as it was done on the overview ruler -- which doesn't call update but has a completely different approach so that a synched draw in done on windows -- see the comments on: org.eclipse.jface.text.source.VerticalRuler.AVOID_NEW_GC and places which use it as a reference). If you want I can also add a check to put the update on windows only (which is the same solution given in org.eclipse.jface.text.source.VerticalRuler.AVOID_NEW_GC). I must say that the themed scroll is even annoying on 4.6.3 (I think the "improvements" to Mac and Linux were backported and the themed scroll now appears unsynched with the editor even on 4.6.3).
(In reply to Fabio Zadrozny from comment #18) > If you want I can also add a check to put the update on windows only Are you 100% sure that this reported bug is Windows only? In normal working mode I can't reproduce it but when I run in Debug mode with a tracepoint set in #notifyScrollbarSelectionChanged I can see the text lagging behind. On the other hand, with the fix the scrolling itself gets slower because it repaints.
Re: comment 17 I agree that calling update() during scroll is extremely dangerous. I'd be disinclined to merge this patch unless we'd tested the heck out of it first and confirmed that it does't cause a regression in bug 511596 on any platform. Out-of-sync scrolling or ugly scrolling is much better than laggy scrolling. > also, the issues on #update are only MacOS/Linux, this code > is Windows only (so, it shouldn't be an issue) The issues with #update are on any platform where input events arrive faster than the paints can execute. Windows tends to have fast paints, but I'd bet that a windows machine with a really bad video card would demonstrate the same behavior.
Another solution to the out-of-sync problem would be to delete the custom paint code that currently performs syncronous paints in the ruler and just let the ruler paint the normal way. That would mean the ruler would be choppier to scroll but it would match the editor. It would also cause the event queue to be processed faster which should allow the editor to repaint faster. It would also delete all the current custom windows-specific code in the ruler.
I'm proposing comment 21 as something to try -- but we shouldn't commit to any solution until we'd coded a few of them and compared them.
(In reply to Stefan Xenos from comment #22) > I'm proposing comment 21 as something to try -- but we shouldn't commit to > any solution until we'd coded a few of them and compared them. I think this is worth a try but something I'd rather do early 4.8.
(In reply to Dani Megert from comment #19) > (In reply to Fabio Zadrozny from comment #18) > > If you want I can also add a check to put the update on windows only > > Are you 100% sure that this reported bug is Windows only? > > > In normal working mode I can't reproduce it but when I run in Debug mode > with a tracepoint set in #notifyScrollbarSelectionChanged I can see the > text lagging behind. On the other hand, with the fix the scrolling itself > gets slower because it repaints. I'm a 100% sure this is windows-only (the themed scrollbar is only available on windows)... I think the video is an extreme case, but in my usual working env, it's already noticeable to be annoying with 4.6.3 (I guess this is because the call to #update was removed from the overview ruler from 4.6.2 to 4.6.3 -- I can also use the same "hack" which was used in the overview ruler -- mainly creating a new GC and then call trigger an SWT.Paint with that GC to avoid the #update call if you feel it's better). A question (Daniel & Stefan): is your main working env on a dark theme on Windows (where this is the issue)... I must say that it's mine and on the day to day this sluggishness is pretty annoying after the calls to update have been removed (presumably because of issues on Linux & Mac where the experience is very different), and I believe the current status quo right now is not really acceptable (i.e.: what happens on the video because of the unsynched scroll should *never* happen). I'm Ok if you provide and test a better solution, but I must say that with the current patch I didn't have any issues and things appear much smoother on the themed scroll on Windows (where this issue happens).
(In reply to Fabio Zadrozny from comment #24) > > Are you 100% sure that this reported bug is Windows only? > > I'm a 100% sure this is windows-only (the themed scrollbar is only available > on windows)... Where can I see that? I quickly checked the Javadoc and that does not mention such a restriction. Please provide details. If that's really the case maybe we should rename the relevant types to include "Windows".
Well, Ok, there's nothing that really prevents the code from being used on Linux or Mac (so, just pushed a new patch which makes sure that the change is done only on Windows, as on Linux/Mac it doesn't seem to take as long after a redraw to actually paint the control again)... The only thing is that we only enable it by default in the Dark theme when the user is on Windows.
(In reply to Fabio Zadrozny from comment #26) > The only thing is that we only enable it by default in the Dark theme when > the user is on Windows. And why is that? And where is the code that shows this?
Because the scrollbars are much more annoying on Windows (in Linux the user can usually configure the scrollbars to work with overlays or even change the color and -- as for Mac, I'm not really familiar with it, but I haven't seen users complain, so, I guess it's better too?). You can see the related setting at: eclipse.platform.ui\bundles\org.eclipse.ui.themes\css\e4-dark_win.css has swt-scrollbar-themed: true; eclipse.platform.ui\bundles\org.eclipse.ui.themes\css\dark\e4-dark_globalstyle.css swt-scrollbar-themed: false; But the user can opt-in by using -Dswt.enable.themedScrollBar (which is why the check is really needed).
> is your main working env on a dark theme on Windows My main env in gtk. I haven't tested your patch - I'm just taking the word of you and Dani as to the actual behavior in practice.
(In reply to Stefan Xenos from comment #29) > > is your main working env on a dark theme on Windows > > My main env in gtk. I haven't tested your patch - I'm just taking the word > of you and Dani as to the actual behavior in practice. Well, in this case, one thing you can do is enable the themed scroll on your own system (it's an opt-in when using the dark theme through -Dswt.enable.themedScrollBar) to see how is the behavior in that case (although from the solution in the overview ruler, I'd say it should be working fine -- the latest patch should change things only on windows).
Yeah, I'm okay with this sort of hack if-and-only-if you've tested it for the sort of input lag that was observed in bug 511596. As long as the calls to update() can only occur on Windows, windows testing should be sufficient.
(In reply to Stefan Xenos from comment #31) > Yeah, I'm okay with this sort of hack if-and-only-if you've tested it for > the sort of input lag that was observed in bug 511596. > > As long as the calls to update() can only occur on Windows, windows testing > should be sufficient. Well, I do get a lag, it's not as apparent as the one in the video (where it's *really* horrible), but it's definitely noticeable for me (and it happens on 4.6.3 and in 4.7m6, whereas it didn't happen in 4.6.2 nor 4.7m5). The patch does fix it though. As a note, I'm not against having a change on SWT so that redraw is more real-time on windows without the need for a synched drawing, but I definitely won't be able to find time to do such a change myself before 4.8 due to the lack of time -- but if someone does volunteer to fix it in a way that's more elegant, I'm all for it (but if not, I think that the patch I provided should be applied to fix the current issue on Windows).
> Well, I do get a lag, it's not as apparent as the one in the video I think maybe we're talking about two different things when we say "lag". The lag in bug 511596 was a delay between the user supplying a mouse input and the UI reacting to that input - see the videos there. To test it properly, you need to scroll up and down rapidly with the scroll wheel, and a video that demonstrates it would need to capture both the user's hand on the mouse and the behavior of the screen. The lag captured in this video was an unacceptably low framerate for editor repaints. However, at the time it painted it was displaying the most up-to-date position of the mouse (so there was no "lag" in the same sense as bug 511596). This should be reproducible with any sort of scrolling motion and wouldn't require rapid inputs. We should test for both.
Gerrit change https://git.eclipse.org/r/96058 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=c101bdccdae81c5cb515fe3ace3752ca79f65e0e
(In reply to Fabio Zadrozny from comment #28) > Because the scrollbars are much more annoying on Windows (in Linux the user > can usually configure the scrollbars to work with overlays or even change > the color and -- as for Mac, I'm not really familiar with it, but I haven't > seen users complain, so, I guess it's better too?). > > You can see the related setting at: > > eclipse.platform.ui\bundles\org.eclipse.ui.themes\css\e4-dark_win.css has > swt-scrollbar-themed: true; > > eclipse.platform.ui\bundles\org.eclipse.ui.themes\css\dark\e4-dark_globalstyle.css > swt-scrollbar-themed: false; > > But the user can opt-in by using -Dswt.enable.themedScrollBar (which is why > the check is really needed). Thanks. I've merged the change.
