RFR: 8283664: Remove jtreg tag manual=yesno for java/awt/print/PrinterJob/PrintTextTest.java [v2]
Alexey Ivanov
aivanov at openjdk.org
Tue Nov 5 18:30:38 UTC 2024
On Tue, 5 Nov 2024 17:37:40 GMT, Daniel Gredler <duke at 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
More information about the client-libs-dev
mailing list