On Wed, 17 May 2023 16:47:56 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 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
