On Tue, 7 Mar 2023 22:03:12 GMT, Phil Race <[email protected]> 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
