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

[Pavel Porvatov]

Hi Jonathan,
> Hi Pavel,
>
> Here's another updated patch.
> http://cr.openjdk.java.net/~luchsh/7154030_5/
>
> My comments are inlined.
See below
>
> 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.
Ok. The next question: I think we should use in "if (isVisible())" 
isShowing instead of isVisible. What do you think about that?
>
>>
>>
>>
>>>> 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.
Why can't we use something like following:
                 button.setOpaque(false);
                 button.setBackground(new Color(255, 0, 0, 128));

> 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.
Looks good. Could you please remove unnecessary "volatile" modifiers?

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