Community
Participate
Working Groups
Created attachment 242548 [details] Screenshot OK up to I20140218-0800 Broken in N20140218-2000, I20140225-0800, and later Trees sometimes don't redraw and leave cheese in Navigator and Git Staging views. Steps: - new workspace, close Welcome - paste this snippet into Package Explorer: package p; public class C { public static void main(String[] args) { int i= 2; System.out.println(i); } } - Show In > Navigator - click the Problems view tab - click "C.java" in the Navigator view => Only "C.java" is rendered, the rest of the Navigator view shows cheese (white background colors; in some recent I-builds even the blue "view background" color) When I revert the fix for bug 422894 (e58f1c7d25caff77c8f2028da69e2df87a9685c0), then the cheese is replaced by a flash of the whole Navigator view.
The same effect sometimes also shows up in the Git Staging view when I select a file. It doesn't always show up when giving focus to an affected view. I've never seen it in views that use custom draw (Package Explorer, Project Explorer, Outline, ...).
Weird. I suspect this is a problem in SWT where setting a background isn't triggering a redraw on the children. I've seen problems like this when running an inner instance from within Eclipse on OSX, but I have never in the outer instance. (There are a few oddities like this in the inner instance e.g. bug 378202.)
Really weird. Now, I can still reproduce the original problem after reverting the commit I cited in comment 0. However, the problem really appears somewhere between I20140218-0800 and N20140218-2000.
It can be affected by the changes introduced with the Bug 430829 Daniel
I've reverted the changes for bug 422894 and bug 430829 and I can still reproduce. Not as frequently, but it's still reproducible. This problem seems to have something to do with event timing/ordering. (And I can reproduce in the IDE itself with just the Resource Navigator and the Problems view.) If I disable CSS properties (modify AbstactCSSEngine#applyStyles() to return immediately), the problem never occurs. One of the properties is causing some kind of change, somehow.
(In reply to Brian de Alwis from comment #5) > I've reverted the changes for bug 422894 and bug 430829 and I can still > reproduce. Not as frequently, but it's still reproducible. This problem > seems to have something to do with event timing/ordering. But restarting my debug image again, I now can't reproduce the problem.
(In reply to Brian de Alwis from comment #6) > (In reply to Brian de Alwis from comment #5) > > I've reverted the changes for bug 422894 and bug 430829 and I can still > > reproduce. Not as frequently, but it's still reproducible. This problem > > seems to have something to do with event timing/ordering. > > But restarting my debug image again, I now can't reproduce the problem. I had just finished preparing a commit to revert the changes, and restarted my debugger and now see the problem again. So this is not caused by the recent CSS performance changes. Unfortunately I haven't yet figured out the cause. I've pushed these reverts as a change to: https://git.eclipse.org/r/25825
I've started hacking AbstractCSSEngine#applyCSSProperty() to filter out the different CSS rules being applied, and the key seems to be the "swt-unselected-tabs-color" property, applied by the CSSPropertyUnselectedTabsSWTHandler handler. When provided a gradient, it continuously changes the CTabFolder's background.
I have an easily reproducible scenario on Windows: 1. Open the Navigator view, find a Java file (the C.java in Markus' workflow above). 2. Double click the Java file to activate the editor. 3. Single click the Java file to bring focus back to the Navigator What seems to be the problem is that there are two particular CSS properties that applied to the CTabFolder and the hosting Composite that change the background color, the CTabFolder's background gradient, and the background image. One of these somehow messes up the Tree widget If I cause the following properties to be skipped, then I don't see these cheese issues: swt-unselected-tabs-color: #org-eclipse-ui-workbench-INACTIVE_UNSELECTED_TABS_COLOR_START #org-eclipse-ui-workbench-INACTIVE_UNSELECTED_TABS_COLOR_END 100.0% 100.0% background-color: #org-eclipse-ui-workbench-INACTIVE_UNSELECTED_TABS_COLOR_START #org-eclipse-ui-workbench-INACTIVE_UNSELECTED_TABS_COLOR_END 100.0% 100.0% What I see in my tracing is that these properties lead to the following changes: swt-unselected-tabs-color: 1399063807027[CTabFolder@28136669] setBackground(Color[],int[],bool) 1399063807027[CTabFolder@28136669] setBackground(Color) 1399063807027[CTabFolder@28136669] setBackground(Color) 1399063807027[Composite@26920745] setBackgroundImage() 1399063807027[Composite@26920745] setBackground(Color) 1399063807027[CTabFolder@28136669] CTabFolder#redraw(false) 1399063807027[CTabFolder@28136669] CTabFolder#redraw(false) background-color: 1399063807027[Composite@26920745] setBackgroundImage() I'm now seeing if I can recreate a similar sequence of events in a standalone SWT app.
It is also broken in the 'classic' theme so it shouldn't be connected to the 'swt-unselected-tabs-color' property Daniel
It can be caused by the same issue as reported in the Bug 433723 Daniel
Daniel, take a look at bug 401633 comment #2. Could this be part of this issue ?
(In reply to Eric Moffatt from comment #12) > Daniel, take a look at bug 401633 comment #2. Could this be part of this > issue ? Unfortunately it doesn't fix the issue Anyway thanks for the clue, Daniel
The issue seems to be caused by the patch for the Bug 408849, the line: if (pStack != null) styleElement(pStack, true); Skipping the 'styleElement' method invocation solves the issue I will continue my investigation, Daniel
(In reply to Daniel Rolka from comment #14) > The issue seems to be caused by the patch for the Bug 408849, the line: > > if (pStack != null) > styleElement(pStack, true); > > Skipping the 'styleElement' method invocation solves the issue Skipping the mentioned line corrupts the styling of the MPartStack with the 'active' state so it is not the solution Daniel
I am not able to reproduce the problem after reverting commit http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=e58f1c7d25caff77c8f2028da69e2df87a9685c0 . Tested with M7 + the latest sources and with build N20140212-2000 + commits up to 2014.02.18.
(In reply to Wojciech Sudol from comment #16) > I am not able to reproduce the problem after reverting commit > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=e58f1c7d25caff77c8f2028da69e2df87a9685c0 . > Tested with M7 + the latest sources and with build N20140212-2000 + commits > up to 2014.02.18. (In reply to Daniel Rolka from comment #14) > The issue seems to be caused by the patch for the Bug 408849, the line: > > if (pStack != null) > styleElement(pStack, true); > > Skipping the 'styleElement' method invocation solves the issue > > I will continue my investigation, > Daniel The issue is directly caused by the optimization used by the Navigator/TreeViewer related to the redrawing of the Tree when the selected child doesn't change. In such case only the selected item gets repainted. Without the commit 'e58f1c7d25caff77c8f2028da69e2df87a9685c0', pointed by Wojtek, we always reset the fonts of the Tree/TreeItems and invalidate the entire tree. So after this step the entire tree gets repainted (without producing the cheese) We can expect similar issues modifying the CSS style sheets for the plugins that use similar improvements during handling the OnPaint events I will prepare the patch for it, Daniel
(In reply to Daniel Rolka from comment #17) > The issue is directly caused by the optimization used by the > Navigator/TreeViewer related to the redrawing of the Tree when the selected > child doesn't change. In such case only the selected item gets repainted. > Without the commit 'e58f1c7d25caff77c8f2028da69e2df87a9685c0', pointed by > Wojtek, we always reset the fonts of the Tree/TreeItems and invalidate the > entire tree. So after this step the entire tree gets repainted (without > producing the cheese) > > We can expect similar issues modifying the CSS style sheets for the plugins > that use similar improvements during handling the OnPaint events > > I will prepare the patch for it, > Daniel I've prepared the first version of the patch the solves the issue - https://git.eclipse.org/r/#/c/26257/ However I will try to prepare another version of the patch that solves the issue in better manner. Anyway, as I've mentioned in my previous comment, we have to be aware of the similar issues when we add/remove any rules from the CSS files that refresh the Widget's parent, but the Widget itself gets partially refreshed only (and we get the cheese). Daniel
Thanks Daniel and Brian for taking this forward. I don't understand all of your problem analysis, but it sounds like there's also a bug in SWT. If you find a good workaround in Platform UI, then that's fine for Luna. But if you can nail it down to a problem in SWT, then please don't forget to file a bug so that the underlying problem can be fixed in Mars.
(In reply to Wojciech Sudol from comment #16) > I am not able to reproduce the problem after reverting commit > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=e58f1c7d25caff77c8f2028da69e2df87a9685c0 . > Tested with M7 + the latest sources and with build N20140212-2000 + commits > up to 2014.02.18. As I mentioned in comment #7 I was able to reproduce these issues even after backing out the fixes. It may occur more rarely, but it's still possible. That particular commit prevents a significant UI regression seen on Linux GTK. We don't see that behaviour in the IDE on Linux, but I have experienced it first hand with an RCP app with custom CSS. I commented on Daniel's patch that a better approach IMHO is to put a Windows-specific fix in the CSS property implementations that sets the background to re-set the font so as to trigger a refresh. It's only a workaround: SWT generally is pretty careful to avoid unnecessary work, so that setting the foreground/background to the same colour is a null-op, and the fact that setFont() doesn't perform such a check is an oversight.
(In reply to Brian de Alwis from comment #20) > (In reply to Wojciech Sudol from comment #16) > > I am not able to reproduce the problem after reverting commit > > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > > ?id=e58f1c7d25caff77c8f2028da69e2df87a9685c0 . > > Tested with M7 + the latest sources and with build N20140212-2000 + commits > > up to 2014.02.18. > > As I mentioned in comment #7 I was able to reproduce these issues even after > backing out the fixes. It may occur more rarely, but it's still possible. > You have to be updated to the M7 or use the M7 build when you revert the fixes. Without the M7 and the 'e58f1c7d25caff77c8f2028da69e2df87a9685c' commit the issue still occurs. > That particular commit prevents a significant UI regression seen on Linux > GTK. We don't see that behaviour in the IDE on Linux, but I have > experienced it first hand with an RCP app with custom CSS. > > I commented on Daniel's patch that a better approach IMHO is to put a > Windows-specific fix in the CSS property implementations that sets the > background to re-set the font so as to trigger a refresh. It's only a > workaround: SWT generally is pretty careful to avoid unnecessary work, so > that setting the foreground/background to the same colour is a null-op, and > the fact that setFont() doesn't perform such a check is an oversight. It is the more general issue and we shouldn't limit it to the Tree widget and the Windows platform only. I'm working on the better fix for it Daniel
Created attachment 242954 [details] CheeseIssueSimulator (In reply to Markus Keller from comment #19) > I don't understand all of your problem analysis, but it sounds like there's > also a bug in SWT It is quite important to understand the root cause of the issue so let me try to explain it using the 'cheese simulator' example that replicates it and the description below. How to generate the cheese with the CSS: 1) We have some plugin (i.e. Navigator) that improves the rendering operations. It is mainly done in order to improve the performance of displaying the GUI items or eliminate the flicking during redrawing it 2) We have some CSS rule that refreshes (repaints with the background color) the parent Widget (the 'TreePaintListener.redrawTree' method in our example) 3) During redrawing of the parent's children (the 'TreePaintListener.redrawTreeItems' method in our case) the optimization is used in order to improve the process (the 'TreePaintListener.getChildrenToDraw/if selected != null)' line in our case) 4) However we don't have another CSS rule/some place in the source code that skips the optimization from the previous step (bypasses the 'TreePaintListener.getChildrenToDraw/if (selected != previousSelected)' line in our example) and we get the gaps (cheese) I don't know maybe it is caused by some SWT bug that works in this way, Daniel
OK, I've pushed the final version of the patch for the issue to Gerrit. Currently we support the Tree widgets only (I don't want to add other Widgets since they seem to be not affected with it. We can add it later, if needed) It is the general solution that, I believe, allows us to forget about the issue during editing the CSS files (that I've mentioned in the #Comment 22/4) When we confirm that it is the SWT issue I will file the proper SWT bug thanks for review, Daniel
Great diagnosis. There's some flicker from the redraw, but that's preferable to the alternative.
Released as http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=052d5d66f0a1b483abcc639dc36dfb9820a326dc PW
(In reply to Brian de Alwis from comment #24) > Great diagnosis. There's some flicker from the redraw, but that's > preferable to the alternative. Yes, the current fix is an improvement (or better: workaround for the real issue) but the flickering still needs to be addressed because it affects every tree which does not use custom draw (e.g. 'Navigator' or 'Git Staging' view). And if a user has disabled 'Use mixed fonts and colors for labels', then he sees the flickering in all our tree-based views, like Package Explorer', 'Project Explorer' and 'Outline'. Test Case: 1. select an item in the 'Navigator' 2. activate any other part 3. click on the previously selected item ==> whole view flashes
(In reply to Dani Megert from comment #26) > (In reply to Brian de Alwis from comment #24) > > Great diagnosis. There's some flicker from the redraw, but that's > > preferable to the alternative. > > Yes, the current fix is an improvement (or better: workaround for the real > issue) but the flickering still needs to be addressed because it affects > every tree which does not use custom draw (e.g. 'Navigator' or 'Git Staging' > view). And if a user has disabled 'Use mixed fonts and colors for labels', > then he sees the flickering in all our tree-based views, like Package > Explorer', 'Project Explorer' and 'Outline'. > > Test Case: > 1. select an item in the 'Navigator' > 2. activate any other part > 3. click on the previously selected item > ==> whole view flashes Probably the optimization during rendering of the Tree, that I've mentioned in one of my comments (avoiding of redrawing the entire Tree when the selection didn't change), has been introduced to eliminate/reduce the flicking. The current patch produces one additional flick - during gaining the focus by the Tree Daniel
I think we should assign the flicking issue to the proper plugin i.e. Navigator Daniel
(In reply to Daniel Rolka from comment #28) > I think we should assign the flicking issue to the proper plugin i.e. > Navigator > > Daniel No! The flickering happens with every view that has no owner draw, or when it is disabled via preference. Also note that the flickering only happens when one selects the previously selected item. Clicking another item or giving focus by clicking the view tab does not cause the flickering.
(In reply to Dani Megert from comment #29) > (In reply to Daniel Rolka from comment #28) > > I think we should assign the flicking issue to the proper plugin i.e. > > Navigator > > > > Daniel > > No! The flickering happens with every view that has no owner draw, or when > it is disabled via preference. > > Also note that the flickering only happens when one selects the previously > selected item. Clicking another item or giving focus by clicking the view > tab does not cause the flickering. All what I can do in terms of flicking is another workaround - https://git.eclipse.org/r/#/c/26530/ Unfortunately flicking during redrawing seems to be the OS specific issue. I think we should consider some double buffering solution on the SWT site what would be good for all SWT users. I was not able to eliminate it with any 'redraw' related SWT method. Basically we need the redraw in order to fix the refreshing issues connected to the overriding of the Widget's properties via CSS. Adding some artificial CSS rules just to refresh the Widget is not an option according to me Daniel
(In reply to Daniel Rolka from comment #30) > (In reply to Dani Megert from comment #29) > > (In reply to Daniel Rolka from comment #28) > > > I think we should assign the flicking issue to the proper plugin i.e. > > > Navigator > > > > > > Daniel > > > > No! The flickering happens with every view that has no owner draw, or when > > it is disabled via preference. > > > > Also note that the flickering only happens when one selects the previously > > selected item. Clicking another item or giving focus by clicking the view > > tab does not cause the flickering. > > All what I can do in terms of flicking is another workaround - > https://git.eclipse.org/r/#/c/26530/ That doesn't work for me: still flickers.
I don't see a problem on linux with master (no flickering). I don't see a problem on linux even if I revert http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=052d5d66f0a1b483abcc639dc36dfb9820a326dc What's the next step here? If https://git.eclipse.org/r/#/c/26530/ doesn't address the flickering for you Dani, does it make it better? Or do we throw out 052d5d66f0a1b483abcc639dc36dfb9820a326dc and try and modify the original change to only target linux? PW
Created attachment 243132 [details] Fix (In reply to Paul Webster from comment #32) > I don't see a problem on linux with master (no flickering). I don't see a > problem on linux even if I revert > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=052d5d66f0a1b483abcc639dc36dfb9820a326dc > > What's the next step here? If https://git.eclipse.org/r/#/c/26530/ doesn't > address the flickering for you Dani, does it make it better? > > Or do we throw out 052d5d66f0a1b483abcc639dc36dfb9820a326dc and try and > modify the original change to only target linux? > > PW The fix is as follows: 1. revert 052d5d66f0a1b483abcc639dc36dfb9820a326dc 2. apply this patch
(In reply to Dani Megert from comment #33) > The fix is as follows: > 1. revert 052d5d66f0a1b483abcc639dc36dfb9820a326dc > 2. apply this patch Paul, can you review it and verify that bug 422894 is still fixed on Linux? Thanks.
I've opened the SWT bug for the flicking issue - Bug 434980. I think the patch on the SWT site will allow us to simplify our implementation/improve performance (we will be able to get rid of any workarounds that we have now for it) Daniel
(In reply to Dani Megert from comment #34) > (In reply to Dani Megert from comment #33) > > The fix is as follows: > > 1. revert 052d5d66f0a1b483abcc639dc36dfb9820a326dc > > 2. apply this patch > > Paul, can you review it and verify that bug 422894 is still fixed on Linux? > Thanks. OK, I've tested with this scenario. 1. I tested Bug 422894 comment #4 and the font mentioned there is definitely being applied, and there's no problem clicking on an item that both activates the part and selects that item 2. I can't reproduce the scenario in this bug 3. I can't see any flickering in the Project Explorer or Navigator in any of my test scenarios. Dani, do you want me to push this fix up? PW
(In reply to Paul Webster from comment #36) > Dani, do you want me to push this fix up? > > PW Yes, please go ahead.
Reverted old fix with http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=680b7c130ca69e04ead87965de22b4ec855cf0f0 and applied new fix with http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=0402ebd10a57f9c2ca5cd2da3479470f98f70973 PW
(In reply to Paul Webster from comment #38) > Reverted old fix with > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=680b7c130ca69e04ead87965de22b4ec855cf0f0 and applied new fix with > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=0402ebd10a57f9c2ca5cd2da3479470f98f70973 > > PW It is the workaround that only hides the cheese issue. I agree that it is the best option for now since we are late in the 4.4 release. However I would skip the bug opened and move it to the 4.5 release. When the Bug 434980 gets fixed we can fix the issue in the proper way (applying all reverted patches including the 052d5d66f0a1b483abcc639dc36dfb9820a326dc one and maybe add some additional code if needed) In the Eclipse 3.x we were be able quite easy to control redrawing of the Widgets and eliminate refreshing/flicking issues by proper optimizations. Now when we added the new external layer - the CSS engine - and we modify plenty of SWT properties "externally", our control of refreshing/flicking issues is limited. The current workaround works fine for us, but someone can add some new CSS rule to their customized style sheet and the flicking will be back, So we have to have proper guards in the CSS engine in order to avoid it Daniel
(In reply to Daniel Rolka from comment #39) > (In reply to Paul Webster from comment #38) > > Reverted old fix with > > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > > ?id=680b7c130ca69e04ead87965de22b4ec855cf0f0 and applied new fix with > > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > > ?id=0402ebd10a57f9c2ca5cd2da3479470f98f70973 > > > > PW > > However I > would skip the bug opened and move it to the 4.5 release. When the Bug > 434980 gets fixed we can fix the issue in the proper way Please open a new bug report that describes the current state and make it depend on bug 434980. Thanks.
Verified in I20140515-1230. The fix is not optimal, but so far we haven't found a better one. We will have to fix this during Mars via bug 435024.
(In reply to Dani Megert from comment #41) > Verified in I20140515-1230. > > The fix is not optimal, but so far we haven't found a better one. Bug 435061 contains a significant improvement (less redraws).
The cheese in the trees in the Git Staging view is still there and doesn't seem to be affected by these changes. Opened bug 435377 for that.
OK, I think I've found the root cause of the cheese issues: Since some bug in the Windows OS there is a workaround in the Tree class and during gaining the focus the current selection on the Tree has to be redrawn - line 7131 in the Tree class (if (redraw) redrawSelection ();). It seems the 'redrawSelection' call produces the cheese in Navigator In order to verify it I've recreated the cheese issue by removing the 'Control.setRedraw' invocations from the CSSPropertyFontSWTHandler class (lines 63 and 69). Next I've skipped the 'Tree.redrawSelection' call by the following modification of the TreeViewer class (lines 124 till 126): public TreeViewer(Composite parent, int style) { this(new Tree(parent, style & ~SWT.MULTI)); } After applying the changes above the cheese is gone. I will open the proper SWT bug for it With the Git staging view will be the same story Daniel
The flicking issue mentioned in the #Comment 29 is produced by removing the Control.setRedraw calls from the CSSPropertyFontSWTHandler class. The Package explorer works fine since it uses the 'setRedraw' calls internally and some redraw operations get skipped. Hence the flicking as well as the cheese issue is fixed/hidden Daniel
(In reply to Daniel Rolka from comment #44) > After applying the changes above the cheese is gone. I will open the proper > SWT bug for it I've opened the Bug 435536 for it Daniel
*** Bug 431048 has been marked as a duplicate of this bug. ***
This issue seems to still occur in Eclipse 4.4, see bug 458015.
This is not fixed. please consider reopen. Check the comments in https://bugs.eclipse.org/bugs/show_bug.cgi?id=458015 https://bugs.eclipse.org/bugs/show_bug.cgi?id=533555 Even the editor tabs disappear.
(In reply to chidveer chinthakuntla from comment #49) > This is not fixed. > please consider reopen. > Check the comments in > https://bugs.eclipse.org/bugs/show_bug.cgi?id=458015 > https://bugs.eclipse.org/bugs/show_bug.cgi?id=533555 > > Even the editor tabs disappear. Let's track that by the above bugs.
