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

[Alexey Ivanov]

On Tue, 5 Dec 2023 12:17:55 GMT, Abhishek Kumar <abhiscxk at openjdk.org> 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:
> 
>   Review comment fix

Changes requested by aivanov (Reviewer).

test/jdk/com/sun/java/swing/plaf/gtk/TestFileChooserSingleDirectorySelection.java line 69:

> 67:             checkFileOnlyTest(laf, testDirFiles);
> 68:             checkDirectoriesOnlyTest(laf, testDirDirs);
> 69:             checkFilesAndDirectoriesTest(laf, testDirDirs);

So, `FILES_AND_DIRECTORIES` mode is tested with directories only. Is it intentional? It's the problem in [JDK-6972078](https://bugs.openjdk.org/browse/JDK-6972078)… that directories can't be selected.

test/jdk/com/sun/java/swing/plaf/gtk/TestFileChooserSingleDirectorySelection.java line 84:

> 82:     }
> 83: 
> 84:     private static void populateDirs(File dirsDir) {

Suggestion:

    private static void populateDirs(File parent) {

It better conveys the meaning of the parameter.

test/jdk/com/sun/java/swing/plaf/gtk/TestFileChooserSingleDirectorySelection.java line 88:

> 86:             File SubDirs = new File(dirsDir, "subDir_" + (i+1));
> 87:             SubDirs.mkdir();
> 88:             SubDirs.deleteOnExit();

Suggestion:

            File subDir = new File(dirsDir, "subDir_" + (i+1));
            subDir.mkdir();
            subDir.deleteOnExit();

It's _one_ subdirectory; local variable names start with lower-case letter in Java.

test/jdk/com/sun/java/swing/plaf/gtk/TestFileChooserSingleDirectorySelection.java line 102:

> 100:     }
> 101: 
> 102:     private static void populateFiles(File filesDir) throws Exception {

Suggestion:

    private static void populateFiles(File parent) throws Exception {

Or `parentDir`.

test/jdk/com/sun/java/swing/plaf/gtk/TestFileChooserSingleDirectorySelection.java line 111:

> 109: 
> 110:     private static void checkFileOnlyTest(UIManager.LookAndFeelInfo laf,
> 111:                                           File crntDir) throws Exception {

Suggestion:

                                          File dir) throws Exception {

Alternatively, spell out `current`; if you prefer to abbreviate, `curr` is rather standard abbreviation for ‘current’. However, I think `dir` is enough, its meaning is easily inferred as it's passed to `fileChooser.setCurrentDirectory`.

test/jdk/com/sun/java/swing/plaf/gtk/TestFileChooserSingleDirectorySelection.java line 134:

> 132: 
> 133:     private static void checkDirectoriesOnlyTest(UIManager.LookAndFeelInfo laf,
> 134:                                              File crntDir) throws Exception {

Suggestion:

    private static void checkDirectoriesOnlyTest(UIManager.LookAndFeelInfo laf,
                                                 File crntDir) throws Exception {

Align the parameters, if you follow this style.

test/jdk/com/sun/java/swing/plaf/gtk/TestFileChooserSingleDirectorySelection.java line 196:

> 194:             frameLocation = fileChooser.getLocationOnScreen();
> 195:             frameWidth = frame.getWidth();
> 196:             frameHeight = frame.getHeight();

Suggestion:

            bounds = new Rectangle(fileChooser.getLocationOnScreen(),
                                   frame.getSize());

Isn't one variable enough? Moreover `frameLocation` stores the location of `fileChooser`.

Since now there's only one click location, this can be simplified further:
Suggestion:

            Point fileChooserLocation = fileChooser.getLocationOnScreen();
            fileChooserLocation.y += frame.getHeight() / 3;
            clickLocation = new Point(fileChooserLocation);


Then you modify your `clickMouse` method to accept two parameters: `Point` and `xOffset`.

test/jdk/com/sun/java/swing/plaf/gtk/TestFileChooserSingleDirectorySelection.java line 217:

> 215:                     "empty array for LAF: " + laf.getClassName());
> 216:         }
> 217:     }

And you ignored _<q cite="https://github.com/openjdk/jdk/pull/16674#discussion_r1414493186">Be sure to run it on EDT</q>_: `fileChooser.getSelectedFiles()` is to be called on EDT.

-------------

PR Review: https://git.openjdk.org/jdk/pull/16674#pullrequestreview-1765091146
PR Review Comment: https://git.openjdk.org/jdk/pull/16674#discussion_r1415660454
PR Review Comment: https://git.openjdk.org/jdk/pull/16674#discussion_r1415618467
PR Review Comment: https://git.openjdk.org/jdk/pull/16674#discussion_r1415620215
PR Review Comment: https://git.openjdk.org/jdk/pull/16674#discussion_r1415620925
PR Review Comment: https://git.openjdk.org/jdk/pull/16674#discussion_r1415624835
PR Review Comment: https://git.openjdk.org/jdk/pull/16674#discussion_r1415627148
PR Review Comment: https://git.openjdk.org/jdk/pull/16674#discussion_r1415631687
PR Review Comment: https://git.openjdk.org/jdk/pull/16674#discussion_r1415644623