On Thu, 18 Jan 2024 13:33:22 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> On Windows we need to ensure InputMethodRequests coming from JFXPanel are >> processed on the JavaFX application thread instead of the AWT EventQueue >> thread. This PR adds the runAndWait() calls to do that. >> >> This would be difficult to test on Windows without a fix for >> [JDK-8090267](https://bugs.openjdk.org/browse/JDK-8090267) so I've included >> the fix first proposed by @prsadhuk in PR #1169. If a developer uses the >> sample code provided in the JavaDoc to create and show a JFXPanel there's a >> good chance the JFXPanel will get focus before the scene has been set. To >> ensure AWT always treats the JFXPanel as an active IME client we return a >> stub version of the InputMethodRequests object if there's no scene. AWT will >> continue to ask for the InputMethodRequests and once the scene has been set >> the panel will return a non-stub version. > > modules/javafx.swing/src/main/java/javafx/embed/swing/InputMethodSupport.java > line 63: > >> 61: private Point2D pointValue; >> 62: private int intValue; >> 63: private String stringValue; > > Using instance fields like this is not thread-safe in general (although > unlikely to cause problems in practice for this specific case). I recommend > either creating a utility method that returns the result of a `Future` (see > PR #1321 ) or changing these to local `AtomicObject<Type>` / `AtomicInteger` > (or `Type[]` `int[]`) variables. Using instance fields here does look clunky. There's no thread-safety issue because the JFXPanel code creates a new adapter for every request so I decided to avoid the overhead of creating local arrays (apologies for the micro-optimizing). I will switch to local array variables if you want. For the same reason I don't think the `getInputMethodRequests` call added to line 1028 in PR #1169 is necessary. The adapter it creates is discarded so the only side-effect is calling the scene's `getInputMethodRequests` before the requests are needed. I can't think of any reason this front-loading would be necessary. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1337#discussion_r1457726480
