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

Reply via email to