Review request for 7154030: java.awt.Component.hide() does not repaint parent container (original) (raw)

Pavel Porvatov pavel.porvatov at oracle.com
Wed Apr 11 13:40:42 UTC 2012

Hi Jonathan,

Hi Pavel,

Here's the update patch, http://cr.openjdk.java.net/~luchsh/71540304/ Could you please explain one change in the JComponent.java:

I think super.hide() should be invoked always. You must also add a javadoc to the new hide method (see {@inheritDoc} for details)

See also my comments below

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/71540302/ 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. Ok 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. The test should be corrected:

  1. All Swing components must be accessed from the EDT thread
  2. What is the reason to introduce the MyButton class? I think the test can be simplified, if you will use the Util#compareBufferedImages (see test\javax\swing\regtesthelpers\Util.java) method. Just take screen-shots between show/hide and compare them
  3. Thread.sleep(milisecond) should be replaced by the SunToolkit#realSync method (many existing tests uses it)

Regards, Pavel -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20120411/77b1c774/attachment.html>

More information about the swing-dev mailing list