Thanks for the review Sergey. Could you also add your review to the CSR? Krishna
-----Original Message----- From: Sergey Bylokhov Sent: Thursday, October 4, 2018 7:12 AM To: Krishna Addepalli <[email protected]> Cc: [email protected] Subject: Re: <Swing Dev> [12]RFR:JDK-8182041- FIle Chooser Shortcut Panel folders under on JDK9 Looks fine. On 26/09/2018 04:50, Krishna Addepalli wrote: > Hi Sergey, > > Per our conversation, I have fixed the test to run on Mac and Linux as > well. Here is the new webrev: > http://cr.openjdk.java.net/~kaddepalli/8182041/webrev05/ > <http://cr.openjdk.java.net/%7Ekaddepalli/8182041/webrev05/> > > Thanks, > > Krishna > > *From:*Krishna Addepalli > *Sent:* Monday, September 17, 2018 10:44 PM > *To:* Sergey Bylokhov <[email protected]> > *Cc:* [email protected] > *Subject:* Re: <Swing Dev> [12]RFR:JDK-8182041- FIle Chooser Shortcut > Panel folders under on JDK9 > > Hi Sergey, > > Thanks for checking on Mac. I realised that there is no code which > filters any inaccessible links, nor there are any special folders that > are returned on Mac. It simply returns the user home folder in the array. > > I have made the test to run only on Windows. > > Here is the new webrev: > http://cr.openjdk.java.net/~kaddepalli/8182041/webrev04/ > <http://cr.openjdk.java.net/%7Ekaddepalli/8182041/webrev04/> > > Thanks, > > Krishna > > > > On 11-Sep-2018, at 1:08 PM, Sergey Bylokhov > <[email protected] <mailto:[email protected]>> wrote: > > Hi, Krishna. > Please recheck the new "ShellFolderQueriesSecurityManagerTest", it > does not work when I run it on macOS. > > On 10/09/2018 06:08, Krishna Addepalli wrote: > > Hi Phil/Sergey, > As per our conversation I have modified the fix to include > following: > 1. Adding final modifier to the new API. > 2. Finding the usages of “fileChooserShortcutPanelFiles”, and > replacing it with the new API - Fortunately, there is only one > place in WindowsPlacesBar.java, and I have changed it. > 3. Modified the comments for the return value to read as > follows: “The array returned may be possibly empty if there are > no appropriate > Permissions.” > Here is the link to the new > webrev:http://cr.openjdk.java.net/~kaddepalli/8182041/webrev03/ > > <http://cr.openjdk.java.net/%7Ekaddepalli/8182041/webrev03/><http://cr.openjdk.java.net/%7Ekaddepalli/8182041/webrev03/> > Thanks, > Krishna > > On 05-Sep-2018, at 5:09 PM, Krishna Addepalli > <[email protected] > > <mailto:[email protected]><mailto:[email protected]>> > wrote: > > Hi Phil, > > Posting your question: Is it valid to fail the test, if it > returns no shortcut files? > > The test runs only on windows, and without security manager, > it is expected to return some well known shortcuts like > "Desktop", "Network" etc, which is why I'm failing if none > of these are returned. > > Thanks, > Krishna > > -----Original Message----- > From: Krishna Addepalli > Sent: Wednesday, September 5, 2018 5:04 PM > To: Sergey Bylokhov <[email protected] > > <mailto:[email protected]><mailto:[email protected]>>; > Prasanta Sadhukhan <[email protected] > > <mailto:[email protected]><mailto:[email protected]>>;[email protected] > > <mailto:[email protected]><mailto:[email protected]> > Subject: Re: <Swing Dev> [12]RFR:JDK-8182041- FIle Chooser > Shortcut Panel folders under on JDK9 > > Hi Sergey, > > This method is a public API, and hence it is only meant for > clients using the API. Internally, WindowsPlacesBar.java > directly calls > ShellFolder.get("fileChooserShortcutPanelFolders"). > Couple other classes Win32ShellFolderManager2.java and > ShellFolderManager.java also use it directly. > Ofcousre, it can be changed to call the method in > FileSystemView.java, but, this api is modeled after > FileSystemView::getChooserComboBoxFiles(), which also does > something similar. > > Thanks, > Krishna > > -----Original Message----- > From: Sergey Bylokhov > Sent: Wednesday, September 5, 2018 4:28 AM > To: Krishna Addepalli <[email protected] > > <mailto:[email protected]><mailto:[email protected]>>; > Prasanta Sadhukhan <[email protected] > > <mailto:[email protected]><mailto:[email protected]>>;[email protected] > > <mailto:[email protected]><mailto:[email protected]> > Subject: Re: <Swing Dev> [12]RFR:JDK-8182041- FIle Chooser > Shortcut Panel folders under on JDK9 > > > BTW I am not sure do we need this refinement, or maybe > this text is > enough: "Returns an array of files representing the > values which will > be shown in the file chooser shortcuts panel." > > > One more notes, if result of this method should be "shown in > the file chooser shortcuts panel", then this method should > be called somewhere, isn't it? > > > > > I also think that "which may be shown" is better than > "which will be > shown". > > > On 30/08/2018 04:15, Krishna Addepalli wrote: > > Hi Prasanta, > > Thanks for the review. Here is the link to CSR: > https://bugs.openjdk.java.net/browse/JDK-8210210 > > Krishna > > *From:*Prasanta Sadhukhan > *Sent:* Thursday, August 30, 2018 12:28 PM > *To:* Krishna Addepalli > <[email protected] > <mailto:[email protected]>>; > [email protected] > <mailto:[email protected]> > *Subject:* Re: <Swing Dev> [12]RFR:JDK-8182041- FIle > Chooser Shortcut > Panel folders under on JDK9 > > ok. in that case, fix looks good. You will need a CSR. > > I guess the test will fail compilation without fix > so no exception > will be thrown but I guess we cannot escape that. > > Regards > Prasanta > > On 8/29/2018 6:50 PM, Krishna Addepalli wrote: > > I presume you are hinting at binary compatibility. I > think, that > should not be an issue, since we are not modifying > any of the > existing functions, but adding a new function. So, I > don't think > there would be any linking issues. Correct me if I'm > wrong. > > Thanks, > > Krishna > > *From:*Krishna Addepalli > *Sent:* Wednesday, August 29, 2018 5:29 PM > *To:* Prasanta Sadhukhan > <[email protected] > <mailto:[email protected]>> > <mailto:[email protected]>; > [email protected] > <mailto:[email protected]> > <mailto:[email protected]> > *Subject:* RE: <Swing Dev> [12]RFR:JDK-8182041- FIle > Chooser > Shortcut Panel folders under on JDK9 > > But we cannot have default methods in abstract > classes. Only > interfaces allow that. > > Thanks, > > Krishna > > *From:*Prasanta Sadhukhan > *Sent:* Wednesday, August 29, 2018 3:21 PM > *To:* Krishna Addepalli > <[email protected] > <mailto:[email protected]>> > <mailto:[email protected]>; > [email protected] > <mailto:[email protected]> > <mailto:[email protected]> > *Subject:* Re: <Swing Dev> [12]RFR:JDK-8182041- FIle > Chooser > Shortcut Panel folders under on JDK9 > > On 8/29/2018 2:23 PM, Krishna Addepalli wrote: > > Hi Prasanta, > > The new method "getChooserShortcutPanelFiles", and > the method > "getChooserComboBoxFiles" are present in > FileSystemView class > itself, which makes it a default implementation already. > > I doubt that. If an application has extended jdk9 > FileSystemView > class and has its own implementation for its OS, > then it probably > will not link with jdk12 modified FileSystemView > class as it > contains a new method. > Probably, you should consider adding "default" > keyword to your > new > method. > > Regards > Prasanta > > As for the string "fileChooserShortcutPanelFolders", > it goes > to > OS specific implementation and provides appropriate > result as > per the underlying OS. > > Thanks, > > Krishna > > *From:*Prasanta Sadhukhan > *Sent:* Wednesday, August 29, 2018 2:07 PM > *To:* Krishna Addepalli > <[email protected] > <mailto:[email protected]>> > <mailto:[email protected]>; > [email protected] > <mailto:[email protected]> > <mailto:[email protected]> > *Subject:* Re: <Swing Dev> [12]RFR:JDK-8182041- FIle > Chooser > Shortcut Panel folders under on JDK9 > > Hi Krishna, > > One more thing...it is mentioned here > > 58 * Java Licensees may want to provide a different > implementation of > > 59 * FileSystemView to better handle a given operating > system > > > If this class is ever extended by applications, then > would it > not be source incompatible change as there is no > default > > implementation of the new method? > > > Regards > > Prasanta > > On 8/27/2018 5:05 PM, Krishna Addepalli wrote: > > Hi Prasanta, > > Thanks for pointing those out. Corrected them in the new > webrev: > http://cr.openjdk.java.net/~kaddepalli/8182041/webrev01/ > > <http://cr.openjdk.java.net/%7Ekaddepalli/8182041/webrev01/> > > > <http://cr.openjdk.java.net/%7Ekaddepalli/8182041/webrev01/> > > Krishna > > *From:*Prasanta Sadhukhan > *Sent:* Monday, August 27, 2018 4:50 PM > *To:* Krishna Addepalli > <[email protected] > <mailto:[email protected]>> > <mailto:[email protected]>; > [email protected] > <mailto:[email protected]> > <mailto:[email protected]> > *Subject:* Re: <Swing Dev> [12]RFR:JDK-8182041- FIle > Chooser > Shortcut Panel folders under on JDK9 > > Hi Krishna, > > Quick comments: > @since 10 should be @since 12 in API javadoc > Is ShellFolderQueriesSecurityManagerTest a manual > test as > you mentioned > > @run main/manual/ > > > Regards > Prasanta > > On 8/27/2018 4:24 PM, Krishna Addepalli wrote: > > Hi All, > > 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/~kaddepalli/8182041/webrev00/ > > <http://cr.openjdk.java.net/%7Ekaddepalli/8182041/webrev00/> > > > <http://cr.openjdk.java.net/%7Ekaddepalli/8182041/webrev00/> > > New API method was added to query shortcut panel > entries > for JFileChooser, since ShellFolder is internal class > which is not publicly accessible. > > Thanks, > > Krishna > > > > -- > Best regards, Sergey. > > > > -- > Best regards, Sergey. > -- Best regards, Sergey.
