On Tue, 5 Apr 2022 18:58:27 GMT, Alexey Ivanov <aiva...@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
