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

[Alexey Ivanov]

On Wed, 11 Jan 2023 10:29:52 GMT, Tejesh R <tr at openjdk.org> wrote:

>> FileChooser Open/Approve button size is shown incorrectly when no Approve button text is set in `CUSTOM_DIALOG` type. Reason being that no default Approve Button text is returned during Dialog Type Property change. Since `null` is returned as Button string the Button size is incorrectly shown. The fix here addresses the issue by adding a default Approve Button Text when no text is set explicitly in case of `CUSTOM_DIALOG` type.
>> Automated test is attached which has been tested with multiple test runs.
>
> 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.

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.

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.

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.

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.

test/jdk/javax/swing/JFileChooser/CustomApproveButtonTest.java line 83:

> 81:             return;
> 82:         }
> 83:         if (customApproveButton.getText() == null) {

Now that you test that the text on the Approve is non-null only, why is Aqua L&F excluded?

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();
    }
}

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

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