<Swing Dev> Review request for 7154030: java.awt.Component.hide() does not repaint parent container

[Jonathan Lu]

Hi Pavel,

Here's another updated patch.
http://cr.openjdk.java.net/~luchsh/7154030_5/

My comments are inlined.

On 04/11/2012 09:40 PM, Pavel Porvatov wrote:
> Hi Jonathan,
>> Hi Pavel,
>>
>> Here's the update patch,
>> http://cr.openjdk.java.net/~luchsh/7154030_4/
>>
> Could you please explain one change in the JComponent.java:
>
> +    public void hide() {
> +        if (isVisible()) {
> +            super.hide();
>
> I think super.hide() should be invoked always. You must also add a 
> javadoc to the new hide method (see {@inheritDoc} for details)

Yes, the old patch may cause Component.isPacked not to be set correctly, 
so moved super.hide() up.

>
> 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/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.
> 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
Updated in the new test.
> 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

The reason of introducing MyButton class is to test the show/hide of 
non-opaque component.
I've changed pixel comparison to Util#compareBufferedImages approach.

> 3. Thread.sleep(milisecond) should be replaced by the 
> SunToolkit#realSync method (many existing tests uses it)
>
Updated in new test.

> Regards, Pavel

Thanks and best regards
- Jonathan

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20120412/1696afb9/attachment.html>