RFR: 8319938: com/sun/java/swing/plaf/gtk/TestFileChooserSingleDirectorySelection.java fails with "getSelectedFiles returned empty array for LAF: javax.swing.plaf.metal.MetalLookAndFeel" [v3]

[Abhishek Kumar]

On Mon, 4 Dec 2023 21:00:58 GMT, Alexey Ivanov <aivanov at openjdk.org> 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