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

[Alexey Ivanov]

On Fri, 15 Mar 2024 14:15:12 GMT, Alexander Zvegintsev <azveg...@openjdk.org> 
wrote:

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

I expected the label to be aligned to the left. For this to happen, the label 
may need to wrapped into another panel or horizontal box with added glue.



            Box logLabelBox = Box.createHorizontalBox();
            logLabelBox.add(new JLabel("Log:"));
            logLabelBox.add(Box.createHorizontalGlue());

            Box buttonsLogPanel = Box.createVerticalBox();
            buttonsLogPanel.add(buttonsPanel);
            buttonsLogPanel.add(logLabelBox);
            buttonsLogPanel.add(new JScrollPane(logArea));


![Log Area with the "Log:" header, screenshot of 
PassFailJFrame](https://github.com/openjdk/jdk/assets/70774172/69a86152-a8a6-45ec-9a66-a457f4f5e396)

Placeholder text isn't supported by Swing natively, is it? Therefore, you'll 
have to remove the placeholder text when you add the first message, which 
complicates logging a message.

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

Okay. Most methods of the builder use `this.` prefix when assigning something 
to a field.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18319#discussion_r1526420764
PR Review Comment: https://git.openjdk.org/jdk/pull/18319#discussion_r1526429613