<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Hi Pavel,<br>
<br>
see my comments below,<br>
<br>
On 04/13/2012 06:39 PM, Pavel Porvatov wrote:
<blockquote cite="mid:4F880273.3080704@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
Hi Jonathan,
<blockquote cite="mid:4F86B92D.9080900@linux.vnet.ibm.com"
type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
Hi Pavel,<br>
<br>
Here's another updated patch.<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eluchsh/7154030_5/">http://cr.openjdk.java.net/~luchsh/7154030_5/</a><br>
<br>
My comments are inlined.<br>
</blockquote>
See below<br>
<blockquote cite="mid:4F86B92D.9080900@linux.vnet.ibm.com"
type="cite"> <br>
On 04/11/2012 09:40 PM, Pavel Porvatov wrote:
<blockquote cite="mid:4F8589DA.5000600@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
Hi Jonathan,
<blockquote cite="mid:4F7EB5B6.6090009@linux.vnet.ibm.com"
type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
Hi Pavel,<br>
<br>
Here's the update patch, <br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eluchsh/7154030_4/">http://cr.openjdk.java.net/~luchsh/7154030_4/</a><br>
<br>
</blockquote>
Could you please explain one change in the JComponent.java:<br>
<br>
+ public void hide() {<br>
+ if (isVisible()) {<br>
+ super.hide();<br>
<br>
I think super.hide() should be invoked always. You must also
add a javadoc to the new hide method (see {@inheritDoc} for
details)<br>
</blockquote>
<br>
Yes, the old patch may cause Component.isPacked not to be set
correctly, so moved super.hide() up.<br>
</blockquote>
Ok. The next question: I think we should use in "if (isVisible())"
isShowing instead of isVisible. What do you think about that?<br>
</blockquote>
<br>
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.<br>
<br>
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. <a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eluchsh/7154030_6/">http://cr.openjdk.java.net/~luchsh/7154030_6/</a><br>
<br>
<blockquote cite="mid:4F880273.3080704@oracle.com" type="cite">
<blockquote cite="mid:4F86B92D.9080900@linux.vnet.ibm.com"
type="cite"> <br>
<blockquote cite="mid:4F8589DA.5000600@oracle.com" type="cite">
<br>
<br>
<br>
<blockquote cite="mid:4F7EB5B6.6090009@linux.vnet.ibm.com"
type="cite">
<blockquote cite="mid:4F71E3B9.9090103@oracle.com"
type="cite"> 3. Could you please follow our code
conventions? (see <a moz-do-not-send="true"
href="http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-141388.html#475">http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-141388.html#475</a>)<br>
<br>
</blockquote>
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.<br>
<br>
<blockquote cite="mid:4F71E3B9.9090103@oracle.com"
type="cite"> 4. Your test is not automatic one. I think
you could use java.awt.Robot#createScreenCapture and
analyze result of hide method.<br>
</blockquote>
<br>
See the link, it should be automatic now.<br>
</blockquote>
The test should be corrected:<br>
1. All Swing components must be accessed from the EDT thread<br>
</blockquote>
Updated in the new test.<br>
<blockquote cite="mid:4F8589DA.5000600@oracle.com" type="cite">
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<br>
</blockquote>
<br>
The reason of introducing MyButton class is to test the
show/hide of non-opaque component.<br>
</blockquote>
Why can't we use something like following:<br>
button.setOpaque(false);<br>
button.setBackground(new Color(255, 0, 0, 128));<br>
</blockquote>
updated in new patch.<br>
<blockquote cite="mid:4F880273.3080704@oracle.com" type="cite"> <br>
<blockquote cite="mid:4F86B92D.9080900@linux.vnet.ibm.com"
type="cite"> I've changed pixel comparison to
Util#compareBufferedImages approach.<br>
<br>
<blockquote cite="mid:4F8589DA.5000600@oracle.com" type="cite">
3. Thread.sleep(milisecond) should be replaced by the
SunToolkit#realSync method (many existing tests uses it)<br>
<br>
</blockquote>
Updated in new test.<br>
</blockquote>
Looks good. Could you please remove unnecessary "volatile"
modifiers?<br>
</blockquote>
done in the new patch.<br>
<blockquote cite="mid:4F880273.3080704@oracle.com" type="cite"> <br>
Regards, Pavel<br>
</blockquote>
<br>
Regards<br>
- Jonathan<br>
</body>
</html>