RFR: 8328242: Add a log area to the PassFailJFrame [v2]

[Alexander Zvegintsev]

On Fri, 15 Mar 2024 13:16:39 GMT, Alexey Ivanov <aivanov at openjdk.org> wrote:

>> Alexander Zvegintsev has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Add logArea placeholder text
>>  - review comments
>
> 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();

Thanks, this is much better.

> 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?

This doesn't look good to me
![image](https://github.com/openjdk/jdk/assets/77687766/331e19e6-e1c6-4fa0-ba65-e925c29ca01d)

So how about a placeholder text? It will not take up any of the height of the window.

![image](https://github.com/openjdk/jdk/assets/77687766/e0390333-4974-4799-adb3-8b8f304b664c)

> 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.

Good point, updated.

> 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.

I keep it for consistency across builder methods, e.g. `screenCapture` does not need to specify it either.

> 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.

It does the job well for now, so we can get to that later when the actual automatic instruction size is implemented.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18319#discussion_r1526353192
PR Review Comment: https://git.openjdk.org/jdk/pull/18319#discussion_r1526356831
PR Review Comment: https://git.openjdk.org/jdk/pull/18319#discussion_r1526357356
PR Review Comment: https://git.openjdk.org/jdk/pull/18319#discussion_r1526352609
PR Review Comment: https://git.openjdk.org/jdk/pull/18319#discussion_r1526360058