RFR: 8280913: Create a regression test for JRootPane.setDefaultButton() method [v4]

[Manukumar V S]

On Thu, 3 Feb 2022 14:27:57 GMT, Alexey Ivanov <aivanov at openjdk.org> wrote:

>> Manukumar V S has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   8280913: Create a regression test for JRootPane.setDefaultButton() method
>
> test/jdk/javax/swing/JRootPane/DefaultButtonTest.java line 39:
> 
>> 37:  * @bug 8280913
>> 38:  * @summary Check whether the default button is honored when <Enter> key is
>> 39:  * pressed while the focus is on the frame.
> 
> …“while the focus is on the frame” is not quite correct. The default button should get pressed when focus is on whatever component that doesn't consume Enter. I suggest dropping this part from the description.

Changed

> test/jdk/javax/swing/JRootPane/DefaultButtonTest.java line 44:
> 
>> 42: public class DefaultButtonTest {
>> 43:     volatile boolean buttonPressed = false;
>> 44:     JFrame frame;
> 
> I'd add `private` to the declaration.
> 
> Assigning `false` is redundant. It's the default value and you explicitly assign `false` to it in each L&F iteration.

changed

> test/jdk/javax/swing/JRootPane/DefaultButtonTest.java line 52:
> 
>> 50:         } finally {
>> 51:             test.disposeFrame();
>> 52:         }
> 
> This partly duplicates the code in `runTest`.
> I'd suggest putting try-finally inside the for-loop in `runTest`.

Done

> test/jdk/javax/swing/JRootPane/DefaultButtonTest.java line 77:
> 
>> 75:         frame = new JFrame();
>> 76:         JPanel panel = new JPanel();
>> 77:         panel.setLayout(new FlowLayout());
> 
> `FlowLayout` is the default layout manager for `JPanel`. If you like to set the layout manager explicitly, you can pass it to `JPanel` constructor.

Removed it.

> test/jdk/javax/swing/JRootPane/DefaultButtonTest.java line 78:
> 
>> 76:         JPanel panel = new JPanel();
>> 77:         panel.setLayout(new FlowLayout());
>> 78:         JButton button1 = new JButton("button1");
> 
> May I suggest adding:
> 
> panel.add(new JTextField("Text field"));
> 
> The text field will get the focus on the frame. Pressing Enter should still activate the default button. I think it's a better test compared to explicitly requesting the focus to the root pane because it's more natural.
> 
> If you do this, you can drop the line:
> 
> frame.getRootPane().requestFocus();

Done

> test/jdk/javax/swing/JRootPane/DefaultButtonTest.java line 89:
> 
>> 87:         frame.add(panel);
>> 88: 
>> 89:         frame.setSize(200, 200);
> 
> Can be replaced with
> 
> frame.pack();

Done

> test/jdk/javax/swing/JRootPane/DefaultButtonTest.java line 96:
> 
>> 94: 
>> 95:     public void runTest() throws Exception {
>> 96:         Robot robot = new Robot();
> 
> Add `robot.setAutoDelay(100);` then you can drop `robot.delay(100);` between key press and release.

Done

> test/jdk/javax/swing/JRootPane/DefaultButtonTest.java line 117:
> 
>> 115:                 throw new RuntimeException("Test Failed, button not pressed for L&F: " + lafName);
>> 116:             }
>> 117:             disposeFrame();
> 
> The frame must be disposed of on EDT. So either
> 
> SwingUtilities.invokeAndWait(this::disposeFrame);
> 
> or wrap the body of `disposeFrame` method into `SwingUtilities.invokeAndWait`.

Done

> test/jdk/javax/swing/JRootPane/DefaultButtonTest.java line 121:
> 
>> 119:     }
>> 120: 
>> 121: }
> 
> Please add line break at the end of the file.

Done

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

PR: https://git.openjdk.java.net/jdk/pull/7278