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

[Sergey Bylokhov]

On 24/11/15 13:01, Semyon Sadetsky wrote:
>
>
> On 11/2/2015 11:09 PM, Sergey Bylokhov wrote:
>> On 29.10.15 21:30, Phil Race wrote:
>>> We should specify what happens if you pass in to get(String)
>>> a) null
>>> b) an unrecognised name.
>>>
>>> Would it make sense to define string constants on FileSystemView
>>> as otherwise people have to spell these exactly right without compiler
>>> help ?
>>>
>>> Also I expect (hope!) that we do not assert any privileges here so there
>>> will be a SecurityException if the caller does not have the necessary
>>> permissions.
>>> That should be documented.
>>>
>>> I find it odd that isLink(File) catches FNFE and just returns "false"
>>> like this
>>> was a normal file. Why is that ? In fact I find it particularly odd
>>> since
>>> getLinkLocation() *does* throw FNFE if it does not exist.
>>
>>     I guess the new code should follow the rules which are used by
>> other methods in the same class, most of them catch FNFE and return
>> some default value. Also most of them returns default value if the
>> passed File is null. Anyway I think that NPE is better than IAE. At
>> least we should discuss this.
> Could you explain why NPE is better?

Because it is caused by incorrect usage of null, for which NPE was created.

I supposed in case of an incorrect
> method usage IAE is more precise than generic NPE.
> The FNFE is used in some methods of the class. I think that is justified
> if the linked file is not found. In other cases it is caught.

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.

>>
>> I am not sure why this methods are static unlike old/others methods().
> I agree, we should be following the class "style", so I have removed
> static modifiers.
> The updated webrev:
> http://cr.openjdk.java.net/~ssadetsky/8081722/webrev.02/
>>
>> public static Object get(String key) {}
>>     Probably this method too flexible? What kind of use case for this
>> method inside the users application? How the user will know which
>> parameters to use in a cross-platform manner?
>>
>> For example "fileChooserDefaultFolder" property already covered by
>> FileSystemView.getDefaultDirectory(), the "roots" is covered by
>> getRoots().
> I don't think that we should elaborate a new Shell API in this fix,
> because we initially aimed to make the minimal change we can to support
> NetBeans ShellFolder extension.  We did a poll on the public alias which
> showed nobody else need this API to be opened.
>>
>>
>>>
>>> Also the docs casually say
>>> "a shell interpreted link" and "platform shell" without explaining
>>> what they mean by a shell. Should this term be used at all or
>>> should the docs describe this in some other words ?
>>>
>>> getLinkLocation says
>>>
>>> "Returns a file linked to the specified file if the specified file"
>>>
>>> What that reads like to me is that you get back a link if
>>> you pass in a regular file, whereas (I believe) you mean
>>> the opposite.
>>>
>>> I think you mean :
>>> "Returns the regular file referenced by the specified link file"
>>>
>>> I also note that one of the uses we located was of
>>> ShellFolder.getShellFolder(File)
>>> That internal API returns a ShellFolder but it we expose that it would
>>> have to
>>> be the java.io.File superclass I expect.
>>>
>>> -phil.
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>> -phil.
>>>
>>>
>>> On 10/26/2015 06:51 AM, Semyon Sadetsky wrote:
>>>> Hello,
>>>>
>>>> Please review fix for JDK9:
>>>>
>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8081722
>>>> webrev: http://cr.openjdk.java.net/~ssadetsky/8081722/webrev.00
>>>>
>>>> As the solution it is suggested to have 3 new static methods in the
>>>> javax.swing.filechooser.FileSystemView class. Those methods proxy
>>>> sun.awt.shell.ShellFolder calls and it is enough to cover NetBeans
>>>> request. getShellFolder() method is not added because it returns the
>>>> ShellFolder instance which is not for public use under the proposed
>>>> approach.
>>>> The CCC request will be rework when the fix is approved.
>>>>
>>>> --Semyon
>>>
>>
>>
>


-- 
Best regards, Sergey.