Community
Participate
Working Groups
Created attachment 251076 [details] Sample Run the attached snippet and notice that adding an item to CTabFolder forces a relayout on ALL siblings of the CTabFolder which makes no sense at all. The offending code for this is found in "org.eclipse.swt.custom.CTabFolder.runUpdate" which does a getParent().layout(true,true)!
This causes a whole lot of layout() calls when e.g. switching perspectives in eclipse and what's really bad is that layout(true,true) unconditionally layouts all and everything even not currently active tabs. A bug report to talk about layouting stuff in CTabFolder currently not visible will follow shortly
See #460745 why this call makes this call even worse than it could be
i would at least have expected some feedback from SWT-Team - the later you look at it the less likely we can ship a fix for Mars
Niraj, can you please investigate this?
Below fix made for GTK3, is forcing a relayout on Parent and all siblings: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=0757bcaef9d164ab26c143786c6ed075b2af44ce Will investigate for a better fix @ GTK3, to avoid this re-layouting.
(In reply to Niraj Modi from comment #5) > Below fix made for GTK3, is forcing a relayout on Parent and all siblings: > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/ > ?id=0757bcaef9d164ab26c143786c6ed075b2af44ce Note: Above fix was made as part of GTK3 bug 421127(https://bugs.eclipse.org/bugs/show_bug.cgi?id=421127#c55)
At least one could constraint the layout only when running on Gtk3, so at least OS-X and win32 are not affected
(In reply to Thomas Schindl from comment #7) > At least one could constraint the layout only when running on Gtk3, so at > least OS-X and win32 are not affected As Linux user, I would appreciate a fix for all platforms.
right - this is just in case no Gtk3 fix is on available!
(In reply to Lars Vogel from comment #8) > As Linux user, I would appreciate a fix for all platforms. The original patch was a fix for *GTK3 only* bug, but the original patch introduced performance regression for ALL platforms. (In reply to Niraj Modi from comment #5) > Below fix made for GTK3, is forcing a relayout on Parent and all siblings: > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/ > ?id=0757bcaef9d164ab26c143786c6ed075b2af44ce > > Will investigate for a better fix @ GTK3, to avoid this re-layouting. Are we talking about the fix for bug 421127 here? In this case I would suggest to reopen bug 421127, and until the proper fix for bug 421127 is not available, fix the performance regression on ALL platforms but GTK3 by restricting the previous patch to *GTK3 only*.
New Gerrit change created: https://git.eclipse.org/r/44413
(In reply to Eclipse Genie from comment #11) > New Gerrit change created: https://git.eclipse.org/r/44413 I've tested on Fedora 21 (rpm -q gtk3 => gtk3-3.14.9-1.fc21.x86_64) and the original fix for bug 421127 comment 55 (commit 0757bcaef9d164ab26c143786c6ed075b2af44ce) is not needed to fix rendering issues in CustomControlExample. getParent().layout(true, *false*) is enough to fix "empty" tabs in CTabFolder.
(In reply to Andrey Loskutov from comment #12) > (In reply to Eclipse Genie from comment #11) > > New Gerrit change created: https://git.eclipse.org/r/44413 > > I've tested on Fedora 21 (rpm -q gtk3 => gtk3-3.14.9-1.fc21.x86_64) and the > original fix for bug 421127 comment 55 (commit > 0757bcaef9d164ab26c143786c6ed075b2af44ce) is not needed to fix rendering > issues in CustomControlExample. > > getParent().layout(true, *false*) is enough to fix "empty" tabs in > CTabFolder. Tested myself, no regression found with your patch. Will push shortly.
Gerrit change https://git.eclipse.org/r/44413 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=dbd94fe4f7cdd9c8a0298948880a920a3e1ad9ca
In master now.
Would it be possible to measure the performance improvement with this patch? My performance measurement skills are very low, but I would be interested in learning how to do it and also to see the results for this enhancement.
In Build id: N20150326-2000 switching CSS styling does not work anymore, e.g., it does not update the CSS in the whole application. Can this be related to this change?
(In reply to Lars Vogel from comment #17) > In Build id: N20150326-2000 switching CSS styling does not work anymore, > e.g., it does not update the CSS in the whole application. Can this be > related to this change? I can't reproduce this, neither on GTK2 nor on GTK3, or I miss right steps to reproduce. The original patch [1] which added getParent().layout(true, true); had nothing to do with CSS styling too. [1] https://git.eclipse.org/r/#/c/25291/6/bundles/org.eclipse.swt/Eclipse+SWT+Custom+Widgets/common/org/eclipse/swt/custom/CTabFolder.java
(In reply to Lars Vogel from comment #17) > In Build id: N20150326-2000 switching CSS styling does not work anymore, > e.g., it does not update the CSS in the whole application. Can this be > related to this change? Lars, can you reproduce it? If not, please close this bug again, I don't think this change is related.
(In reply to Andrey Loskutov from comment #12) > (In reply to Eclipse Genie from comment #11) > > New Gerrit change created: https://git.eclipse.org/r/44413 > > I've tested on Fedora 21 (rpm -q gtk3 => gtk3-3.14.9-1.fc21.x86_64) and the > original fix for bug 421127 comment 55 (commit > 0757bcaef9d164ab26c143786c6ed075b2af44ce) is not needed to fix rendering > issues in CustomControlExample. > > getParent().layout(true, *false*) is enough to fix "empty" tabs in > CTabFolder. I think we should limit this fix only to GTK for 2 reasons: - The code is unnecessary for Windows & Mac. - It sends unexpected SWT.Resize events on its parent and siblings (for example when adding a new tab) IMO, the real fix should be made in GTK code and this should be treated as a temporary fix/workaround.
(In reply to Andrey Loskutov from comment #19) > (In reply to Lars Vogel from comment #17) > > In Build id: N20150326-2000 switching CSS styling does not work anymore, > > e.g., it does not update the CSS in the whole application. Can this be > > related to this change? > > Lars, can you reproduce it? If not, please close this bug again, I don't > think this change is related. I plan to test the reversion of change this week, sorry for the delay I have some pressing issues to work on and using a custom modified SWT is new to me.
(In reply to Lakshmi Shanmugam from comment #20) > I think we should limit this fix only to GTK for 2 reasons: > - The code is unnecessary for Windows & Mac. > - It sends unexpected SWT.Resize events on its parent and siblings (for > example when adding a new tab) > > IMO, the real fix should be made in GTK code and this should be treated as a > temporary fix/workaround. Restricted the fix and effect to GTK only via below git patch: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=ef9b2ace84f9bf3603ba33e4a4c8fc9ae7227032
(In reply to Niraj Modi from comment #22) > (In reply to Lakshmi Shanmugam from comment #20) > > I think we should limit this fix only to GTK for 2 reasons: > > - The code is unnecessary for Windows & Mac. > > - It sends unexpected SWT.Resize events on its parent and siblings (for > > example when adding a new tab) > > > > IMO, the real fix should be made in GTK code and this should be treated as a > > temporary fix/workaround. > > Restricted the fix and effect to GTK only via below git patch: > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/ > ?id=ef9b2ace84f9bf3603ba33e4a4c8fc9ae7227032 Thanks. I think this is only needed for GTK3. Does it make sense (and is it possible) to restrict the patch to this platform only? If not, should we close this bug?
(In reply to Lars Vogel from comment #17) > In Build id: N20150326-2000 switching CSS styling does not work anymore, > e.g., it does not update the CSS in the whole application. Can this be > related to this change? Looks like the fix for Bug 372517 causes the issues during theme switching. Sorry for the noise.
(In reply to Andrey Loskutov from comment #23) > I think this is only needed for GTK3. Does it make sense (and is it > possible) to restrict the patch to this platform only? If not, should we > close this bug? Since implementation of CTabFolder Control is common for all platforms, so we should-not(may be cannot) have GTK3 specific check in this code. However, the 2nd concerns raised in comment 20 (now GTK only) is still open and should be addressed with a proper fix for GTK3. Considering current fix as temporary, hence reopening this bug to continue discussion on this.
I'm not the SWT guru, so I fear I'm now not the right assignee for SWT GTK changes required according to comment 20 and comment 25.
(In reply to Niraj Modi from comment #25) > (In reply to Andrey Loskutov from comment #23) > > I think this is only needed for GTK3. Does it make sense (and is it > > possible) to restrict the patch to this platform only? If not, should we > > close this bug? > Since implementation of CTabFolder Control is common for all platforms, so > we should-not(may be cannot) have GTK3 specific check in this code. > > However, the 2nd concerns raised in comment 20 (now GTK only) is still open > and should be addressed with a proper fix for GTK3. > Considering current fix as temporary, hence reopening this bug to continue > discussion on this. Since the actual performance issue addressed by this particular bug has been fixed, I'll go ahead and resolve this one. I'll clone and create a new bug to investigate if a GTK+ specific fix for the CTabFolder rendering issues can be found.
Verified in debugger in I20150428-0800 on GTK
