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

[Alexander Scherbatiy]

  The fix looks good to me.

  Thanks,
  Alexandr.

On 17/02/16 19:20, Sergey Bylokhov wrote:
> 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.
>>>>
>>>>
>>>
>>
>
>