On Tue, 7 Mar 2023 22:03:12 GMT, Phil Race <p...@openjdk.org> wrote: >> This fixes an the AIOOBE when finding a line break point in RTL laid out >> glyphs. >> The comment in the bug report explains how we can end up trying to find an >> unachievable break point and yet there's no "stop" on the search when we've >> run out of glyphs so hence the exception. >> >> The fix uses a different method to choose a break point. >> >> A system test has been supplied which will fail on macOS (even with standard >> macOS fonts, not just the Noto Sans Arabic) unless the fix is applied. > > Phil Race has updated the pull request incrementally with one additional > commit since the last revision: > > 8302797
The fix looks fine. I left a few comments on the test. tests/system/src/test/java/test/javafx/scene/text/ArabicWrappingTest.java line 46: > 44: import java.util.TimerTask; > 45: > 46: public class ArabicWrappingTest extends Application { The main test class should not extend Application. See [JDK-8234876](https://bugs.openjdk.org/browse/JDK-8234876). Instead, create an static nested class that extends Application. tests/system/src/test/java/test/javafx/scene/text/ArabicWrappingTest.java line 83: > 81: System.setErr(systemErrFilter); > 82: new Thread(() -> { > 83: Application.launch(ArabicWrappingTest.class); I recommend using the recently-added [Util.launch](https://github.com/openjdk/jfx/blob/ba0f28dc072605c1ccd30e2736d39b1fcb11c4cd/tests/system/src/test/java/test/util/Util.java#L319) utility method. tests/system/src/test/java/test/javafx/scene/text/ArabicWrappingTest.java line 89: > 87: @AfterClass > 88: public static void exitTest() { > 89: Platform.exit(); I recommend using the recently-added [Util.shutdown](https://github.com/openjdk/jfx/blob/ba0f28dc072605c1ccd30e2736d39b1fcb11c4cd/tests/system/src/test/java/test/util/Util.java#L380) utility method. tests/system/src/test/java/test/javafx/scene/text/ArabicWrappingTest.java line 95: > 93: static volatile boolean testPassed; > 94: > 95: @Test Can you add a timeout value? That way if something goes wrong the test won't hang indefinitely. Something like: @Test (timeout=60000) tests/system/src/test/java/test/javafx/scene/text/ArabicWrappingTest.java line 150: > 148: stage.setTitle("Test bidi text wrapping"); > 149: stage.setWidth(MAX_WW+50); > 150: stage.setHeight(600); In connection with `Util.launch(startupLatch, ...)`, here is where you can add: stage.setOnShown(e -> Platform.runLater(startupLatch::countDown)); ------------- PR: https://git.openjdk.org/jfx/pull/1055