RFR: 8307105: JFileChooser InvalidPathException when selecting some system folders on Windows [v7]

[Alexey Ivanov]

On Tue, 30 May 2023 07:08:47 GMT, Tejesh R <tr at 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 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