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

Fri May 13 18:13:56 UTC 2016

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/20160513/af7e443e/attachment.html>