RFR: 8299522: JFilechooser open button size is incorrectly shown

[Damon Nguyen]

On Mon, 9 Jan 2023 10:24:49 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.

src/java.desktop/share/classes/javax/swing/plaf/basic/BasicFileChooserUI.java line 413:

> 411:         helpButtonText   = UIManager.getString("FileChooser.helpButtonText",l);
> 412:         directoryOpenButtonText = UIManager.getString("FileChooser.directoryOpenButtonText",l);
> 413:         customApproveButtonText = UIManager.getString("FileChooser.customApproveButtonText", l);

Not sure if it needs to be addressed throughout the file, but for the "L" var at the end of each getString(), sometimes there's a space after the comma and sometimes there's none. Maybe it's better to follow the same convention throughout the file. Maybe it's better to just copy the lines above the one's you added to match the code block only.

src/java.desktop/share/classes/javax/swing/plaf/basic/BasicFileChooserUI.java line 601:

> 599:         } else if(fc.getDialogType() == JFileChooser.SAVE_DIALOG) {
> 600:             return saveButtonToolTipText;
> 601:         } else if (fc.getDialogType() == JFileChooser.CUSTOM_DIALOG) {

Same spacing concern here.

src/java.desktop/share/classes/javax/swing/plaf/basic/BasicFileChooserUI.java line 928:

> 926:         } else if (fc.getDialogType() == JFileChooser.SAVE_DIALOG) {
> 927:             return saveButtonMnemonic;
> 928:         } else if(fc.getDialogType() == JFileChooser.CUSTOM_DIALOG) {

Spacing here too

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

> 50:                 UIManager.LookAndFeelInfo[] lookAndFeel = UIManager.getInstalledLookAndFeels();
> 51:                 for (UIManager.LookAndFeelInfo look : lookAndFeel) {
> 52:                     if(look.getClassName().equals(aquaLAF)) {

Would it be cleaner to remove the aquaLAF string and replace the getClassName() in the "if" condition at line 52 for look.getName().equals("Aqua")? This condenses the block by one line and keeps clarity.

Also there should be a space after the if.

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

> 61: 
> 62:     private CustomApproveButtonTest(String lookAndFeel) {
> 63:         System.out.println("Testing Look & Feel : "+lookAndFeel);

Spacing

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

> 91:         }
> 92: 
> 93:         if(frame != null) {

Spacing

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

> 113:                 if (result == null) {
> 114:                     result = button;
> 115:                 }

What's the `result == null` for? 

Is it meant to not recursively call findCustomApproveButton() if a JButton result is found? Because then the break at line 107 resolves that I think.

If it's to only have a result for the first instance of a non-null result, then wouldn't adding a similar break after `result = button` be better so we don't have to recursively call findCustomApproveButton() if a result is already found?

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

> 120: 
> 121:     private void fail(String s) {
> 122:         if(frame != null) {

Spacing after if

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

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