<Swing Dev> [10] Review request for 8182041: File Chooser Shortcut Panel folders under on JDK 9

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

<Swing Dev> [10] Review request for 8182041: File Chooser Shortcut Panel folders under on JDK 9

semyon.sadetsky
Hello,

Please review fix for JDK10 (the changes involve AWT and Swing):

bug: https://bugs.openjdk.java.net/browse/JDK-8182041

webrev: http://cr.openjdk.java.net/~ssadetsky/8182041/webrev.00/

New API method was added letting to query shortcut panel entries for
JFileChooser.

--Semyon

Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10] Review request for 8182041: File Chooser Shortcut Panel folders under on JDK 9

Alexander Zvegintsev

Hi Semyon,

the fix looks good to me, but I found a minor typo in the test:

testShortcatPanelFiles -> testShortcutPanelFiles

no need for a new webrev

Thanks,
Alexander.
On 04/10/2017 00:41, Semyon Sadetsky wrote:
Hello,

Please review fix for JDK10 (the changes involve AWT and Swing):

bug: https://bugs.openjdk.java.net/browse/JDK-8182041

webrev: http://cr.openjdk.java.net/~ssadetsky/8182041/webrev.00/

New API method was added letting to query shortcut panel entries for JFileChooser.

--Semyon


Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10] Review request for 8182041: File Chooser Shortcut Panel folders under on JDK 9

Philip Race
Hi,
I have some additional comments on this old review thread. Hopefully we can close it out soon
although it will need a CSR whatever ..

Since the implementation of ShellFolderManager filters out inaccessible files we
should document this somewhere.
I suggest either on the class or relevant methods saying something like
"Files or resources which are not accessible in the current security context
may be filtered out from the returned set".

The word "may" is key here ..

If we are sure that this is always the case then it would follow that SecurityException
does not need to be documented.
Consistency would suggest that then this policy would extend to the other methods
added in JDK 9 which declare that exception. So all or none.
Being a RuntimeException that is not checked I think we can compatibly remove those.

-phil.


On 10/24/2017 09:22 PM, Alexander Zvegintsev wrote:

Hi Semyon,

the fix looks good to me, but I found a minor typo in the test:

testShortcatPanelFiles -> testShortcutPanelFiles

no need for a new webrev

Thanks,
Alexander.
On 04/10/2017 00:41, Semyon Sadetsky wrote:
Hello,

Please review fix for JDK10 (the changes involve AWT and Swing):

bug: https://bugs.openjdk.java.net/browse/JDK-8182041

webrev: http://cr.openjdk.java.net/~ssadetsky/8182041/webrev.00/

New API method was added letting to query shortcut panel entries for JFileChooser.

--Semyon



Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10] Review request for 8182041: File Chooser Shortcut Panel folders under on JDK 9

semyon.sadetsky

On 12/06/2017 08:33 AM, Phil Race wrote:

Hi,
I have some additional comments on this old review thread. Hopefully we can close it out soon
although it will need a CSR whatever ..

Since the implementation of ShellFolderManager filters out inaccessible files we
should document this somewhere.
I suggest either on the class or relevant methods saying something like
"Files or resources which are not accessible in the current security context
may be filtered out from the returned set".

The word "may" is key here ..
This looks to me like implementation details. I agree that it is worth to mention this problematic in the method spec but since it is a generic method for different platforms it probably should be given in a different form than a particular platform specific solution. If the entries of the list are virtual they may not have any file system permissions at all.

If we are sure that this is always the case then it would follow that SecurityException
does not need to be documented.
Consistency would suggest that then this policy would extend to the other methods
added in JDK 9 which declare that exception. So all or none.
Being a RuntimeException that is not checked I think we can compatibly remove those.
This is not consistent with other JDK classes in which the SecurityException is always mentioned, for example, the java.awt.Desktop class and there are many others. I think it would be non-practical to rewrite all other specs because you've changed your mind in this particular fix review.
Also, in this fix review [1]  you brought an opposite point of view.

[1] http://mail.openjdk.java.net/pipermail/swing-dev/2015-October/005073.html

--Semyon


-phil.


On 10/24/2017 09:22 PM, Alexander Zvegintsev wrote:

Hi Semyon,

the fix looks good to me, but I found a minor typo in the test:

testShortcatPanelFiles -> testShortcutPanelFiles

no need for a new webrev

Thanks,
Alexander.
On 04/10/2017 00:41, Semyon Sadetsky wrote:
Hello,

Please review fix for JDK10 (the changes involve AWT and Swing):

bug: https://bugs.openjdk.java.net/browse/JDK-8182041

webrev: http://cr.openjdk.java.net/~ssadetsky/8182041/webrev.00/

New API method was added letting to query shortcut panel entries for JFileChooser.

--Semyon




Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10] Review request for 8182041: File Chooser Shortcut Panel folders under on JDK 9

Philip Race


On 12/06/2017 09:39 AM, Semyon Sadetsky wrote:

On 12/06/2017 08:33 AM, Phil Race wrote:

Hi,
I have some additional comments on this old review thread. Hopefully we can close it out soon
although it will need a CSR whatever ..

Since the implementation of ShellFolderManager filters out inaccessible files we
should document this somewhere.
I suggest either on the class or relevant methods saying something like
"Files or resources which are not accessible in the current security context
may be filtered out from the returned set".

The word "may" is key here ..
This looks to me like implementation details.

No. On the contrary. It can be specified.

I agree that it is worth to mention this problematic in the method spec but since it is a generic method for different platforms it probably should be given in a different form than a particular platform specific solution. If the entries of the list are virtual they may not have any file system permissions at all.

If we are sure that this is always the case then it would follow that SecurityException
does not need to be documented.
Consistency would suggest that then this policy would extend to the other methods
added in JDK 9 which declare that exception. So all or none.
Being a RuntimeException that is not checked I think we can compatibly remove those.
This is not consistent with other JDK classes in which the SecurityException is always mentioned, for example, the java.awt.Desktop class and there are many others. I think it would be non-practical to rewrite all other specs because you've changed your mind in this particular fix review.

The API contract of some other class such as Desktop is not relevant.

I am saying FileSystemView should be internally consistent


Also, in this fix review [1]  you brought an opposite point of view.

[1] http://mail.openjdk.java.net/pipermail/swing-dev/2015-October/005073.html

That was because no one told me at the time that we were filtering out to prevent leakage.
i.e I have new information.


-phil.

--Semyon


-phil.


On 10/24/2017 09:22 PM, Alexander Zvegintsev wrote:

Hi Semyon,

the fix looks good to me, but I found a minor typo in the test:

testShortcatPanelFiles -> testShortcutPanelFiles

no need for a new webrev

Thanks,
Alexander.
On 04/10/2017 00:41, Semyon Sadetsky wrote:
Hello,

Please review fix for JDK10 (the changes involve AWT and Swing):

bug: https://bugs.openjdk.java.net/browse/JDK-8182041

webrev: http://cr.openjdk.java.net/~ssadetsky/8182041/webrev.00/

New API method was added letting to query shortcut panel entries for JFileChooser.

--Semyon