RFR: 8328242: Add a log area to the PassFailJFrame

[Alexey Ivanov]

On Fri, 15 Mar 2024 04:40:14 GMT, Alexander Zvegintsev <azvegint at openjdk.org> wrote:

> Often manual tests have a text area in the instruction window to print feedback from the test to be evaluated by a tester.
> I stumbled across another test that needed this, so I decided to make these changes a standard feature.
> 
> List of changes:
> * The log text area can be added below the `Pass` and `Fail` buttons panel.
> To do this, use the builder methods `logArea()`, `logArea(int rows)`.
> * `PassFailJFrame.log(message)`, `PassFailJFrame.logClear()` and `PassFailJFrame.logSet()` are added to control this text area. (maybe I missed something?)
> * added `invokeOnEDTUncheckedException` for easier use of logging methods
> 
> So typical usage would be like:
> 
>         PassFailJFrame
>                 .builder()
>                 .title("GetBoundsResizeTest Instructions")
>                 .instructions(INSTRUCTIONS)
>                 .rows(20)
>                 .columns(70)
>                 .logArea()
>                 .build()
>                 .awaitAndCheck();
> 
> 
> ----
> 
> //somewhere in event handlers:
> PassFailJFrame.log("Very important message");

Changes requested by aivanov (Reviewer).

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 486:

> 484:             JPanel buttonsLogPanel = new JPanel();
> 485:             BoxLayout layout = new BoxLayout(buttonsLogPanel, BoxLayout.Y_AXIS);
> 486:             buttonsLogPanel.setLayout(layout);

You can use Box:
Suggestion:

            Box buttonsLogPanel = Box.createVerticalBox();

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 489:

> 487: 
> 488:             buttonsLogPanel.add(buttonsPanel);
> 489:             buttonsLogPanel.add(new JScrollPane(logArea));

Suggestion:

            buttonsLogPanel.add(new JLabel("Log:"));
            buttonsLogPanel.add(new JScrollPane(logArea));

Add a header to explain the purpose of the component below?

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 1088:

> 1086:     public static void log(String message) {
> 1087:         invokeOnEDTUncheckedException(() -> {
> 1088:             if (logArea != null) {

Should it throw `NullPointerException`? It's a developer error. If logging is used, it should be enabled; therefore throwing an exception rather than hiding the problem is reasonable, isn't it? Otherwise, the problem could remain unnoticed.

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 1090:

> 1088:             if (logArea != null) {
> 1089:                 logArea.append(message + System.lineSeparator());
> 1090:             }

Suggestion:

            if (logArea != null) {
                logArea.append(message + "\n");
            }

Text components in Swing use `\n` as line separator.

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 1109:

> 1107:      * Replaces the log area content with provided {@code text}, if enabled by
> 1108:      * {@link Builder#logArea()} or {@link Builder#logArea(int)}.
> 1109:      * @param text replacement

Suggestion:

     * @param text new text for log area

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 1179:

> 1177:         /**
> 1178:          * Adds a log area below the "Pass", "Fail" buttons.
> 1179:          * <p> The log area can be controlled by {@link #log(String)},

Suggestion:

         * <p>
         * The log area can be controlled by {@link #log(String)},

Starting on the next line makes it easier to parse when reading the source code.

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 1185:

> 1183:          */
> 1184:         public Builder logArea() {
> 1185:             this.addLogArea = true;

Suggestion:

            addLogArea = true;

I think the usage of `this.` is redundant when there's no naming conflict.

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 1195:

> 1193:          *
> 1194:          * <p> The number of columns is taken from the number of
> 1195:          * columns in the instructional JTextArea.

This may be not what we want…

I'd like to get rid of specifying rows and columns for instructions, see [JDK-8328163](https://bugs.openjdk.org/browse/JDK-8328163). I can't remember why we decided to specify those explicitly.

On the other hand, if adjustment is required, `.columns` will do the job for both cases, so adding another parameter is likely an overkill.

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

PR Review: https://git.openjdk.org/jdk/pull/18319#pullrequestreview-1938932389
PR Review Comment: https://git.openjdk.org/jdk/pull/18319#discussion_r1526274310
PR Review Comment: https://git.openjdk.org/jdk/pull/18319#discussion_r1526277177
PR Review Comment: https://git.openjdk.org/jdk/pull/18319#discussion_r1526298831
PR Review Comment: https://git.openjdk.org/jdk/pull/18319#discussion_r1526280119
PR Review Comment: https://git.openjdk.org/jdk/pull/18319#discussion_r1526281419
PR Review Comment: https://git.openjdk.org/jdk/pull/18319#discussion_r1526284864
PR Review Comment: https://git.openjdk.org/jdk/pull/18319#discussion_r1526284131
PR Review Comment: https://git.openjdk.org/jdk/pull/18319#discussion_r1526293989