Bugzilla – Full Text Bug Listing
|
Description
Andrey Loskutov
Forgot to mention: RHEL 7.4, GTK 3.22. New Gerrit change created: https://git.eclipse.org/r/131209 I've ran some of our failing tests with the fix for bug 533469 and https://git.eclipse.org/r/131209. I think the patch is promising. Xi, If you have no objections, I've assigned this bug to you. Gerrit change https://git.eclipse.org/r/131209 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=e4b55c0b9e272f58deca6320664a073a2af373d4 *** Bug 540643 has been marked as a duplicate of this bug. *** Created attachment 276427 [details]
Broken compare editor
With this patch compare editor is totally broken (aka empty). Reverting the patch now. New Gerrit change created: https://git.eclipse.org/r/131724 Gerrit change https://git.eclipse.org/r/131724 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=e36bed5af1ef3281401b74ba2232ae4f84143cda New Gerrit change created: https://git.eclipse.org/r/131745 New Gerrit change created: https://git.eclipse.org/r/131747 (In reply to Alexander Kurtakov from comment #8) > With this patch compare editor is totally broken (aka empty). Reverting the > patch now. Sounds like we should add Compare editor tests to SWT tests, we had recently only issues with that code :-) (In reply to Eclipse Genie from comment #11) > New Gerrit change created: https://git.eclipse.org/r/131745 I think the patch for bug 533469 has a flaw. I see in the debugger that many widgets are by default coming with (0,0) sizes in the IDE, but usually we don't see "Ghost" widgets all over the place. So I guess "hiding" widgets with 0,0 sizes on draw is probably wrong place to fix "Ghosts". By occasion I was inspecting places where we check the state for ZERO_WIDTH / ZERO_HEIGHT values, and stumbled over the old code in setInitialBounds() which "corrected" the widget size AND set the widget visibility to true. On my GTK 3.22 I see that the size is already right, and removing the call to GTK.gtk_widget_set_visible(topHandle, true); fixes the "Ghost" button issue described in bug 533469 too ... Except that this breaks Group size or layout calculation (have not investigated this further) => Group needs an extra treatment to work, see the patch. So patch https://git.eclipse.org/r/131745 fixes this issue and bug 533469 without touching the "Ghost" Group bug (which is present through all other patches as well). Here is the snippet which should show "invisible" Button, Text and Group - currently Group is still shown (but this is independently of any patches). public class Snippet1 { public static void main(String[] args) { Display display = new Display(); Shell shell = new Shell(display); Text text = new Text(shell, SWT.None); // text size is not set, it // should be 0x0 text.setText("hello"); text.setVisible(true); Button button = new Button(shell, SWT.None); button.setText("Hi"); // size is not set, it should be 0x0 Group group = new Group(shell, SWT.None); group.setText("Hi Group"); // size is not set, it should be 0x0 System.err.println(group.getSize()); shell.setSize(300, 300); shell.open(); while (!shell.isDisposed()) { if (!display.readAndDispatch()) display.sleep(); } display.dispose(); } } (In reply to Andrey Loskutov from comment #13) I think this patch https://git.eclipse.org/r/131745 is the right way to go! The group seems like an independent issue itself with Composite#setBounds. Gerrit change https://git.eclipse.org/r/131745 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=6ebcb89907ca02f035b4773c6409ad79aabc4b72 (In reply to Eclipse Genie from comment #15) > Gerrit change https://git.eclipse.org/r/131745 was merged to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/ > ?id=6ebcb89907ca02f035b4773c6409ad79aabc4b72 This causes a regression :-( Eclipse workspace prompt has no default focus now. Means: if you start Eclipse and see the workspace selection dialog, hitting "Enter" does not work anymore, because no widget is focused. (In reply to Andrey Loskutov from comment #16) > (In reply to Eclipse Genie from comment #15) > > Gerrit change https://git.eclipse.org/r/131745 was merged to [master]. > > Commit: > > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/ > > ?id=6ebcb89907ca02f035b4773c6409ad79aabc4b72 > > This causes a regression :-( > > Eclipse workspace prompt has no default focus now. > Means: if you start Eclipse and see the workspace selection dialog, hitting > "Enter" does not work anymore, because no widget is focused. Sorry, wrong bug. The problem appears via bug 540002 commit 492144bd4e220e6daeac4fcc4a3f65f4ffef6f95. (In reply to Andrey Loskutov from comment #17) > (In reply to Andrey Loskutov from comment #16) > > (In reply to Eclipse Genie from comment #15) > > > Gerrit change https://git.eclipse.org/r/131745 was merged to [master]. > > > Commit: > > > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/ > > > ?id=6ebcb89907ca02f035b4773c6409ad79aabc4b72 > > > > This causes a regression :-( > > > > Eclipse workspace prompt has no default focus now. > > Means: if you start Eclipse and see the workspace selection dialog, hitting > > "Enter" does not work anymore, because no widget is focused. > > Sorry, wrong bug. The problem appears via bug 540002 commit > 492144bd4e220e6daeac4fcc4a3f65f4ffef6f95. No, not! reverting 6ebcb89907ca02f035b4773c6409ad79aabc4b72 OR 492144bd4e220e6daeac4fcc4a3f65f4ffef6f95 fixes the issue! (In reply to Andrey Loskutov from comment #18) > (In reply to Andrey Loskutov from comment #17) > > (In reply to Andrey Loskutov from comment #16) > > > (In reply to Eclipse Genie from comment #15) > > > > Gerrit change https://git.eclipse.org/r/131745 was merged to [master]. > > > > Commit: > > > > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/ > > > > ?id=6ebcb89907ca02f035b4773c6409ad79aabc4b72 > > > > > > This causes a regression :-( > > > > > > Eclipse workspace prompt has no default focus now. > > > Means: if you start Eclipse and see the workspace selection dialog, hitting > > > "Enter" does not work anymore, because no widget is focused. > > > > Sorry, wrong bug. The problem appears via bug 540002 commit > > 492144bd4e220e6daeac4fcc4a3f65f4ffef6f95. > > No, not! reverting 6ebcb89907ca02f035b4773c6409ad79aabc4b72 OR > 492144bd4e220e6daeac4fcc4a3f65f4ffef6f95 fixes the issue! I don't know what I've tested before the lunch, after the lunch I only see that reverting commit 6ebcb89907ca02f035b4773c6409ad79aabc4b72 fixes the problem. I'm trying to debug this now. (In reply to Andrey Loskutov from comment #19) > I don't know what I've tested before the lunch, after the lunch I only see > that reverting commit 6ebcb89907ca02f035b4773c6409ad79aabc4b72 fixes the > problem. > > I'm trying to debug this now. So it looks like before the patch, there were two calls to Control.forceFocus() on a Combo, calling *both* to Control.forceFocus(long). After the patch (which does NOT implicitly makes all zero size widgets visible), the first call to the Control.forceFocus() on a Combo does NOT forward to Control.forceFocus(long) because of if (!isEnabled () || !isVisible ()) condition. Changing Shell.open() to reset the "restored" focus flag if there is no control focused, fixes this: if (restored) { Control focusControl = display.getFocusControl (); if (focusControl instanceof Button && (focusControl.style & SWT.PUSH) != 0) { restored = false; } if(focusControl == null) { restored = false; } } However, this above does not fix the problem seen with "Ctrl+L" (Go To Line) dialog, which *has* the focus set to the text, but has no *default focused* button. So there seem to be a general problem with *default focused buttons*. I'm about to reverting the change https://git.eclipse.org/r/#/c/131745/ for the next SDK build. If anyone has an idea how to fix *default focused buttons* without reverting the patch https://git.eclipse.org/r/#/c/131745/, just do it :-) Two extra test fails also shows that the patch was not OK: http://download.eclipse.org/eclipse/downloads/drops4/I20181106-0125/testresults/html/org.eclipse.ui.tests.forms_ep410I-unit-cen64-gtk3_linux.gtk.x86_64_8.0.html expected:<Rectangle {0, 0, 150, 40}> but was:<Rectangle {-1, -1, 150, 40}> java.lang.AssertionError: expected:<Rectangle {0, 0, 150, 40}> but was:<Rectangle {-1, -1, 150, 40}> at org.junit.Assert.fail(Assert.java:88) at org.junit.Assert.failNotEquals(Assert.java:834) at org.junit.Assert.assertEquals(Assert.java:118) at org.junit.Assert.assertEquals(Assert.java:144) at org.eclipse.ui.tests.forms.layout.TestColumnWrapLayout.testHorizontalMargins(TestColumnWrapLayout.java:156) expected:<Rectangle {0, 0, 20, 180}> but was:<Rectangle {-1, -1, 20, 180}> java.lang.AssertionError: expected:<Rectangle {0, 0, 20, 180}> but was:<Rectangle {-1, -1, 20, 180}> at org.junit.Assert.fail(Assert.java:88) at org.junit.Assert.failNotEquals(Assert.java:834) at org.junit.Assert.assertEquals(Assert.java:118) at org.junit.Assert.assertEquals(Assert.java:144) at org.eclipse.ui.tests.forms.layout.TestColumnWrapLayout.testVerticalMargins(TestColumnWrapLayout.java:193) I have not found yet any satisfiable explanation for the focus problem on default buttons, so I will revert https://git.eclipse.org/r/#/c/131745/ now and so we need another patch for the original NPE issue :-( New Gerrit change created: https://git.eclipse.org/r/131981 Gerrit change https://git.eclipse.org/r/131981 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=96ab2fe4d68170062b2505323e75255cbc4d4252 Back to the roots.
The original issue we had in our tests is that we had NPE caused by gtk_widget_hide() call added inside Control.gtk_draw().
A trivial attempt to fix it by checking if the widget is disposed after hide via:
if (GTK.GTK_VERSION > OS.VERSION (3, 18, 0) && (state & ZERO_WIDTH) != 0 && (state & ZERO_HEIGHT) != 0) {
if (GTK.gtk_widget_get_visible(widget)) {
GTK.gtk_widget_hide(widget);
/*
* It is possible (but unlikely), that application
* code could have disposed the widget in the hide
* event. If this happens, just return.
*/
if (isDisposed ()) return 0;
}
}
does work for most of the tests, but fails for another test trying to open content assist popup. The test fails with:
org.eclipse.swt.SWTException: Widget is disposed
at org.eclipse.swt.SWT.error(SWT.java:4595)
at org.eclipse.swt.SWT.error(SWT.java:4510)
at org.eclipse.swt.SWT.error(SWT.java:4481)
at org.eclipse.swt.widgets.Widget.error(Widget.java:522)
at org.eclipse.swt.widgets.Widget.checkWidget(Widget.java:459)
at org.eclipse.swt.widgets.Widget.addListener(Widget.java:313)
at org.eclipse.jface.fieldassist.ContentProposalAdapter$ContentProposalPopup$PopupCloserListener.installListeners(ContentProposalAdapter.java:142)
at org.eclipse.jface.fieldassist.ContentProposalAdapter$ContentProposalPopup.open(ContentProposalAdapter.java:849)
at org.eclipse.jface.fieldassist.ContentProposalAdapter.openProposalPopup(ContentProposalAdapter.java:1857)
at org.eclipse.jface.fieldassist.ContentProposalAdapter.openProposalPopup(ContentProposalAdapter.java:1876)
So I conclude, that gtk_widget_hide() call in Control.gtk_draw() is not a safe solution, even if it properly fixes bug 533469 (see snippet from comment 13). Either we find another place to hide zero-size widgets, or we need something like https://git.eclipse.org/r/#/c/131747/ (which only fixes "ghost buttons" but not "ghost texts or ghost groups").
@Eric, Xi: this gtk_widget_hide() call inside Control.gtk_draw() breaks few our automated tests and who knows what else due the hide/focus/dispose listener which might be called as a result of that.
I would really like to get rid of *this* bug (so that we can continue with tests on 4.10 platform) - either we resurrect patch https://git.eclipse.org/r/#/c/131747/ (which works for our related tests but does not fully fixes bug 533469) or we revert the original patch for bug 533469.
In both cases it means we should re-open bug 533469.
WDYT?
(In reply to Andrey Loskutov from comment #24) > In both cases it means we should re-open bug 533469. Or someone fixes "no default focused button" issue I've introduced by the (reverted) patch https://git.eclipse.org/r/#/c/131745/. (In reply to Andrey Loskutov from comment #24) > So I conclude, that gtk_widget_hide() call in Control.gtk_draw() is not a > safe solution, even if it properly fixes bug 533469 (see snippet from > comment 13). Either we find another place to hide zero-size widgets, or we > need something like https://git.eclipse.org/r/#/c/131747/ (which only fixes > "ghost buttons" but not "ghost texts or ghost groups"). > > @Eric, Xi: this gtk_widget_hide() call inside Control.gtk_draw() breaks few > our automated tests and who knows what else due the hide/focus/dispose > listener which might be called as a result of that. > > I would really like to get rid of *this* bug (so that we can continue with > tests on 4.10 platform) - either we resurrect patch > https://git.eclipse.org/r/#/c/131747/ (which works for our related tests but > does not fully fixes bug 533469) or we revert the original patch for bug > 533469. > > In both cases it means we should re-open bug 533469. > > WDYT? That linked gerrit change is not a good approach IMO, as messing with the opacity of widgets can lead to nasty regressions which are hard to debug. However we should resolve this, so let's reopen bug 533469 and do a proper fix. Maybe we can limit the fix to buttons only instead of all Controls? Might not be the best solution but it's a start -- Xi, please investigate. New Gerrit change created: https://git.eclipse.org/r/131996 (In reply to Eclipse Genie from comment #27) > New Gerrit change created: https://git.eclipse.org/r/131996 This reverts the original fix for bug 533469 (but keeps the example snippet). Gerrit change https://git.eclipse.org/r/131996 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=0dcb0602f8deb9fbd8e99f8952ca266c99e091b9 I've reverted change from bug 533469 and reopened original bug 533469. |