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

[Jonathan Lu]

Hi Pavel,

see my comments below,

On 04/13/2012 06:39 PM, Pavel Porvatov wrote:
> 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?

I do not think so, since the problem here is that the repaining work is 
not done after hiding a visible component, but the fields' status of 
this hidden component have been correct, so has its parent. And 
isShowing() 's implementation is just checking those fields in similar 
way as isVisible() except for additional checks for the parent, whose 
status also cannot reflect the visual picture from user's eyes. So in my 
opinion, isShowing does not help with this case better than isVisible 
but introduces more efforts of checking and branching. Pls fix me if 
anything wrong with my understanding.

BTW, there's a stupid bug in previous patch caused by my local testing 
cache was not cleaned up completely, terribly sorry for that and here's 
the updated patch. http://cr.openjdk.java.net/~luchsh/7154030_6/

>>
>>>
>>>
>>>
>>>>> 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));
updated in new patch.
>
>> 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?
done in the new patch.
>
> Regards, Pavel

Regards
- Jonathan
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20120420/0cbeb30c/attachment.html>