On Wed, 17 May 2023 16:47:56 GMT, Tejesh R <t...@openjdk.org> wrote: >> This is a regression from fix >> [JDK-8281966](https://bugs.openjdk.org/browse/JDK-8281966): Absolute path of >> symlink is null in JFileChooser. The fix checks whether the file path is a >> symbolic link using `Files.isSymbolicLink()` method with path as input. In >> windows for specific folders like "This PC"/"Network"/"Libraries" the path >> value will be a hex values which causes InvalidPathException. In order to >> resolve the issue, since no other checks are available to validate the path >> of these folders, checking if the file is link firstly and then for symbolic >> link resolves the problem (since File.isLink() doesn't take path as input >> rather file is a parameter). Since every symbolic link is a link, this fix >> seems logical to me. >> The fix is tested in CI for regression and is green. The regression fix is >> also tested for confirmation and works fine. > > Tejesh R has updated the pull request incrementally with one additional > commit since the last revision: > > Updated based on review comments
Changes requested by aivanov (Reviewer). src/java.desktop/share/classes/javax/swing/plaf/basic/BasicFileChooserUI.java line 725: > 723: || > (chooser.isDirectorySelectionEnabled() > 724: && (fsv.isFileSystem(f) > 725: || (fsv.isLink(f) && > Files.isSymbolicLink(f.toPath()))) I don't think it's correct. `Files.isSymbolicLink` should only be called for objects for which `fsv.isFileSystem(f)` returns `true`. `fsv.isLink(f)` returns `true` for `.lnk` files which are the common Windows shortcuts; such a file can also be a symbolic link. test/jdk/javax/swing/JFileChooser/FileChooserIPETest.java line 45: > 43: */ > 44: > 45: public class FileChooserIPETest { Suggestion: public class FileChooserInvalidPathExceptionTest { Isn't it more descriptive this way? IPE isn't as common as NPE. test/jdk/javax/swing/JFileChooser/FileChooserIPETest.java line 68: > 66: Network. > 67: 2. Select and traverse through those folders. > 68: 3. If InvalidPathException occurs then test FAIL else > test is PASS. You may be more specific: if exception occurs, the test fails automatically. ------------- PR Review: https://git.openjdk.org/jdk/pull/13998#pullrequestreview-1436912464 PR Review Comment: https://git.openjdk.org/jdk/pull/13998#discussion_r1200690007 PR Review Comment: https://git.openjdk.org/jdk/pull/13998#discussion_r1200711027 PR Review Comment: https://git.openjdk.org/jdk/pull/13998#discussion_r1200708730