RFR: 8299522: Incorrect size of Approve button in custom JFileChooser [v5]

[Tejesh R]

On Wed, 11 Jan 2023 19:44:32 GMT, Alexey Ivanov <aivanov at openjdk.org> wrote:

>> Tejesh R has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Spacing updation
>
> src/java.desktop/share/classes/javax/swing/plaf/basic/BasicFileChooserUI.java line 586:
> 
>> 584:         }
>> 585: 
>> 586:         if (fc.getDialogType() == JFileChooser.OPEN_DIALOG ||
> 
> Since you added spaces after `if`, you should probably update the `if` above too.

Updated.

> src/java.desktop/share/classes/javax/swing/plaf/basic/BasicFileChooserUI.java line 587:
> 
>> 585: 
>> 586:         if (fc.getDialogType() == JFileChooser.OPEN_DIALOG ||
>> 587:                 fc.getDialogType() == JFileChooser.CUSTOM_DIALOG) {
> 
> Suggestion:
> 
>         if (fc.getDialogType() == JFileChooser.OPEN_DIALOG
>                 || fc.getDialogType() == JFileChooser.CUSTOM_DIALOG) {
> 
> I prefer it this way: wrapping before the operator makes it clear that it's a continuation line.

Updated.

> test/jdk/javax/swing/JFileChooser/CustomApproveButtonTest.java line 62:
> 
>> 60:     private CustomApproveButtonTest(String lookAndFeel) {
>> 61:         System.out.println("Testing Look & Feel : " + lookAndFeel);
>> 62:         frame = new JFrame("CustomApproveButtonTest");
> 
> You should create the frame after you set the Look-and-Feel.

Updated.

> test/jdk/javax/swing/JFileChooser/CustomApproveButtonTest.java line 69:
> 
>> 67:             throw new RuntimeException("Failed to set " + lookAndFeel +
>> 68:                     " LookAndFeel. Error : " + e);
>> 69:         }
> 
> You should probably quietly ignore the Look-and-Feel if `UnsupportedLookAndFeelException` is thrown.

Updated.

> test/jdk/javax/swing/JFileChooser/CustomApproveButtonTest.java line 75:
> 
>> 73:         frame.add(fileChooser);
>> 74:         frame.setVisible(true);
>> 75:         frame.pack();
> 
> Suggestion:
> 
>         frame.pack();
>         frame.setVisible(true);
> 
> 
> Usually, `pack()` goes before `setVisible` to ensure the size is calculated before the frame is made visible so that no flickering occurs because the size gets updated.

Updated.

> test/jdk/javax/swing/JFileChooser/CustomApproveButtonTest.java line 96:
> 
>> 94:         if (frame != null) {
>> 95:             frame.dispose();
>> 96:         }
> 
> The easiest way to avoid duplicate code for disposing of the frame is to use `try`-`finally` block which is commonly used in tests:
> 
> 
> // Set Look-and-Feel
> try {
>     frame = new JFrame("title");
>     // The rest of the test code
> 
>     // You can throw any exception to fail the test,
>     // the finally block below will still dispose of the frame.
> } finally {
>     if (frame != null) {
>         frame.dispose();
>     }
> }

Updated.

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

PR: https://git.openjdk.org/jdk/pull/11901