On Mon, 9 Jan 2023 10:24:49 GMT, Tejesh R <[email protected]> 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
