<Swing Dev> [9] Review Request for 8081722: Provide public API for file hierarchy provided by sun.awt.shell.ShellFolder

[Sergey Bylokhov]

Looks fine.

On 15.02.16 18:17, Semyon Sadetsky wrote:
> Please look at the updated webrev:
> http://cr.openjdk.java.net/~ssadetsky/8081722/webrev.05/
> Changes was made According to off-line discussion wit Sergey:
>
> - IAE -> NPE for file == null
> - FNFE is corrected for getLinkLocation()
>
> --Semyon
>
> On 1/22/2016 9:34 AM, Semyon Sadetsky wrote:
>>
>>
>> On 1/21/2016 6:25 PM, Sergey Bylokhov wrote:
>>> On 21/01/16 09:16, Semyon Sadetsky wrote:
>>>>>
>>>>> There are still inconsistency. The isFileSystem(), isParent(),
>>>>> getSystemIcon, getSystemDisplayName() etc do not throw an exception in
>>>>> case of null, but return some default value(null,true,false). Same for
>>>>> FileNotFoundException() most of the methods just catch this exception
>>>>> and return the default value.
>>>> The methods you've mentioned do not throw NPE but other methods in the
>>>> FSV class do throw NPE if called with null argument.
>>>
>>> they raise NPE not an IAE. ok I am fine to use NPE.
>>>
>>> Some other issue is inconsistency between isLink() vs getLinkLocation()
>>> getLinkLocation() should "Returns {@code null} if the specified file
>>> is not a shell interpreted link", but isLink() return false if
>>> FileNotFoundException which means that getLinkLocation() should
>>> return null;
>>>
>>> the next code can work incorrectly:
>>>         FileSystemView fsv = FileSystemView.getFileSystemView();
>>>         File file = "some unexisted file";
>>>         if (fsv.isLink(file)) {
>>>             if (fsv.getLinkLocation(file) == null) {
>>>                 throw new RuntimeException();
>>>             }
>>>         } else {
>>>             if (fsv.getLinkLocation(file) != null) {
>>>                 throw new RuntimeException();
>>>             }
>>>         }
>>>
>> The code will not compile because FileNotFoundException is not caught.
>>
>> The logic of getLinkLocation() is pretty simple:
>> - If the file is not a link, it refers to nothing and the method
>> returns nothing (null).
>> - If the file is a link, it refers to an another file but in case the
>> another file cannot be found in the FS the FileNotFoundException is
>> thrown.
>>> There are some checks in the code related to "C:\pagefile.sys" and
>>> "C:\Winnt\Profiles\joe\history\History.IE5" can you double check how
>>> the new methods will work with these files.
>> This situations are only possible for the listFiles() return. If it
>> happens for a File object then it is a real error which should be
>> reported.
>>>
>>>
>>>> Your point that there is inconsistency sounds a bit odd.  If this
>>>> reasoning we can not add a new method which throws an exception to the
>>>> class not having methods throwing this exception. How methods of the
>>>> FSV
>>>> class related to different aspects of the file system can be
>>>> inconsistent? They are simply not related to each other.
>>>
>>>
>>
>


-- 
Best regards, Sergey.