On Tue, 30 May 2023 07:08:47 GMT, Tejesh R <[email protected]> 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 with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains eight additional commits since > the last revision: > > - Updated new fix > - Merge branch 'master' of https://git.openjdk.java.net/jdk into > branch_8307105 > - White space fix > - Alternate Fix > - Updated based on review comments > - Updated based on review comments > - Updated based on review comments > - Fix + Manual Test
Other than the couple of comments, the fix looks good to me now. The `BasicFileChooserUI.Handler.valueChanged` method looks like it was before the [JDK-8281966](https://bugs.openjdk.org/browse/JDK-8281966) fix. src/java.desktop/share/classes/javax/swing/plaf/basic/BasicFileChooserUI.java line 742: > 740: setDirectorySelected(true); > 741: setDirectory(file); > 742: I guess this blank line can be removed. src/java.desktop/share/classes/javax/swing/plaf/basic/BasicFileChooserUI.java line 1250: > 1248: } > 1249: > 1250: I am for preserving the blank line here, two blank lines separate method bodies from a nested class. src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line 862: > 860: //Checks only if it's .lnk shortcut link. Other two links > 861: //namely Symbolic and Junctions returns false, so that the > 862: //absolute path can be resolved. Should it be included inside the javadoc as an `@implNote`? The class isn't public so we can change it, and it is more helpful because it's shown right away without opening source code for the method. src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line 867: > 865: cachedIsLink = hasAttribute(ATTRIB_LINK) > 866: && (!isFileSystem() > 867: || getPath().toLowerCase().endsWith(".lnk")); At the very least, you should indent `||` farther because it's inside the parentheses (the condition is `c1 && (!c2 || c3)`), otherwise it could be misinterpreted as `c1 && !c2 || c3` which is completely different. Suggestion: cachedIsLink = hasAttribute(ATTRIB_LINK) && (!isFileSystem() || getPath().toLowerCase().endsWith(".lnk")); However, my personal preference is to align wrapped lines to the start of their corresponding part of the expression: Suggestion: cachedIsLink = hasAttribute(ATTRIB_LINK) && (!isFileSystem() || getPath().toLowerCase().endsWith(".lnk")); Either way is fine. ------------- PR Review: https://git.openjdk.org/jdk/pull/13998#pullrequestreview-1450471218 PR Review Comment: https://git.openjdk.org/jdk/pull/13998#discussion_r1209994712 PR Review Comment: https://git.openjdk.org/jdk/pull/13998#discussion_r1209995818 PR Review Comment: https://git.openjdk.org/jdk/pull/13998#discussion_r1209978564 PR Review Comment: https://git.openjdk.org/jdk/pull/13998#discussion_r1209973851
