On Fri, 15 Mar 2024 04:40:14 GMT, Alexander Zvegintsev <azveg...@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
