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.
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
