On Wed, 22 Nov 2023 05:42:24 GMT, Abhishek Kumar <[email protected]> wrote:
>> The test fails for JFileChooser selection mode set to `DIRECTORIES_ONLY`.
>> For `DIRECTORIES_ONLY `mode, there may not be any directories in home
>> directory and due to that test failed. Added the code to create temporary
>> directories and files for the test.
>> Tested the current change on the machine it failed for multiple times, no
>> failure observed.
>> CI link attached in JBS.
>
> Abhishek Kumar has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Test update
Changes requested by aivanov (Reviewer).
test/jdk/com/sun/java/swing/plaf/gtk/TestFileChooserSingleDirectorySelection.java
line 1:
> 1: /*
Since GitHub doesn't allow adding comments to lines which aren't modified:
In `doTesting`, you should get the location and dimensions of the frame on EDT
(and for the button too).
The `passed` field is to be declared `volatile`: you modify it on EDT in the
button listener but you read its value on main thread.
Basically, you don't need the button, you can run the code in the button action
listener directly. Be sure to run it on EDT though.
The listener code can be modified to perform only one volatile write:
public void actionPerformed(ActionEvent evt) {
File files[] = fileChooser.getSelectedFiles();
passed = files.length != 0;
}
test/jdk/com/sun/java/swing/plaf/gtk/TestFileChooserSingleDirectorySelection.java
line 26:
> 24: /*
> 25: * @test
> 26: * @bug 6972078 8319938
It's a test bug, no code in JDK is changed as the result — the bugid shouldn't
be added.
test/jdk/com/sun/java/swing/plaf/gtk/TestFileChooserSingleDirectorySelection.java
line 57:
> 55: private static File testFile;
> 56: private static File[] SubDirs;
> 57: private static File[] subFiles;
You don't use these arrays, they could be local variables; but even there
they're not needed — a local variable for a current file or directory is enough.
test/jdk/com/sun/java/swing/plaf/gtk/TestFileChooserSingleDirectorySelection.java
line 66:
> 64: createFilesOnlyDir();
> 65: populateDirs();
> 66: populateFiles();
Returning and passing the `File` object would make code flow clearer:
Suggestion:
testDir = createFoldersOnlyDir();
populateDirs(testDir);
testFile = createFilesOnlyDir();
populateFiles(testFile);
I still find `testFile` confusing because it represents a directory. What about
`testDirDirs`, `testDirFiles`?
test/jdk/com/sun/java/swing/plaf/gtk/TestFileChooserSingleDirectorySelection.java
line 93:
> 91: SubDirs[i].mkdir();
> 92: SubDirs[i].deleteOnExit();
> 93: }
The array is redundant:
Suggestion:
for (int i = 0; i < 10; ++i) {
File subDir = new File(testDir, "subDir_" + (i+1));
subDir.mkdir();
subDir.deleteOnExit();
}
test/jdk/com/sun/java/swing/plaf/gtk/TestFileChooserSingleDirectorySelection.java
line 111:
> 109: ".txt", new File(testFile.getAbsolutePath()));
> 110: subFiles[i].deleteOnExit();
> 111: }
The array is redundant. I also suggest following the same pattern for creating
files:
Suggestion:
for (int i = 0; i < 10; ++i) {
File subFile = new File(testFile, "subFiles_" + (i+1));
subFile.createNewFile();
subFile.deleteOnExit();
}
test/jdk/com/sun/java/swing/plaf/gtk/TestFileChooserSingleDirectorySelection.java
line 143:
> 141: createAndShowUI();
> 142: fileChooser.setCurrentDirectory(testDir);
> 143: });
`fileChooser.setFileSelectionMode` that you call below should be called on EDT,
I suggest moving it into this block.
This applies to all `check-*` methods.
-------------
PR Review: https://git.openjdk.org/jdk/pull/16674#pullrequestreview-1763366339
PR Review Comment: https://git.openjdk.org/jdk/pull/16674#discussion_r1414493186
PR Review Comment: https://git.openjdk.org/jdk/pull/16674#discussion_r1414468948
PR Review Comment: https://git.openjdk.org/jdk/pull/16674#discussion_r1414470226
PR Review Comment: https://git.openjdk.org/jdk/pull/16674#discussion_r1414472923
PR Review Comment: https://git.openjdk.org/jdk/pull/16674#discussion_r1414474303
PR Review Comment: https://git.openjdk.org/jdk/pull/16674#discussion_r1414480135
PR Review Comment: https://git.openjdk.org/jdk/pull/16674#discussion_r1414481695
