On Wed, 11 Jan 2023 10:29:52 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.
>
> 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
