<Swing Dev> Review request for 7154030: java.awt.Component.hide() does not repaint parent container
Pavel Porvatov
pavel.porvatov at oracle.com
Tue Mar 27 15:58:49 UTC 2012
Hi Jonathan,
> Hello Pavel,
>
> Here's the updated patch and automatic test for bug 7154030, could you
> please take another look?
> http://cr.openjdk.java.net/~luchsh/7154030_2/
I have several comments:
1. What about the following comment from Artem:
"Even if we accept the change in JComponent.hide(), we should then
override show() as well (lightweight component may be non-opaque, so we
should repaint from its parent)"
?
2. Could you please clarify your changes in setVisible method?
As I see in comments
// Some (all should) LayoutManagers do not consider components
// that are not visible. As such we need to revalidate when the
// visible bit changes.
revalidate();
but now this code is invoked only for setVisible(true)
3. Could you please follow our code conventions? (see
http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-141388.html#475)
4. Your test is not automatic one. I think you could use
java.awt.Robot#createScreenCapture and analyze result of hide method.
Regards, Pavel
>
> Thanks and best regards
> - Jonathan Lu
>
> On 03/26/2012 09:38 PM, Pavel Porvatov wrote:
>> Hi Jonathan,
>>> Hi Pavel,
>>>
>>> Thanks for your explanation.
>>>
>>> But this bug affects almost all Swing components, hide()'s presence
>>> also helps to maintain backward compatibility, so is it possible to
>>> put a fix in JComponent to help all the potential affected
>>> applications to work correctly?
>> Of course that's possible. Do you have final version of the fix?
>> Please don't forget write an automatic test.
>>> if not, is it there any sunset plan for these deprecated APIs?
>> I don't now such plans.
>>
>> Regards, Pavel
>>
>> P.S. I removed <AWT Dev>, it seems only Swing will be affected in
>> this fix
>>> Thanks and best regards!
>>>
>>> - Jonathan
>>>
>>> 2012/3/20 Pavel Porvatov <pavel.porvatov at oracle.com
>>> <mailto:pavel.porvatov at oracle.com>>
>>>
>>> Hi Jonathan,
>>>> Hi Artem,
>>>>
>>>> 2012/3/20 Artem Ananiev <artem.ananiev at oracle.com
>>>> <mailto:artem.ananiev at oracle.com>>
>>>>
>>>> Hi, Jonathan,
>>>>
>>>> I'm adding swing-dev to CC as we now consider changing
>>>> Swing code.
>>>>
>>>> What you propose sounds technically reasonable, but I don't
>>>> think it is worth doing anyway as show() and hide() have
>>>> been deprecated for years now.
>>>>
>>>>
>>>> Although show() and hide() have been deprecated for years, in
>>>> my opinion supporting these APIs will still benefit many
>>>> applications and convince users that Java still has got strong
>>>> backward compatibility :D. Any ideas from Swing group?
>>> I don't see why the words "backward compatibility" are here.
>>> There is a bug in deprecated methods "show" and "hide" (I've
>>> checked that jdk5 has the same problem), and that's one
>>> additional reason to use setVisible(). I agree with Artem that
>>> fixing deprecated API is not a high priority task (but we should
>>> keep backward compatibility, of course). I also think, that "to
>>> leave all as is" is a good decision for the described problem
>>>
>>> Regards, Pavel
>>>
>>>>
>>>>
>>>> Even if we accept the change in JComponent.hide(), we
>>>> should then override show() as well (lightweight component
>>>> may be non-opaque, so we should repaint from its parent),
>>>> so there will be code duplication. This is one more reason
>>>> to leave all as is.
>>>>
>>>>
>>>> Yes, I noticed that code duplication too and am trying to make
>>>> a more compact patch for this problem.
>>>>
>>>> This is my personal opinion, I'm not a Swing expert,
>>>> though. Let anyone from the Swing group comment.
>>>>
>>>> Thanks,
>>>>
>>>> Artem
>>>>
>>>> On 3/20/2012 12:28 PM, Jonathan Lu wrote:
>>>>
>>>> Hi Artem,
>>>>
>>>> Thanks for your time.
>>>>
>>>> 2012/3/19 Artem Ananiev <artem.ananiev at oracle.com
>>>> <mailto:artem.ananiev at oracle.com>
>>>> <mailto:artem.ananiev at oracle.com
>>>> <mailto:artem.ananiev at oracle.com>>>
>>>>
>>>> Hi, Jonathan,
>>>>
>>>> given the code in java.awt.Component, your statement
>>>> about
>>>> difference between hide() and setVisible(false)
>>>> looks pretty strange
>>>> to me. Indeed, here is the implementation:
>>>>
>>>>
>>>>
>>>> public void show(boolean b) {
>>>> if (b) {
>>>> show();
>>>> } else {
>>>> hide();
>>>> }
>>>> }
>>>>
>>>> and
>>>>
>>>> public void setVisible(boolean b) {
>>>> show(b);
>>>> }
>>>>
>>>> In JComponent the latter method is overridden and
>>>> adds exactly what
>>>> you propose: parent.repaint(). This addition makes
>>>> sense for
>>>> lightweight components (e.g. Swing), but heavyweight
>>>> AWT components
>>>> shouldn't require this: repaint request is sent from
>>>> the native system.
>>>>
>>>>
>>>> Yes, lightweight and heavyweight components differ in
>>>> painting. The
>>>> original test case only works for the conditions of
>>>> lightweight
>>>> components, with another test case for heavyweight
>>>> components, I found
>>>> that the problem could not be reproduced on AWT any
>>>> more. I think the
>>>> change is only applicable for Swing components, so how
>>>> about repaint in
>>>> JComponent.hide() like this?
>>>>
>>>> diff -r cdbb33303ea3
>>>> src/share/classes/javax/swing/JComponent.java
>>>> --- a/src/share/classes/javax/swing/JComponent.java
>>>> Wed Mar 14
>>>> 13:50:37 2012 -0700 <tel:2012%20-0700> <tel:2012%20-0700>
>>>> +++ b/src/share/classes/javax/swing/JComponent.java
>>>> Tue Mar 20
>>>> 16:24:09 2012 +0800
>>>> @@ -5237,6 +5237,16 @@
>>>> }
>>>> }
>>>>
>>>> + public void hide() {
>>>> + super.hide();
>>>> + Container parent = getParent();
>>>> + if (parent != null) {
>>>> + Rectangle r = getBounds();
>>>> + parent.repaint(r.x, r.y, r.width, r.height);
>>>> + parent.invalidate();
>>>> + }
>>>> + }
>>>> +
>>>> /**
>>>> * Returns whether or not the region of the
>>>> specified component is
>>>> * obscured by a sibling.
>>>>
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> Artem
>>>>
>>>>
>>>> On 3/15/2012 12:24 PM, Jonathan Lu wrote:
>>>>
>>>> Hi awt-dev,
>>>>
>>>> java.awt.Component.hide() was declared as
>>>> deprecation and
>>>> replaced by
>>>> setVisible(boolean), but in my tests, it does
>>>> not works in the
>>>> same way
>>>> as setVisible(false). The reason of this failure
>>>> is that
>>>> java.awt.Component.hide() does not repaint the
>>>> special area it
>>>> used to
>>>> taken of parent container. Although this is
>>>> deprecated method,
>>>> it may
>>>> still valuable for customers due to
>>>> compatibility reason. Bug
>>>> 7154030
>>>> created for this issue.
>>>>
>>>> Here's a simple test case to demonstrate this
>>>> problem.
>>>>
>>>> /*
>>>> * 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> <http://www.oracle.com> if you need
>>>> additional information or have any
>>>> * questions.
>>>> */
>>>>
>>>> /*
>>>> * Portions Copyright (c) 2012 IBM Corporation
>>>> */
>>>>
>>>> import javax.swing.*;
>>>>
>>>> /* @test 1.1 2012/03/15
>>>> @bug 7154030
>>>> @run main/manual ComponentHideShowTest.html */
>>>>
>>>> @SuppressWarnings("serial")
>>>> public class ComponetHideShowTest extends JFrame {
>>>> JInternalFrame internalFrame;
>>>> JButton btn;
>>>> JDesktopPane desktop;
>>>>
>>>> ComponetHideShowTest(String name) {
>>>> super(name);
>>>> desktop = new JDesktopPane();
>>>> setContentPane(desktop);
>>>>
>>>> setSize(600, 400);
>>>> setVisible(true);
>>>>
>>>> internalFrame = new
>>>> JInternalFrame("Test Internal Frame");
>>>> internalFrame.setSize(100, 100);
>>>> internalFrame.setLocation(10, 10);
>>>> internalFrame.setVisible(true)__;
>>>> desktop.add(internalFrame);
>>>>
>>>> btn = new JButton("OK");
>>>> btn.setSize(100, 50);
>>>> btn.setLocation( 300, 300);
>>>> btn.setVisible(true);
>>>> desktop.add(btn);
>>>>
>>>>
>>>> setDefaultCloseOperation(__JFrame.EXIT_ON_CLOSE);
>>>> }
>>>>
>>>> @SuppressWarnings("__deprecation")
>>>> public void runTest() throws Exception {
>>>> Object[] options = { "Yes, I saw it",
>>>> "No, I did not
>>>> see it!" };
>>>>
>>>> int ret =
>>>> JOptionPane.showOptionDialog(__this,
>>>> "Do you see the internal window?",
>>>> "InternalFrmaeHideTest",
>>>> JOptionPane.YES_NO_OPTION,
>>>> JOptionPane.QUESTION_MESSAGE, null,
>>>> options, options[1]);
>>>>
>>>> if (ret == 1 || ret ==
>>>> JOptionPane.CLOSED_OPTION) {
>>>> throw new Exception("Failed to
>>>> display internal
>>>> window");
>>>> }
>>>>
>>>> internalFrame.hide();
>>>> btn.hide();
>>>>
>>>> ret = JOptionPane.showOptionDialog(__this,
>>>> "Do you see the internal window?",
>>>> "InternalFrmaeHideTest",
>>>> JOptionPane.YES_NO_OPTION,
>>>> JOptionPane.QUESTION_MESSAGE, null,
>>>> options, options[1]);
>>>>
>>>> if (ret == 0 || ret ==
>>>> JOptionPane.CLOSED_OPTION) {
>>>> throw new Exception("Failed to hide
>>>> internal window");
>>>> }
>>>>
>>>> internalFrame.show();
>>>> btn.show();
>>>>
>>>> ret = JOptionPane.showOptionDialog(__this,
>>>> "Do you see the internal window?",
>>>> "InternalFrmaeHideTest",
>>>> JOptionPane.YES_NO_OPTION,
>>>> JOptionPane.QUESTION_MESSAGE, null,
>>>> options, options[1]);
>>>>
>>>> if (ret == 1 || ret ==
>>>> JOptionPane.CLOSED_OPTION) {
>>>> throw new Exception("Failed to hide
>>>> internal window");
>>>> }
>>>> }
>>>>
>>>> public static void main(String[] args)
>>>> throws Exception {
>>>> ComponetHideShowTest test = null;
>>>> test = new
>>>> ComponetHideShowTest("__InternalFrameHideTest");
>>>> test.runTest();
>>>> }
>>>> }
>>>>
>>>> And here's the patch
>>>> http://cr.openjdk.java.net/~__littlee/7154030/
>>>> <http://cr.openjdk.java.net/%7E__littlee/7154030/>
>>>> <http://cr.openjdk.java.net/%7Elittlee/7154030/>
>>>>
>>>> Can anybody please help to take a look?
>>>>
>>>> Cheers!
>>>> - Jonathan
>>>>
>>>>
>>>> Best regards!
>>>> - Jonathan
>>>>
>>>>
>>>> Thanks a lot !
>>>>
>>>> - Jonathan
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20120327/37ccd23e/attachment.html>
More information about the swing-dev
mailing list