<Swing Dev> Request review for 7129742 : Unable to view focus in Non-Editable TextArea
Sean Chou
zhouyx at linux.vnet.ibm.com
Tue Apr 17 08:22:49 UTC 2012
Hi Pavel,
Thank you very much.
On Mon, Apr 16, 2012 at 8:10 PM, Pavel Porvatov
<pavel.porvatov at oracle.com>wrote:
> Hi Sean
>
>
> The fix looks good for me
>
> Regards, Pavel
>
>
> 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.porvatov at 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.porvatov at 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.porvatov at 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.porvatov at 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.Potochkin at 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
>
>
>
--
Best Regards,
Sean Chou
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20120417/85e540e1/attachment.html>
More information about the swing-dev
mailing list