JDK-8193502: Dialog height switches between correct and too small when showing and hiding one dialog repeatedly on Linux

[tsayao]

Fixes:
https://bugs.openjdk.java.net/browse/JDK-8193502

The problem:
WindowContextBase::set_view is called before WindowContextTop::window_configure and it relies on size information that is not correct yet (because it's set on WindowContextTop::window_configure).

This causes DialogPane calculations to output wrong values.

So the fix was to move the call to inform java about the size to WindowContextTop::window_configure where the size is sure to be correct.

Also gtk_window_get_size doc informs about possible conditions where it might not behave as expected.

WindowContextPlug and WindowContextChild overrides set_view, so no harm caused there.

[kevinrushforth]

@pankaj-bansal can you also review this?

[kevinrushforth]

For all of the glass changes, but especially this one, I would ask you to run a full unit test, including robot tests on your local machine(s). This can be done as follows:

gradle -PFULL_TEST=true -PUSE_ROBOT=true test -x :web:test

while running the robot tests, you need to make sure you don't interact with the system (especially don't touch the keyboard or mouse). If you don't have an up-to-date webkit library, then you will get some failures in those system tests, but don't worry about them.

[tsayao]

With the change:

Original openjfx develop branch:

[kevinrushforth]

I can confirm that this fixes the bug.

Can you provide a test case for it? An automated test in tests/system is preferred, but if that turns out to be too difficult, then a manual test in tests/manual would be an OK substitute.

[kevinrushforth]

We need to ensure that there are no cases where set_view can be called without a subsequent call to window_configure.

[kevinrushforth]

Thanks for running the tests. Some other time it would be good to know why you get so many failures, but good that your patch didn't cause any new test failures.

I can confirm that on my system I also get the same pass rate: 100% pass (except for a couple known intermittent tests, which decided to behave today).

[tsayao]

I can confirm that this fixes the bug.

Can you provide a test case for it? An automated test in tests/system is preferred, but if that turns out to be too difficult, then a manual test in tests/manual would be an OK substitute.

Test provided.

[tsayao]

About the failing tests on my setup: It was because Wayland was enable. Switching back to X.org solved it.

[kevinrushforth]

A couple comments on the test. I haven't run it yet, but will before finishing the review.

[tsayao]

Ran full robot tests, including webkit with all pending PR from me applied - #446 #450 #451 #445 (there are other event order changes in other PRs which might affect Kevin's concern about set_view being called without window_configure).

Results:

623 tests completed, 4 failed, 118 skipped

test.robot.javafx.scene.RobotTest > testKeyPress FAILED
    org.junit.ComparisonFailure: letter 'a' should be pressed by Robot expected:<[a]> but was:<[]>
        at org.junit.Assert.assertEquals(Assert.java:123)
        at test.robot.javafx.scene.RobotTest.testKeyboard(RobotTest.java:193)
        at test.robot.javafx.scene.RobotTest.testKeyPress(RobotTest.java:144)

test.robot.javafx.scene.tableview.TableViewResizeColumnToFitContentTest > resizeColumnToFitContentTest FAILED
    java.lang.AssertionError: resizeColumnToFitContent failed
        at org.junit.Assert.fail(Assert.java:91)
        at org.junit.Assert.assertTrue(Assert.java:43)
        at test.robot.javafx.scene.tableview.TableViewResizeColumnToFitContentTest.resizeColumnToFitContentTest(TableViewResizeColumnToFitContentTest.java:96)

test.robot.javafx.scene.treetableview.TreeTableViewResizeColumnToFitContentTest > resizeColumnToFitContentTest FAILED
    java.lang.AssertionError: resizeColumnToFitContent failed
        at org.junit.Assert.fail(Assert.java:91)
        at org.junit.Assert.assertTrue(Assert.java:43)
        at test.robot.javafx.scene.treetableview.TreeTableViewResizeColumnToFitContentTest.resizeColumnToFitContentTest(TreeTableViewResizeColumnToFitContentTest.java:96)

test.memoryleak.JSCallbackMemoryTest > testJsCallbackLeak FAILED
    java.lang.AssertionError: All Stages are null
        at org.junit.Assert.fail(Assert.java:91)
        at org.junit.Assert.assertTrue(Assert.java:43)
        at test.memoryleak.JSCallbackMemoryTest.testJsCallbackLeak(JSCallbackMemoryTest.java:331)

[kevinrushforth]

I ran the tests last week with all 4 patches applied as well with no failures. I've seen very occasional failures in the 4 tests you've listed, and they seem unrelated to your fixes. I'll run them again when I review your new tests.

[kevinrushforth]

You might already know this, but the CI build failed due to a white-space issue.

[pankaj-bansal]

I have run the full system test with two patches applied #456 and #446 . All tests pass successfully.

[kevinrushforth]

Both the fix and the test look good. I can confirm that on Linux, the test fails without the fix and passes with the fix.

[pankaj-bansal]

Approved

[DJViking]

Will this fix be backported to JavaFX 11? It has been speculated that this could fix the problem with #222

[kevinrushforth]

Will this fix be backported to JavaFX 11?

That's a good question for @johanvos -- it seems this might be a good candidate for backport, although there is one report of a possible regression caused by this fix when using GTK 2 as reported in #475