On Thu, 16 Oct 2025 19:59:06 GMT, Andy Goryachev <[email protected]> wrote:
> Adds Input Method Editor (IME) support to `RichTextArea`/`CodeArea`. > > Tested on macOS and Windows 11 with Japanese and Chinese (pinyin) input > methods. > Please test this on Linux, even though there is no platform-specific code in > this PR (should work the same way it does in `TextArea`/`TextField`) > > For testing, one can use the updated Monkey Tester > https://github.com/andy-goryachev-oracle/MonkeyTest > (optionally enable IME events in stdout with Logging -> IME Monitor) It works as expected on macOS (I'll try it on Windows later). I left a few comments, mostly about the line separator as it relates to the private `getText(...)` method in RichTextArea. modules/jfx.incubator.richtext/src/main/java/com/sun/jfx/incubator/scene/control/richtext/RichTextAreaHelper.java line 58: > 56: } > 57: > 58: public static boolean getText(RichTextArea t, TextPos start, TextPos > end, StringBuilder sb, int limit, String lineSeparator) { Is the lineSeparator parameter really needed here? If not, I recommend removing it. modules/jfx.incubator.richtext/src/main/java/com/sun/jfx/incubator/scene/control/richtext/TextCell.java line 264: > 262: dy = f.snappedTopInset(); > 263: > 264: p = f.rangeShape(start, end); // TODO new api, no null Is this "TODO" really related to IME? If not, I recommend removing it. modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/RichTextArea.java line 1438: > 1436: * @param sb the buffer to copy to > 1437: * @param limit the maximum number of characters to copy, must be > >= 0 > 1438: * @param lineSeparator the newline separator sequence, or null to > use the platform default Whether and how to add lineSeparator is under review via PR #1944. I recommend removing everything related to line separators for this PR, unless you want this PR to be dependent on #1944. modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/RichTextArea.java line 1442: > 1440: * @since 26 > 1441: */ > 1442: // TODO depends on JDK-8370140 (line separator property), private > for now While a `getText(...)` method would be useful, I don't see an enhancement request that proposes it. I would make it more clear that this is not public API. If you want to leave the docs in place for a future enhancement that might provide such a public API, I recommend changing `@since 26` to `@since 99` (or similar). modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/RichTextArea.java line 1469: > 1467: beg = start.offset(); > 1468: } else { > 1469: sb.append(lineSeparator); For now, should `lineSeparator` be hardcoded as `"\n"` or should it be the `line.separator` property? We often use `\n` regardless of which platform we are on unless we are writing to a file or doing something else where it matters. This is more a question for PR #1944, so whatever you do here will very likely be modified when that enhancement is implemented. Unless you want to make this PR dependent on that one, choose something now (other than passing it in as a method parameter, since that seems an unlikely choice) and add a comment that it will need to be revisited. modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/skin/RichTextAreaSkin.java line 597: > 595: > 596: // while IME is active > 597: static class Ime { I think can be private. ------------- PR Review: https://git.openjdk.org/jfx/pull/1938#pullrequestreview-3393143461 PR Review Comment: https://git.openjdk.org/jfx/pull/1938#discussion_r2472872756 PR Review Comment: https://git.openjdk.org/jfx/pull/1938#discussion_r2472883166 PR Review Comment: https://git.openjdk.org/jfx/pull/1938#discussion_r2472916772 PR Review Comment: https://git.openjdk.org/jfx/pull/1938#discussion_r2473111435 PR Review Comment: https://git.openjdk.org/jfx/pull/1938#discussion_r2473179810 PR Review Comment: https://git.openjdk.org/jfx/pull/1938#discussion_r2473056656
