<Swing Dev> Request review for 7129742 : Unable to view focus in Non-Editable TextArea
Pavel Porvatov
pavel.porvatov at oracle.com
Sun Apr 15 09:31:06 UTC 2012
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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20120415/242b9169/attachment.html>
More information about the swing-dev
mailing list