On Wed, 31 May 2023 06:13:22 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 10 additional commits since
> the last revision:
>
> - Merge Master + Updated based on review comments
> - Merge branch 'master' of https://git.openjdk.java.net/jdk into
> branch_8307105
> - 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
Changes requested by aivanov (Reviewer).
src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line 861:
> 859: * @implNote Checks only if it is .lnk shortcut link. Other two links
> 860: * namely Symbolic and Junctions returns false, so that the
> 861: * absolute path can be resolved.
Suggestion:
*
* @implNote Returns {@code true} for {@code .lnk} shortcuts only.
* For <i>symbolic links</i> and <i>junctions</i>, it returns
* {@code false} even though {@code IShellFolder} returns
* {@code true} now. It is a workaround for easier handling of
* symbolic links and junctions.
The text may still be imperfect, however I hope, it explains the reasons
clearer.
src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line 868:
> 866: cachedIsLink = hasAttribute(ATTRIB_LINK)
> 867: && (!isFileSystem()
> 868: ||
> getPath().toLowerCase().endsWith(".lnk"));
Suggestion:
cachedIsLink = hasAttribute(ATTRIB_LINK)
&& (!isFileSystem()
|| getPath().toLowerCase().endsWith(".lnk"));
One space less, the first ampersand in `&&` should align with `h` on the line
above.
-------------
PR Review: https://git.openjdk.org/jdk/pull/13998#pullrequestreview-1452558817
PR Review Comment: https://git.openjdk.org/jdk/pull/13998#discussion_r1211332971
PR Review Comment: https://git.openjdk.org/jdk/pull/13998#discussion_r1211321342
