On Tue, 25 May 2021 23:36:43 GMT, Alexander Zuev <[email protected]> wrote:
>> Fix updated after first round of review.
>
> Alexander Zuev has updated the pull request incrementally with one additional
> commit since the last revision:
>
> null file now properly causes IllegalArgumentException
> Small fixed in JavaDoc
src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line
296:
> 294:
> 295: if (f == null){
> 296: throw new IllegalArgumentException("File reference should be
> specified");
Shall the exception message be more specific: "The file reference should not be
null"?
src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line
300:
> 298:
> 299: if(!f.exists()) {
> 300: return null;
Shall it throw `FileNotFoundException` or `IllegalArgumentException` if the
file doesn't exist?
It could more convenient to return `null` rather than catch an exception.
The space is missing between if and the opening parenthesis.
test/jdk/javax/swing/JFileChooser/FileSystemView/SystemIconTest.java line 58:
> 56:
> 57: static void negativeTests() {
> 58: ImageIcon icon;
Can it be icon? This test doesn't use the fact that the returned object is
`ImageIcon`.
test/jdk/javax/swing/JFileChooser/FileSystemView/SystemIconTest.java line 67:
> 65: if (!gotException) {
> 66: throw new RuntimeException("Negative size icon should throw
> invalid argument exception");
> 67: }
A suggestion: throw the exception inside try-block and ignore the expected
exception, then `gotException` can be dropped.
Suggestion:
try {
fsv.getSystemIcon(new File("."), -1, 16);
throw new RuntimeException("Negative size icon should throw invalid
argument exception");
} catch (IllegalArgumentException iae) {
// expected
}
Shall the test also exercise 0 as an invalid parameter? Shall the test also
pass an invalid height?
-------------
PR: https://git.openjdk.java.net/jdk/pull/2875
