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

[Charles Lee]

Hi Jonathan,

The patch has been committed @

Changeset: 340cda7e1430
Author:    luchsh
Date:      2012-04-26 12:39 +0800
URL:http://hg.openjdk.java.net/jdk8/awt/jdk/rev/340cda7e1430

7154030: java.awt.Component.hide() does not repaint parent component
Reviewed-by: rupashka


Please verify it.

Hi Pavel,

Thank you for reviewing it.


On 04/24/2012 03:50 PM, Jonathan Lu wrote:
> Hi Pavel,
>
> Thanks for the explanation, I've updated the patch accordingly, could 
> you please take a look?
> http://cr.openjdk.java.net/~luchsh/7154030_7/
>
> Best regards
> -Jonathan
>
> On 04/22/2012 12:23 AM, Pavel Porvatov wrote:
>> Hi Jonathan,
>>> 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.
>> The java.awt.Component#isVisible method just returns value of the 
>> "visible" field, but isShowing reflects visible component on screen 
>> or not. E.g. almost all created components returns isVisible true, 
>> even if they were not showed on screen...
>>
>> Regards, Pavel
>>>
>>> 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
>>
>
>


-- 
Yours Charles

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