<Swing Dev> [9] Review request for 8041694: JFileChooser removes trailing spaces in the selected directory name

[Anton Litvinov]

Hello Sergey and Semyon,

According to today's offline discussion with Sergey, it was decided to 
make the automatic regression test not only MS Windows OS specific, but 
cross-platform. In order to resolve the encountered problem on OS X, 
which was described in my previous e-mail, it was necessary only to set 
Metal L&F in the beginning of the test. Could you please review the 3-rd 
version of the fix which allows the test to be fully accomplished on all 
supported platforms.

Webrev (the 3rd version): 
http://cr.openjdk.java.net/~alitvinov/8041694/jdk9/webrev.02

The fix was successfully verified using the new version of the 
regression test on MS Windows OS, Linux OS and OS X.

Thank you,
Anton

On 5/13/2016 9:13 PM, Anton Litvinov wrote:
> Hello Semyon, Sergey and Alexander,
>
> Thank you very much for review of this fix. The new version of the fix 
> was created. Could you please review the second version of the fix.
>
> Webrev (the 2nd version): 
> http://cr.openjdk.java.net/~alitvinov/8041694/jdk9/webrev.01
>
> The second version of the fix is different from the first version only 
> in the regression test part, where the following changes were introduced:
>
> 1. The shell script was eliminated.
> 2. The number of threads involved in the test was reduced from 3 to 2, 
> therefore extra synchronization was removed.
> 3. The main thread waits until JFileChooser is shown using 
> "robot.delay(1000);" and "robot.waitForIdle();" calls instead of 
> previously used do/while loop with "Thread.sleep(50)" in the 3rd 
> thread, which is eliminated in the new version of the fix.
> 4. The test was made to be only Windows specific "if 
> (OSInfo.getOSType() != OSInfo.OSType.WINDOWS) {", because, for 
> example, on OS X JFileChooser is absolutely different from Windows 
> version and does not allow to type a file name from the keyboard. 
> Support of OS X platform by the regression test would require complete 
> rework of the whole test idea.
>
> Thank you,
> Anton
>
> On 5/13/2016 8:19 PM, Semyon Sadetsky wrote:
>>
>>
>> On 5/13/2016 7:54 PM, Alexander Potochkin wrote:
>>> Hello
>>>
>>>>> I just checked the code in JDK9 and found that it still syncs only 
>>>>> with the EDT
>>>>> and completely ignores the toolkit thread.
>>>> I cannot agree. Robot uses native SunToolkit.syncNativeQueue() to 
>>>> syncs it with the native event queue.
>>>
>>> Robot.waitForIdle() doesn't use SunToolkit.syncNativeQueue()
>>> it uses SunToolkit.flushPendingEvents() but it is a different story
>> Alexander, I just double-checked the jdk9 code. See this
>>
>>     public synchronized void waitForIdle() {
>>         checkNotDispatchThread();
>>         SunToolkit.flushPendingEvents();
>>         ((SunToolkit) Toolkit.getDefaultToolkit()).realSync();
>>     }
>>
>> it calls both flushPendingEvents() and the realSync().
>>
>> --Semyon
>>>
>>> The javadoc for Robot.waitForIdle() says:
>>> "Waits until all events currently on the event queue have been 
>>> processed."
>>>
>>> realSync() does use syncNativeQueue()
>>> so its javadoc states:
>>>
>>>      * Forces toolkit to synchronize with the native windowing
>>>      * sub-system, flushing all pending work and waiting for all the
>>>      * events to be processed.  This method guarantees that after
>>>      * return no additional Java events will be generated, unless
>>>      * cause by user."
>>>
>>> Thanks
>>> alexp
>>>
>>>>
>>>> --Semyon
>>>>>
>>>>> So I don't recommend using it at all.
>>>>>
>>>>> The most comprehensive approach is to use the realSync() method 
>>>>> from the SunToolkit
>>>>> (as many of the Swing jtregs tests do).
>>>>>
>>>>> This method is definitely more stable than waitForIdle and it 
>>>>> works well for most of the tests.
>>>>>
>>>>> Thanks
>>>>> alexp
>>>>>
>>>>> On 5/11/2016 18:38, Semyon Sadetsky wrote:
>>>>>>
>>>>>>
>>>>>> On 5/11/2016 5:49 PM, Anton Litvinov wrote:
>>>>>>> Hi Semyon,
>>>>>>>
>>>>>>> Thank you for this information. Current version of the 
>>>>>>> regression test using the shell script is tested, cross platform 
>>>>>>> and does not contain any code specific to Windows platform. This 
>>>>>>> shell script is a copy of many other stable regression tests 
>>>>>>> existing in "jdk9/jdk/test" directory and is different from them 
>>>>>>> only in its commented test header (jtreg options like: @summary, 
>>>>>>> @author) and 7 code lines between "###############  YOUR TEST 
>>>>>>> CODE HERE!!!!!!! #############" and "###############  END YOUR 
>>>>>>> TEST CODE !!!!! ############".
>>>>>> I guess other platforms don't remove trailing spaces in the path.
>>>>>>>
>>>>>>> What is so bad in usage of the well established approach based 
>>>>>>> on shell script?
>>>>>> Nothing really bad. It just a bit more complex then it might be.
>>>>>>
>>>>>> You use Thread.sleep and critical sections in the test. Why not 
>>>>>> to use AWT robot's waitForIdle() ?
>>>>>>
>>>>>> --Semyon
>>>>>>>
>>>>>>> Thank you,
>>>>>>> Anton
>>>>>>>
>>>>>>> On 5/11/2016 5:29 PM, Semyon Sadetsky wrote:
>>>>>>>> Hi Anton,
>>>>>>>>
>>>>>>>> In windows you may use "\\?\" prefix with absolute path of a 
>>>>>>>> new folder.
>>>>>>>> For example:
>>>>>>>> new File("\\\\?\\C:\\tmp\\test2 ").mkdir();
>>>>>>>>
>>>>>>>> --Semyon
>>>>>>>>
>>>>>>>> On 5/11/2016 4:47 PM, Anton Litvinov wrote:
>>>>>>>>> Hello Sergey,
>>>>>>>>>
>>>>>>>>> Thank you for review of this fix. No, unfortunately, on MS 
>>>>>>>>> Windows OS, if the method "java.io.File.mkdir()" is called on 
>>>>>>>>> "java.io.File" instance which contains trailing space 
>>>>>>>>> characters in the directory name, the corresponding directory 
>>>>>>>>> is created but without trailing space characters in its name 
>>>>>>>>> in file system.
>>>>>>>>>
>>>>>>>>> The method "java.nio.file.Files.createDirectory(Path dir, 
>>>>>>>>> FileAttribute<?>... attrs)" cannot be used for this purpose 
>>>>>>>>> also, because "java.nio.file.Path" cannot be constructed for 
>>>>>>>>> the directory name ending with spaces and 
>>>>>>>>> "java.io.File.toPath()" throws the exception 
>>>>>>>>> "java.nio.file.InvalidPathException: Trailing char < > at 
>>>>>>>>> index N: <DIRECTORY_PATH>".
>>>>>>>>>
>>>>>>>>> It is possible to create a directory with such a name from the 
>>>>>>>>> shell script on MS Windows OS, therefore I decided to use the 
>>>>>>>>> shell script for this regression test.
>>>>>>>>>
>>>>>>>>> Thank you,
>>>>>>>>> Anton
>>>>>>>>>
>>>>>>>>> On 5/11/2016 4:16 PM, Sergey Bylokhov wrote:
>>>>>>>>>> Hi, Anton.
>>>>>>>>>> Probably the test can create the folder w/o the shell script?
>>>>>>>>>>
>>>>>>>>>> On 11.05.16 15:14, Anton Litvinov wrote:
>>>>>>>>>>> The bug consists in the fact that the method
>>>>>>>>>>> "JFileChooser.getSelectedFile()" returns "java.io.File" 
>>>>>>>>>>> object which
>>>>>>>>>>> does not contain trailing spaces in the directory name, in 
>>>>>>>>>>> spite of the
>>>>>>>>>>> fact that the corresponding directory in the file system has 
>>>>>>>>>>> trailing
>>>>>>>>>>> spaces in its name. The fix deletes the code in the method
>>>>>>>>>>> "javax.swing.plaf.basic.BasicFileChooserUI.ApproveSelectionAction.actionPerformed" 
>>>>>>>>>>>
>>>>>>>>>>> which deliberately modifies the selected directory string 
>>>>>>>>>>> name by
>>>>>>>>>>> removing trailing spaces from it.
>>>>>>>>>>>
>>>>>>>>>>> All automatic regression tests from open and closed sets 
>>>>>>>>>>> located in
>>>>>>>>>>> "javax/swing/JFileChooser" directories were run on MS 
>>>>>>>>>>> Windows 7 OS
>>>>>>>>>>> during verification of the fix.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20160516/4c1ff38f/attachment.html>