Hi Pavel,
Modified and tested on windows and linux.
webrev at http://cr.openjdk.java.net/~zhouyx/7129742/webrev.07/
On Sun, Apr 15, 2012 at 5:31 PM, Pavel Porvatov
<pavel.porva...@oracle.com>wrote:
> Hi Sean,
>
> Hi Pavel,
>
> I modified the testcase according to your comments. The webrev is
> http://cr.openjdk.java.net/~zhouyx/7129742/webrev.06/ . Please take a
> look again.
>
> And now when fastreturn is true the test doesn't stop.
>
> Regards, Pavel
>
>
> On Thu, Apr 12, 2012 at 10:24 PM, Pavel Porvatov <
> pavel.porva...@oracle.com> wrote:
>
>> Hi Sean,
>>
>> The fix looks good, but I have several comments about the test:
>> 1. You shouldn't use Swing components on non-EDT threads, so
>> frame.dispose() should be done on the EDT
>>
> I made a really stupid mistake... When I was checking the testcase and
> found frame.dispose() in main method, I added a volatile to the frame
> variable...
>
>
>> 2. "These exceptions mean the implementation of XTextAreaPeer is
>> changed" - I think is XTextAreaPeer is changed, then the test should be
>> fixed as well or removed (if the test become inapplicable. Therefore in
>> that situation the test should fail but not skipped
>>
>> Regards, Pavel
>>
>>
>> Hi Pavel,
>>
>> As awt team has committed the related bug(
>> http://hg.openjdk.java.net/jdk8/awt/jdk/rev/96340349e35b), I prepared
>> the webrev for this
>> bug(7129742<http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7129742>)
>> again.
>>
>> The new webrev:
>> http://cr.openjdk.java.net/~zhouyx/7129742/webrev.05/ .
>>
>> The sunbug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7129742
>> Previous discussion:
>> http://mail.openjdk.java.net/pipermail/swing-dev/2012-March/001923.html
>>
>> Please take a look once more, thank you.
>>
>> On Thu, Mar 15, 2012 at 8:57 PM, Pavel Porvatov <
>> pavel.porva...@oracle.com> wrote:
>>
>>> Hi Sean,
>>>
>>>
>>> Hi Pavel,
>>>
>>> Thanks for your comments.
>>>
>>> About the 1st question, I modified the testcase a little to
>>> demonstrate the problem, testcase is pasted at the end.
>>>
>>> If textArea.setEditable(true) is executed, the frame disposes, but
>>> application doesn't exit.
>>> If textArea.setEditable(false), the application exits as normal.
>>>
>>> java 1.7.0_01 and latest java8 code are tested. And TextField
>>> contains this problem as well. I'll add it to fix later.
>>> Shall I report a separate bug for it ?
>>>
>>> Yes. I think it will be better if you fix the described above problem
>>> as a separate bug. Note that it should be reviewed by the AWT team...
>>>
>>>
>>>
>>> About the 2nd question, I was driven by the comment "// TODO : fix
>>> this duplicate code". Is there any strong reason to keep it ?
>>>
>>> I'm absolutely agree with removing duplicate code. My second question
>>> was about added by you "jtext.getCaret().setVisible(false)" in the
>>> XTextAreaPeer.java class. I wrote:
>>> 2. Why don't we need the same code in the XTextFieldPeer class?
>>>
>>> As I can see you've noticed that XTextFieldPeer should be fixed as well
>>> (in a separate fix with XTextAreaPeer)
>>>
>>> Regards, Pavel
>>>
>>>
>>>
>>> /////////////////////////////////// a testcase
>>> ////////////////////////////////////////////////////////////
>>> /*
>>> * Copyright (c) 2012 Oracle and/or its affiliates. All rights reserved.
>>> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>> *
>>> * This code is free software; you can redistribute it and/or modify it
>>> * under the terms of the GNU General Public License version 2 only, as
>>> * published by the Free Software Foundation.
>>> *
>>> * This code is distributed in the hope that it will be useful, but
>>> WITHOUT
>>> * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>> * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
>>> * version 2 for more details (a copy is included in the LICENSE file
>>> that
>>> * accompanied this code).
>>> *
>>> * You should have received a copy of the GNU General Public License
>>> version
>>> * 2 along with this work; if not, write to the Free Software Foundation,
>>> * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
>>> *
>>> * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065
>>> USA
>>> * or visit www.oracle.com if you need additional information or have
>>> any
>>> * questions.
>>> */
>>>
>>> /*
>>> * Portions Copyright (c) 2012 IBM Corporation
>>> */
>>>
>>> /* @test
>>> * @bug
>>> * @summary editable TextArea blocks gui app from dispose.
>>> * @author Sean Chou
>>> */
>>>
>>> import java.awt.FlowLayout;
>>> import java.awt.Frame;
>>> import java.awt.TextArea;
>>> import java.awt.Toolkit;
>>> import java.lang.reflect.Field;
>>>
>>> import javax.swing.JFrame;
>>> import javax.swing.JTextArea;
>>> import javax.swing.SwingUtilities;
>>> import javax.swing.text.DefaultCaret;
>>>
>>> import sun.awt.SunToolkit;
>>>
>>> public class TextAreaDisposeBug {
>>> public static volatile boolean passed = false;
>>>
>>> public static Frame frame = null;
>>> public static TextArea textArea = null;
>>>
>>> public static DefaultCaret caret = null;
>>> public static Class XTextAreaPeerClzz = null;
>>>
>>> public static void main(String[] args) throws Exception {
>>> SunToolkit toolkit = (SunToolkit) Toolkit.getDefaultToolkit();
>>> SwingUtilities.invokeAndWait(new Runnable() {
>>> @Override
>>> public void run() {
>>> frame = new JFrame("Test");
>>>
>>> textArea = new TextArea("editable textArea");
>>> textArea.setEditable(true);
>>> // textArea.setEditable(false);
>>>
>>> frame.setLayout(new FlowLayout());
>>> frame.add(textArea);
>>>
>>> frame.pack();
>>> frame.setVisible(true);
>>>
>>> }
>>> });
>>> toolkit.realSync();
>>>
>>> SwingUtilities.invokeAndWait(new Runnable() {
>>> @Override
>>> public void run() {
>>> frame.dispose();
>>> }
>>> });
>>> toolkit.realSync();
>>> }
>>>
>>> }
>>>
>>>
>>>
>>> //////////////////////////////////////////////////////////////////////////////////////////
>>> /*
>>> * Copyright (c) 2012 Oracle and/or its affiliates. All rights reserved.
>>> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>> *
>>> * This code is free software; you can redistribute it and/or modify it
>>> * under the terms of the GNU General Public License version 2 only, as
>>> * published by the Free Software Foundation.
>>> *
>>> * This code is distributed in the hope that it will be useful, but
>>> WITHOUT
>>> * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>> * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
>>> * version 2 for more details (a copy is included in the LICENSE file
>>> that
>>> * accompanied this code).
>>> *
>>> * You should have received a copy of the GNU General Public License
>>> version
>>> * 2 along with this work; if not, write to the Free Software Foundation,
>>> * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
>>> *
>>> * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065
>>> USA
>>> * or visit www.oracle.com if you need additional information or have
>>> any
>>> * questions.
>>> */
>>>
>>> /*
>>> * Portions Copyright (c) 2012 IBM Corporation
>>> */
>>>
>>>
>>> /* @test
>>> * @bug
>>> * @summary editable TextField blocks gui app from dispose.
>>> * @author Sean Chou
>>> */
>>>
>>> import java.awt.FlowLayout;
>>> import java.awt.Frame;
>>> import java.awt.TextField;
>>> import java.awt.Toolkit;
>>> import java.lang.reflect.Field;
>>>
>>> import javax.swing.JFrame;
>>> import javax.swing.JTextArea;
>>> import javax.swing.SwingUtilities;
>>> import javax.swing.text.DefaultCaret;
>>>
>>> import sun.awt.SunToolkit;
>>>
>>> public class TextFieldDisposeBug {
>>> public static volatile boolean passed = false;
>>>
>>> public static Frame frame = null;
>>> public static TextField textField = null;
>>>
>>> public static DefaultCaret caret = null;
>>> public static Class XTextAreaPeerClzz = null;
>>>
>>> public static void main(String[] args) throws Exception {
>>> SunToolkit toolkit = (SunToolkit) Toolkit.getDefaultToolkit();
>>> SwingUtilities.invokeAndWait(new Runnable() {
>>> @Override
>>> public void run() {
>>> frame = new JFrame("Test");
>>>
>>> textField = new TextField("editable textArea");
>>> // textField.setEditable(true);
>>> textField.setEditable(false);
>>>
>>> frame.setLayout(new FlowLayout());
>>> frame.add(textField);
>>>
>>> frame.pack();
>>> frame.setVisible(true);
>>>
>>> }
>>> });
>>> toolkit.realSync();
>>>
>>> SwingUtilities.invokeAndWait(new Runnable() {
>>> @Override
>>> public void run() {
>>> frame.dispose();
>>> }
>>> });
>>> toolkit.realSync();
>>> }
>>>
>>> }
>>>
>>>
>>> On Tue, Mar 13, 2012 at 9:49 PM, Pavel Porvatov <
>>> pavel.porva...@oracle.com> wrote:
>>>
>>>> Hi Sean,
>>>>
>>>> I have a couple questions about the following code in the
>>>> XTextAreaPeer.java class
>>>> + // visible caret has a timer thread, which must be stopped
>>>> + jtext.getCaret().setVisible(false);
>>>>
>>>> 1. Why this code wasn't needed before your fix, when caret was visible
>>>> for enabled text area?
>>>> 2. Why don't we need the same code in the XTextFieldPeer class?
>>>>
>>>> About the bug7129742 test:
>>>> Because of the passed field is used from different threads you must use
>>>> synchronization or mark the field as a volatile.
>>>>
>>>> Regards, Pavel
>>>>
>>>>
>>>> Hi Alexander,
>>>>
>>>> XTextFieldPeer and XTextAreaPeer have a same inner
>>>> class XAWTCaret, and in XTextAreaPeer there is also a comment:
>>>> "// TODO : fix this duplicate code " before XAWTCaret . So I removed
>>>> the XAWTCaret in XTextFieldPeer and changed the
>>>> XAWTCaret into a static class, so XTextFieldPeer can
>>>> use XAWTCaret from XTextAreaPeer .
>>>>
>>>> As XAWTCaret is only used in the following
>>>> method in both XTextAreaPeer and XTextFieldPeer .
>>>> protected Caret createCaret() {
>>>> return new XAWTCaret();
>>>> }
>>>> I think this modification should not bring side effect.
>>>>
>>>> On Mon, Mar 12, 2012 at 1:27 AM, Alexander Potochkin <
>>>> alexander.potoch...@oracle.com> wrote:
>>>>
>>>>> Hello Sean
>>>>>
>>>>> Could you give more details about your changes in XTextFieldPeer?
>>>>>
>>>>> Thanks
>>>>> alexp
>>>>>
>>>>> Hi all,
>>>>>>
>>>>>> I updated the patch to as suggested and simplified the testcase .
>>>>>> Would anyone like to take a look again ? Thanks.
>>>>>>
>>>>>> The webrev is at :
>>>>>> http://cr.openjdk.java.net/~zhouyx/7129742/webrev.04/ <
>>>>>> http://cr.openjdk.java.net/%7Ezhouyx/7129742/webrev.04/>
>>>>>>
>>>>>>
>>>>>> Previous discussion at :
>>>>>>
>>>>>> http://mail.openjdk.java.net/pipermail/swing-dev/2012-February/001913.html
>>>>>>
>>>>>> --
>>>>>> Best Regards,
>>>>>> Sean Chou
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>> --
>>>> Best Regards,
>>>> Sean Chou
>>>>
>>>>
>>>>
>>>
>>>
>>> --
>>> Best Regards,
>>> Sean Chou
>>>
>>>
>>>
>>
>>
>> --
>> Best Regards,
>> Sean Chou
>>
>>
>>
>
>
> --
> Best Regards,
> Sean Chou
>
>
>
--
Best Regards,
Sean Chou
