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

Mon May 16 14:46:58 UTC 2016

Looks fine.

On 16.05.16 17:41, Anton Litvinov wrote:
> 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.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

-- 
Best regards, Sergey.