<Swing Dev> Request review for 7129742 : Unable to view focus in Non-Editable TextArea
Pavel Porvatov
pavel.porvatov at oracle.com
Mon Apr 16 12:10:18 UTC 2012
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/
> <http://cr.openjdk.java.net/%7Ezhouyx/7129742/webrev.07/>
>
>
>
> On Sun, Apr 15, 2012 at 5:31 PM, Pavel Porvatov
> <pavel.porvatov at oracle.com <mailto: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/
>> <http://cr.openjdk.java.net/%7Ezhouyx/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 <mailto: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/
>>> <http://cr.openjdk.java.net/%7Ezhouyx/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
>>> <mailto: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 <http://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 <http://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
>>>> <mailto: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
>>>>> <mailto: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/>
>>>>> <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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20120416/1575cc6c/attachment.html>
More information about the swing-dev
mailing list