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
