On Tue, 9 Jan 2024 21:45:33 GMT, Marius Hanl <[email protected]> wrote:
> This PR fixes the dialog freeze problem once and for all.
>
> This one is a bit tricky to understand, here is how it works:
> This bug happens on every platform, although the implementation of nested
> event loops differs on every platform.
> E.g. on Linux we use `gtk_main` and `gtk_main_quit`, on Windows and Mac we
> have an own implementation of a nested event loop (while loop), controlled by
> a boolean flag.
>
> Funny enough, the reason why this bug happens is always the same: Timing.
>
> 1. When we hide a dialog, `_leaveNestedEventLoop` is called.
> 2. This will call native code to get out of the nested event loop, e.g. on
> Windows we try to break out of the while loop with a boolean flag, on Linux
> we call `gtk_main_quit`.
> 3. Now, if we immediately open a new dialog, we enter a new nested event loop
> via `_enterNestedEventLoop`, as a consequence we do not break out of the
> while loop on Windows (the flag is set back again, the while loop is still
> running), and we do not return from `gtk_main` on Linux.
> 4. And this will result in the Java code never returning and calling
> `notifyLeftNestedEventLoop`, which we need to recover the UI.
>
> So it is actually not trivial to fix this problem, and we can not really do
> anything on the Java side. We may can try to wait until one more frame has
> run so that things will hopefully be right, but that sounds rather hacky.
>
> I therefore analyzed, if we even need to return from `_enterNestedEventLoop`.
> Turns out, we don't need to.
> There is a return value which we actually do not use (it does not have any
> meaning to us, other that it is used inside an assert statement).
> Without the need of a return value, we also do not need to care when
> `_enterNestedEventLoop` is returning - instead we cleanup and call
> `notifyLeftNestedEventLoop` in `_leaveNestedEventLoop`, after the native code
> was called.
>
> Lets see if this is the right approach (for all platforms).
> Testing appreciated.
> #
> - [x] Tested on Windows
> - [x] Tested on Linux
> - [x] Tested on Mac
> - [ ] Tested on iOS (although the nested event loop code is the same as for
> Mac) (I would appreciate if someone can do this as I have no access to an iOS
> device)
> - [ ] Adjust copyright
> - [ ] Write Systemtest
As I was afraid of, this change causes many test failures (see below for just
one example). I think a different approach is needed, one that preserves the
synchronous nature of these methods.
NestedEventLoopTest > testCanEnterAndExitNestedEventLoop FAILED
java.lang.AssertionError
at org.junit.Assert.fail(Assert.java:87)
at org.junit.Assert.assertTrue(Assert.java:42)
at org.junit.Assert.assertFalse(Assert.java:65)
at org.junit.Assert.assertFalse(Assert.java:75)
at
test.javafx.stage.NestedEventLoopTest.lambda$testCanEnterAndExitNestedEventLoop$2(NestedEventLoopTest.java:120)
at test.util.Util.lambda$submit$0(Util.java:108)
at
javafx.graphics@23-ea/com.sun.javafx.application.PlatformImpl.lambda$runLater$10(PlatformImpl.java:456)
at
java.base/java.security.AccessController.doPrivileged(AccessController.java:400)
at
javafx.graphics@23-ea/com.sun.javafx.application.PlatformImpl.lambda$runLater$11(PlatformImpl.java:455)
at
javafx.graphics@23-ea/com.sun.glass.ui.InvokeLaterDispatcher$Future.run(InvokeLaterDispatcher.java:95)
at
javafx.graphics@23-ea/com.sun.glass.ui.mac.MacApplication._enterNestedEventLoopImpl(Native
Method)
at
javafx.graphics@23-ea/com.sun.glass.ui.mac.MacApplication._enterNestedEventLoop(MacApplication.java:169)
at
javafx.graphics@23-ea/com.sun.glass.ui.Application.enterNestedEventLoop(Application.java:512)
at
javafx.graphics@23-ea/com.sun.glass.ui.EventLoop.enter(EventLoop.java:107)
at
javafx.graphics@23-ea/com.sun.javafx.tk.quantum.QuantumToolkit.enterNestedEventLoop(QuantumToolkit.java:656)
at
javafx.graphics@23-ea/javafx.application.Platform.enterNestedEventLoop(Platform.java:310)
at
test.javafx.stage.NestedEventLoopTest.lambda$testCanEnterAndExitNestedEventLoop$0(NestedEventLoopTest.java:112)
@Maran23 I see from the description that you tested this on mac. How did you
test it? Did you run with `-PFULL_TEST=true`?
-------------
Changes requested by kcr (Lead).
PR Review: https://git.openjdk.org/jfx/pull/1324#pullrequestreview-1896186878
PR Comment: https://git.openjdk.org/jfx/pull/1324#issuecomment-1959717323
