On Tue, 5 Dec 2023 12:17:55 GMT, Abhishek Kumar <abhis...@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
