<Swing Dev> [9] Review request for 8041694: JFileChooser removes trailing spaces in the selected directory name
Semyon Sadetsky
semyon.sadetsky at oracle.com
Mon May 16 14:57:17 UTC 2016
+1
--Semyon
On 5/16/2016 5:41 PM, 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.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20160516/3d47b5e2/attachment.html>
More information about the swing-dev
mailing list