On Tue, 5 Nov 2024 17:37:40 GMT, Daniel Gredler <d...@openjdk.org> wrote:
>> test/jdk/java/awt/print/PrinterJob/PrintTextTest.java line 1: >> >>> 1: /* >> >> Could you make the `PrintJAText` class a nested static class inside >> `PrintTextTest`? >> >> Yes, it requires more refactoring but this ensures no duplicate class names. >> >> Since `PrintJAText` extends `PrintTextTest`, I suggest moving all the >> painting logic into `PrintText` class which will be a nested static class. >> Then `PrintJAText` (renamed to `PrintJapaneseText` if I understand the >> intent correctly) will extend `PrintText`. >> >> All the other logic will remain unchanged. >> >> Separating the painting logic from the test class itself, I believe, would >> also make thing clearer. >> >> >> In addition to the above, let's resolve IDE warnings at lines 349 and 411 >> (in the current revision): >> >> >> - byte data[] = new byte[s.length()]; >> + byte[] data = new byte[s.length()]; >> >> - TextLayout tl = new TextLayout(s, new HashMap(), frc); >> + TextLayout tl = new TextLayout(s, new HashMap<>(), frc); >> >> >> Overall, the changes look good to me. Thank you for your contribution. > > Thanks! Let me know if this last commit is what you had in mind. It might be > easier to review with whitespace ignored, since the class additions required > some indentation changes. Yes, that's what I had in mind. Thank you. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21716#discussion_r1829825695
