On Mon, 4 Dec 2023 21:00:58 GMT, Alexey Ivanov <[email protected]> wrote:
>> Abhishek Kumar has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Test update
>
> 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;
> }
`Button` and `passed` field are removed since it may not be required. The
selected files are fetched in `checkResult` method.
> 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.
Updated.
> 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.
Updated.
> 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`?
Updated the var names.
> 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();
> }
Updated.
> 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();
> }
Updated.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16674#discussion_r1415506579
PR Review Comment: https://git.openjdk.org/jdk/pull/16674#discussion_r1415500444
PR Review Comment: https://git.openjdk.org/jdk/pull/16674#discussion_r1415500684
PR Review Comment: https://git.openjdk.org/jdk/pull/16674#discussion_r1415501849
PR Review Comment: https://git.openjdk.org/jdk/pull/16674#discussion_r1415502104
PR Review Comment: https://git.openjdk.org/jdk/pull/16674#discussion_r1415502358
