Community
Participate
Working Groups
Created attachment 271803 [details] A plug-in with which the problem can be reproduced. The problem is observed if a view in a view part stack defines a CTabFolder child. See attached screenshots. The view with the CTabFolder is highlighted as expected. Other views in the same part stack however will not be highlighted on click. Pressing a button (e.g. print screen) or some clicks in a not-highlighted (but active) view will highlight the view tab. Steps to reproduce: 1. Import plug-in from attachment. 2. Debug Eclipse. 3. Open "Sample View" and another view in the same part stack. 4. Click on the "Sample View", then on the other view. 5. Observe that the other view is not highlighted. Or: 1. Create plug-in with default Sample View for 4.x 2. Add a CTabFolder to the view contents. 3. Debug Eclipse. 4. Open "Sample View" and another view in the same part stack. 5. Click on the "Sample View", then on the other view. 6. Observe that the other view is not highlighted. Note that "theming" must be disabled to reproduce the problem (Window -> Preferences -> General -> Appearance -> ensure enable theming is off, restart Eclipse). Environment: RHEL 7.2, GTK 3.14 (gtk3-3.14.13-16.el7.x86_64) Eclipse SDK Version: Photon (4.8) Build id: I20171129-2000
Created attachment 271804 [details] A selected tab, with a view that defines a CTabFolder component.
Created attachment 271805 [details] Selecting another view's tab results in no highlighting.
The Console / Sample Views tabs in the bottom part stack are the interesting part of the screenshot. I forgot to mark the area in the screenshots.
During debugging I observed that the CTabFolder of the part stack will normally (without a CTabFolder in the view) receive an onActivate(), onPaint(), onDeactivate(), onPaint() sequence. I'm guessing the onActivate() and onPaint() are for the tab which was clicked, and the onDeactivate() and onPaint() come for the tab which is no longer active. With a CTabFolder in the active view, the CTabFolder of the part stack will receive only onActivate(), onDeactivate(), onPaint(). I.e. it doesn't repaint the (now) active tab.
View code: public class SampleView { private CTabFolder subtabs; @PostConstruct public void createPartControl(Composite parent) { subtabs = new CTabFolder(parent, SWT.BOTTOM | SWT.FLAT); } @Focus public void setFocus() { subtabs.setFocus(); } } Note that delegating the focus to the child CTabFolder is essential to observe the problem.
Disregard comment #4, the not-highlighted tab doesn't receive CTabFolder.onActivation. Switching from one Eclipse to the other Eclipse (due to breakpoint activation) resulted on an onActivation call.
Mickael, this is smth you worked on. Please investigate.
Hi Alexander, Mickael, the fix for Bug 474444 introduces this regression. Our application has a number of views which make use of CTabFolders. If one of those views is active, clicking on another view in the same part stack does not result in a highlight. This is very awkward: the view becomes active and works as expected, but the user is uncertain if this is the case. After some debugging, we saw that a CTabFolder will propagate all the way to the part stack tab folder, whether tabs should be highlighted. This is part of the changes introduced cor Bug 474444. Namely, in org.eclipse.swt.custom.CTabFolder: void onDeactivate(Event event) { if (!highlightEnabled) { return; } Composite current = this; while (current != null) { if (current instanceof CTabFolder) { ((CTabFolder) current).highlight = false; } current = current.getParent(); current = null; } redraw(); } void onActivate(Event event) { if (!highlightEnabled) { return; } Composite current = this; while (current != null) { if (current instanceof CTabFolder) { ((CTabFolder) current).highlight = true; } current = current.getParent(); current = null; } redraw(); } Switching to another tab deactivates CTabFolders in the current view. However the tab folder of the part stack itself does not become activated. This seems to be done in org.eclipse.swt.widgets.Shell.setActiveControl: void setActiveControl (Control control, int type) { if (control != null && control.isDisposed ()) control = null; if (lastActive != null && lastActive.isDisposed ()) lastActive = null; if (lastActive == control) return; /* * Compute the list of controls to be activated and * deactivated by finding the first common parent * control. */ Control [] activate = (control == null) ? new Control[0] : control.getPath (); Control [] deactivate = (lastActive == null) ? new Control[0] : lastActive.getPath (); lastActive = control; int index = 0, length = Math.min (activate.length, deactivate.length); while (index < length) { if (activate [index] != deactivate [index]) break; index++; } /* * It is possible (but unlikely), that application * code could have destroyed some of the widgets. If * this happens, keep processing those widgets that * are not disposed. */ for (int i=deactivate.length-1; i>=index; --i) { if (!deactivate [i].isDisposed ()) { deactivate [i].sendEvent (SWT.Deactivate); } } for (int i=activate.length-1; i>=index; --i) { if (!activate [i].isDisposed ()) { Event event = new Event (); event.detail = type; activate [i].sendEvent (SWT.Activate, event); } } } The result is as described. A nested tab will tell the part stack tabs to not show highlighting. Clicking on other part stacks and back shows highlighting as expected. This is an issue only within a single part stack. What can be done here to fix the regression? Best regards, Simeon
Right a Junit test to verify that once fixed this doesn't get broken again. A patch which fixes it would be nice too.
New Gerrit change created: https://git.eclipse.org/r/113138
It would be nice to get this into 4.7.3.
Eric, would you please review this one?
IIRC, the use-case in bug 474444 comment 25 is the one that was worth the most care when implementing/reviewing.
I also have the impression that the issue you see happens because the Deactivate events happen after the Activate events (so deactivate removes highlights set by activate). Is it something we should expect in general or could it be a deeper bug in the event creation/processing?
(In reply to Mickael Istria from comment #13) > IIRC, the use-case in bug 474444 comment 25 is the one that was worth the > most care when implementing/reviewing. Right now (SWT master) exact the opposite use case is broken. As soon as we have plugin editor opened (we have "tabs in tabs"), switching to another editor in the same editor area *disables* the editor tab highlighting. With the proposed patch everything seems to work again.
In general, the original part of the patch which traverses all CTabFolder parents and changes their ".highlight" flags is a "no go" inside SWT from my point of view. A child widget should not make any changes to the parents, this is something out of the child scope. This could easily lead to all kinds of unexpected side effects including stack overflows and endless repaints. Just put few of such "clever" children to a common parent and the results could be unpredictable. Such changes could be done in the application code, where we render something in a specific context (activate a view in a stack), but not inside the concrete widget, which should never know if it is in a stack or not.
(In reply to Alexander Kurtakov from comment #12) > Eric, would you please review this one? Will do.
New Gerrit change created: https://git.eclipse.org/r/113210
Gerrit change https://git.eclipse.org/r/113138 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=23a636dd535cf5d034a32c01f3b70238ac237533
(In reply to Eclipse Genie from comment #19) > Gerrit change https://git.eclipse.org/r/113138 was merged to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/ > ?id=23a636dd535cf5d034a32c01f3b70238ac237533 In master now, thanks for the patch!
I would like to backport the patch to 4.7.3.
(In reply to Andrey Loskutov from comment #21) > I would like to backport the patch to 4.7.3. Please prepare a patch against the maintenance branch. I'd like the current fix to sit in master for about a week just to make sure there are no unforeseen issues. Then I would be happy to merge the backport.
New Gerrit change created: https://git.eclipse.org/r/113297
(In reply to Eric Williams from comment #22) > (In reply to Andrey Loskutov from comment #21) > > I would like to backport the patch to 4.7.3. > > Please prepare a patch against the maintenance branch. Here is it: https://git.eclipse.org/r/113297 > I'd like the current fix to sit in master for about a week just to make sure > there are no unforeseen issues. Then I would be happy to merge the backport. Sure.
New Gerrit change created: https://git.eclipse.org/r/113301
Gerrit change https://git.eclipse.org/r/113210 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=fc22ad835b303358b44053dbf13b338315828ee6
(In reply to Eclipse Genie from comment #26) > Gerrit change https://git.eclipse.org/r/113210 was merged to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/ > ?id=fc22ad835b303358b44053dbf13b338315828ee6 Test cases have been merged.
Gerrit change https://git.eclipse.org/r/113301 was merged to [R4_7_maintenance]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=a8a92a1b007b7537adf6affc8ed777ae556e68ba
New Gerrit change created: https://git.eclipse.org/r/114953
Gerrit change https://git.eclipse.org/r/114953 was merged to [R4_7_maintenance]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.binaries.git/commit/?id=61c1826fd55fd11d58ecd6a902b294f37f2a36f7
Gerrit change https://git.eclipse.org/r/113297 was merged to [R4_7_maintenance]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=08ea543810619de5ab631ec2caca8ea2b5f5b7bc
(In reply to Eclipse Genie from comment #31) > Gerrit change https://git.eclipse.org/r/113297 was merged to > [R4_7_maintenance]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/ > ?id=08ea543810619de5ab631ec2caca8ea2b5f5b7bc Changes have been backported to 4.7.3.
Andrey/Eric, can you please verify the fix with the latest 4.7.3 M-build?
(In reply to Lakshmi Shanmugam from comment #33) > Andrey/Eric, can you please verify the fix with the latest 4.7.3 M-build? Ping!
(In reply to Dani Megert from comment #34) > (In reply to Lakshmi Shanmugam from comment #33) > > Andrey/Eric, can you please verify the fix with the latest 4.7.3 M-build? > > Ping! I just saw you marked it as VERIFIED. Usually we add a comment where we say with which build it was verified.
(In reply to Dani Megert from comment #35) > (In reply to Dani Megert from comment #34) > > (In reply to Lakshmi Shanmugam from comment #33) > > > Andrey/Eric, can you please verify the fix with the latest 4.7.3 M-build? > > > > Ping! > > I just saw you marked it as VERIFIED. Usually we add a comment where we say > with which build it was verified. Verified in M20180214-1700.
