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

[Jonathan Lu]

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
>


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