RFR: 8284535 : Fix PrintLatinCJKTest.java test that is failing with Parse Exception [v4]

[lawrence.andrews]

On Tue, 5 Apr 2022 18:58:27 GMT, Alexey Ivanov <aivanov at openjdk.org> wrote:

>> lawrence.andrews has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Dispose the Frame based in EDT
>
> test/jdk/java/awt/print/PrinterJob/PrintLatinCJKTest.java line 91:
> 
>> 89:     public static void main(String[] args) throws InterruptedException, InvocationTargetException {
>> 90:         PassFailJFrame passFailJFrame = new PassFailJFrame("Test Instruction" +
>> 91:                 "Frame", info, 7, 30, 3);
> 
> This already breaks the threading rules of Swing: every access to Swing components must be done on EDT.
> 
> The thing is `PassFailJFrame` extends `JFrame`, thus super constructor (the inherited one from `JFrame`) will be called on main thread. It's not what we want, isn't it?
> 
> I'd suggest not to extend `JFrame` at all. We're not developing new functionality to `JFrame` but build UI using Swing components.
> 
> As a benefit, you'll be able to initialise GUI on EDT if the constructor is called on another thread.
> 
> I support Phil that the framework should document threading and mark methods which are allowed to be called on any thread. Probably all the methods should allow for it.

Fixed, while the comment was added

> test/jdk/java/awt/print/PrinterJob/PrintLatinCJKTest.java line 93:
> 
>> 91:                 "Frame", info, 7, 30, 3);
>> 92:         PrintLatinCJKTest.showFrame();
>> 93:         passFailJFrame.awaitAndCheck();
> 
> I believe `awaitAndCheck()` is the method that must be called from the main thread. It waits until the test is completed.
> 
> If it was called on EDT, it would block the event queue.

Added the Note as part of the method description

> test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 42:
> 
>> 40:     private final CountDownLatch latch = new CountDownLatch(1);
>> 41:     private boolean failed = false;
>> 42:     private String testFailedReason;
> 
> These two are accessed by EDT and main thread. The boolean field should be declared `volatile` or use `AtomicBoolean`. The same applies for `testFailedReason`.

Fixed

> test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 45:
> 
>> 43:     private JTextArea instructionsText;
>> 44:     private final int maxRowLength;
>> 45:     private final int maxStringLength;
> 
> These aren't needed as fields, they're used only to create instruction text.

Removed

> test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 65:
> 
>> 63:      * @throws HeadlessException
>> 64:      * @throws InterruptedException
>> 65:      * @throws InvocationTargetException
> 
> I suggest adding generic descriptions of exceptions to avoid IDE warnings.

Added the description

> test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 78:
> 
>> 76:         invokeAndWait(() -> {
>> 77:             setLayout(new BorderLayout());
>> 78:             instructionsText = new JTextArea(instructions, maxRowLength, maxStringLength);
> 
> You could use line wrap functionality of JTextArea to avoid adding `\n` to the instructions.
> 
> Consider placing it into JScrollPane for flexibility, though not required.
> 
> `maxRowLength` is actually the number of rows (lines), and `maxStringLength` is the number of columns. These parameters are used to calculate the preferred size of the text area, they don't limit the text the component can store.

Added JScrollPane & used wrap function on JTextArea

> test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 80:
> 
>> 78:             instructionsText = new JTextArea(instructions, maxRowLength, maxStringLength);
>> 79:             instructionsText.setEditable(false);
>> 80:             instructionsText.setFocusable(false);
> 
> I think leaving the instructions focusable won't hurt. It would allow selecting text (and copying it) if the tester needs it for whatever reason.

Done

> test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 95:
> 
>> 93:             add(buttonsPanel, BorderLayout.SOUTH);
>> 94:             pack();
>> 95:             setLocation(10, 10);
> 
> The instruction window should probably be placed around the centre of the screen.
> 
> The class should provide a method to position a secondary frame near the instruction frame so that both frames don't overlap.
> 
> A possibility to include test UI to the instructions could be beneficial in some cases: dealing with one window is easier than with several ones. It's just a suggestion for further improvement though.

Fixed

> test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 96:
> 
>> 94:             pack();
>> 95:             setLocation(10, 10);
>> 96:             setVisible(true);
> 
> Closing the window with Close button should also fail the test, now it doesn't.

Fixed

> test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 125:
> 
>> 123:     public void getFailureReason() {
>> 124:         final JDialog dialog = new JDialog();
>> 125:         dialog.setTitle("Failure reason");
> 
> I suggest making the dialog Toolkit-modal so that only the dialog can be interacted with.
> 
> 
>         final JDialog dialog = new JDialog(this, "Failure reason", TOOLKIT_MODAL);
> 
> 
> At this time, I can click Fail again several times and I'll have several dialogs open. I can also click Pass which completes the test and disposes of the dialogs.

Yes, I made the JDialog to be Modal dialog

> test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 144:
> 
>> 142:         jPanel.add(okayBtnPanel, BorderLayout.SOUTH);
>> 143:         dialog.add(jPanel);
>> 144:         dialog.setLocationRelativeTo(null);
> 
> We should probably position the dialog relatively to the frame or the Fail button.
> 
> Now the frame is displayed on the left edge of the screen but the dialog is in the centre of the screen as if they're unrelated.

Fixed.

-------------

PR: https://git.openjdk.java.net/jdk/pull/7966