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

[Semyon Sadetsky]

On 5/17/2016 7:53 PM, Alexander Potochkin wrote:
> Hello Semyon
>
>> 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().
>
> Ups, you are right.
>
> I looked at the JDK8u repo where realSync() is not used by the awt.Robot
> and was under impression that in JDK9 we have the same code.
>
> It is good to know that Robot.waitForIdle() is fixed in JDK9!
> So it is definitely fine to use it here.
>
> On the other side - we need to change the test when backporting a fix 
> from JDK9 to JDK8/7.
>
> So when the backports are required it may still make sense to use the 
> plain realSync()
> to keep the backports identical to avoid the additional reviews.
I did not mean to use waitForIdle() strictly. Sorry if that added you an 
extra work to backport the test to 8. Actually, I was only concerned 
about too many threads and synchronization between them in the test, 
while usually main+EDT threads with waitForIdle() or realSync() are 
enough for such scenarios. I have no objections to use the realSync() + 
delay.

--Semyon

>
> Thanks!
> alexp
>
>>
>> --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.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>