RFR: 8302797: ArrayIndexOutOfBoundsException in TextRun.getWrapIndex() [v2]

[Kevin Rushforth]

On Tue, 7 Mar 2023 22:03:12 GMT, Phil Race <prr at 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