<Swing Dev> Review request for 7154030: java.awt.Component.hide() does not repaint parent container
Jonathan Lu
luchsh at linux.vnet.ibm.com
Fri Apr 6 09:21:58 UTC 2012
Hi Pavel,
Here's the update patch,
http://cr.openjdk.java.net/~luchsh/7154030_4/
My comments are inlined.
On 03/27/2012 11:58 PM, Pavel Porvatov wrote:
> 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)"
> ?
>
I've updated my test case by including non-opaque components, but I do
no see a need for overriding show(), is there anything incorrect with
the updated testcase or my understanding?
> 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)
>
For the setVisible(false) case, the repainting and revalidating
operations will be done in new method JComponent.hide(), so this change
is just to reduce some duplicated actions.
> 3. Could you please follow our code conventions? (see
> http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-141388.html#475)
>
Sorry for this problem, I was trying to keeping aligned with the
original style of JComponent.java, which I later realized to be
inappropriate. In the updated patch, code has been well formatted.
> 4. Your test is not automatic one. I think you could use
> java.awt.Robot#createScreenCapture and analyze result of hide method.
See the link, it should be automatic now.
>
> 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/20120406/54c13c62/attachment.html>
More information about the swing-dev
mailing list