search

GoogleCodeArchive / piccolo2d

Automatically exported from code.google.com/p/piccolo2d
0 stars 0 forks source link

PSwing doesn't draw dynamic JComponent properly #163

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
What steps will reproduce the problem?

Run the attached example (TestPSwingDynamicComponent). This example shows 2
identical JPanels with light blue backgrounds and a line border. One panel
is drawn using pure Swing, the other using PSwing.  The JPanels contain 2
JLabels, which can be updated by typing into JTextFields and pressing a
JButton labeled Update.

What is the expected output? What do you see instead?

I would expect the 2 panels to look identical.  The PSwing panel frequently
looks wrong, with one or more of the following symptoms: ellipsis in JLabel
text, cropped rightmost character(s) of JLabel text, entire JPanel scaled,
JLabels rendered in wrong location in JPanel.  Resizing the JFrame seems to
force a redraw, and sometime corrects the rendering.

Please use labels and text to provide additional information.

Also attached are 3 screenshots showing some examples of incorrect rendering.

This problem was first noticed in a PhET application that does something
similar to this example (updates JLabels in a JPanel).

I've spent a little time trying to debug this, but have unsuccessful so far.

Original issue reported on code.google.com by cmal...@pixelzoom.com on 24 Feb 2010 at 11:04

Attachments:

GoogleCodeExporter commented 9 years ago 
FYI... This is not a new problem.  It happens with our 1.2 snapshot (r390) and
1.3-rc3.  So it's the consensus of the PhET team that this issue is not a 
reason to
fail 1.3-rc3.  But if anyone else on the Piccolo team feels otherwise, we'll 
defer to
your decision.

Original comment by cmal...@pixelzoom.com on 25 Feb 2010 at 11:56

GoogleCodeExporter commented 9 years ago 
I confirm that this happens with 1.3-rc3.

If you add

TestPSwingDynamicComponent.java, line 64:
+ piccoloLabelPanel.invalidate();
+ piccoloLabelPanel.validate();
+ pswing.repaint();

then things usually right themselves the second time you hit the Update button. 
 So
there might be a workaround possible with 1.3.

If you are OK with pushing this off to 2.0 or 1.4 then so am I.  There isn't a 
"Known
issues" section of the release notes, perhaps there should be.

Original comment by heue...@gmail.com on 26 Feb 2010 at 5:26

GoogleCodeExporter commented 9 years ago 
On second thought, I might -1 the 1.3-rc3 because of Issue 165, so let's see if 
we
can get this fixed quickly.

Original comment by heue...@gmail.com on 26 Feb 2010 at 5:59

GoogleCodeExporter commented 9 years ago 
Fixed in r973

Original comment by allain.lalonde on 26 Feb 2010 at 9:33

GoogleCodeExporter commented 9 years ago 
Verified that r973 solves the problem, both in TestPSwingDynamicComponent and 
the
PhET application where this was first noticed.

Note that there were a couple of unrelated format changes in PSwing r973, 
probably
attributable to Eclipse.

Thanks once again Allain. HierarchyBoundsListener is a new one for me.

Should someone change their vote and fail 1.3-rc3?

Original comment by cmal...@pixelzoom.com on 26 Feb 2010 at 11:15

GoogleCodeExporter commented 9 years ago 
Marked as verified.

Original comment by cmal...@pixelzoom.com on 26 Feb 2010 at 11:19

GoogleCodeExporter commented 9 years ago 
There's still a problem here.  In TestPSwingDynamicComponent, change the type of
topLabel and bottomLabel to JCheckBox or JRadioButton.  You'll see the text of 
these
radio buttons rendered as ellipses.  See attached screenshot, ellipses.jpg.

Also attached is another test program, DebugBeakerControlsLayout.  I discovered 
this
problem in the content of another PhET application, and it immediately smelled 
like
this same PSwing bounds issue.  

Reopening this issue. (Sorry...)

Original comment by cmal...@pixelzoom.com on 5 Mar 2010 at 12:04

Attachments:

GoogleCodeExporter commented 9 years ago 
Attached is an updated version of TestPSwingDynamicComponent that tests using a
JLabel, JCheckBox and JRadioButton.  The previous version used only JLabel, 
which
clearly didn't demonstrate all the problems.

Original comment by cmal...@pixelzoom.com on 5 Mar 2010 at 3:43

Attachments:

GoogleCodeExporter commented 9 years ago 
In PSwing r973, a HierarchyBoundsListener was added in the PSwing constructor.  
The
javadoc for this interface says "The listener interface for receiving ancestor 
moved
and resized events."  An ancestor is a parent or (recursively) the parent of an
ancestor. Maybe I don't understand something, but if a PSwing's component is a
JPanel, why would we be interested in ancestors?  Aren't we interested in 
descendents
(contents of the panel)?

Original comment by cmal...@pixelzoom.com on 5 Mar 2010 at 4:06

GoogleCodeExporter commented 9 years ago 
If I add this call as the first line of PSwing.updateBounds, the problem seems 
to be
resolved:

component.revalidate();

I also don't think the HierarchyBoundsListener added in the constructor is doing
anything, so it should probably be deleted. Allain, please correct me if I'm 
wrong here.

Ditto for the PropertyChangeListener that's added in the constructor, it 
doesn't even
check for specific properties, just calls updateBounds when *anything* changes.

Original comment by cmal...@pixelzoom.com on 5 Mar 2010 at 4:33

GoogleCodeExporter commented 9 years ago 
Btw... If we decide to keep the listeners added to component in the 
constructor, they
should really be added in initializeComponent.  I find it confusing that some 
very
similar initialization (ie, adding listeners) is currently done in two different
places (constructor and initializeComponent).

Original comment by cmal...@pixelzoom.com on 5 Mar 2010 at 4:36

GoogleCodeExporter commented 9 years ago 
Can't disagree with you regarding the hierarchy listener, I even mentioned the
strange behaviour when I made the change. The fact is, it did resolve the 
problem. It
shouldn't have, but it did.

You said, that if you add component.revalidate(); to PSwing updateBounds, then 
it
fixes the problem, what problem? The ellipsis, or the incorrect layout, or 
both? And
is that after you've removed the hierarchy listener?

Original comment by allain.lalonde on 5 Mar 2010 at 2:47

GoogleCodeExporter commented 9 years ago 
Sorry, I should have been clearer and it was late.

Adding component.revalidate() with no other changes to PSwing fixes both the 
ellipsis
and layout issues.  Apparently the bounds of both the component and PSwing were 
being
updated using the component's dirty notion of its preferred size.

I then took a look at the HierarchyBoundsListener and PropertyChangeListener 
added in
the constructor, both of which exist to call updateBounds.  I added debug 
output to
HierarchyBoundsListener.ancestorResized and 
PropertyChangeListener.propertyChanged
(printing getPropertyName), then ran both  TestPSwingDynamicComponent and one 
of our
applications that makes heavy use of PSwing.  I didn't see anything that leads 
me to
believe that these listeners are being called in a situation that affects the 
bounds.

If you'd like me to make the changes to PSwing myself, let me know.  I've never
committed to Piccolo's SVN repository, so I'll need a little guidance on any 
formalities.

Original comment by cmal...@pixelzoom.com on 5 Mar 2010 at 3:11

GoogleCodeExporter commented 9 years ago 
This issue was closed by revision r978.

Original comment by cmal...@pixelzoom.com on 8 Mar 2010 at 6:35

GoogleCodeExporter commented 9 years ago 
[deleted comment]
GoogleCodeExporter commented 9 years ago 
I applied the component.revalidate fix in r978.

One thing that I didn't note in the commit message: I opted NOT to remove the
PropertyChangeListener that's added in the PSwing constructor.  Unlike the
HierarchyBoundsListener (which I did remove), this PropertyChangeListener has 
been
there for a long time.  So rather than introduce some new problem, I thought it
prudent to leave it alone for now.  I did add a "TODO" comment indicating that 
this
listener looks suspicious, because it's calling updateBounds regardless of which
property changes.

Original comment by cmal...@pixelzoom.com on 8 Mar 2010 at 7:02

GoogleCodeExporter commented 9 years ago 
Rats. This is still broken. I've been developing and testing on a very fast 
MacBook
Pro.  One of my colleagues ran TestPSwingDynamicComponent on a slower Windows
machine, and when that app starts up, none of the Swing stuff outside the
PSwingCanvas is drawn. See attached screenshot, no-draw.png.  When he mouses 
over
areas of the frame outside the canvas, JComponents are drawn.

Looking into PSwingCanvas, I see that the Swing repaint manager is replaced by
PSwingRepaintManager.  The javadoc for PSwingRepaintManager.addInvalidComponent
indicates that it's called by component.revalidate, and it in turn calls
PSwing.updateBounds.  So what I've done by calling component.revalidate in
updateBounds is to introduce an infinite loop: component.revalidate ->
PSwingRepaintManager.addInvalidComponent -> PSwing.updateBounds ->
component.revalidate -> ...  Putting a System.out.prinln at the first line of
PSwing.updateBounds confirms this. 

On my speedy Mac, everything still manages to get redraw.  But on a slow 
machine, the
event dispatch queue is hammered, and some things don't draw.

Back to the drawing board.

Original comment by cmal...@pixelzoom.com on 8 Mar 2010 at 11:39

GoogleCodeExporter commented 9 years ago 
[deleted comment]
GoogleCodeExporter commented 9 years ago 
[deleted comment]
GoogleCodeExporter commented 9 years ago 
Commited PSwing r979. This backs out the component.revalidate added in r978, 
since it
was causing an infinite loop.  PSwing should now be back to the state it was in
before we started working on this issue (r965).

Original comment by cmal...@pixelzoom.com on 9 Mar 2010 at 12:37

GoogleCodeExporter commented 9 years ago 
Committed PSwing r980, forgot to restore one component.revalidate call in 
constructor.

Original comment by cmal...@pixelzoom.com on 9 Mar 2010 at 12:38

GoogleCodeExporter commented 9 years ago 
It's interesting that this bounds problem does not occur with PhET's Piccolo 1.2
snapshot (r390).  I recall that we reported some breaking changes to PSwing, so 
I
looked back through the commit comments and found this for r863:

> Applying PSwing file provided by sreid. Javadocs have been re-added. And some
Refactoring has taken place.

The only file changed in r863 was PSwing. But other source in the pswing package
(PSwingRepaintManager, PSwingCanvas,...) also differs from our 1.2 snapshot.  
So I
suspect that something was changed outside of PSwing that doesn't place nice 
with the
PSwing file provided by sreid.

Original comment by cmal...@pixelzoom.com on 9 Mar 2010 at 12:44

GoogleCodeExporter commented 9 years ago 
component.revalidate calls PSwingRepaintManager.addInvalidComponent, which calls
PSwing.updateBounds.  So calls to revalidate will result in calls to 
updateBounds. 
In PSwing r930, there are 3 calls to updateBounds in listeners (lines 
325,523,527). 
If I replace those calls with component.revalidate, things seem to work 
correctly,
but the PSwing's components visibly changes its layout (looks jerky) when it's
repainted.

Original comment by cmal...@pixelzoom.com on 9 Mar 2010 at 5:50

GoogleCodeExporter commented 9 years ago 
More general issue... I don't understand why we need to explicitly call
component.revalidate.  When (for example) ComponentListener.componentResized is
called, why is the component's size/preferredSize invalid?

Original comment by cmal...@pixelzoom.com on 9 Mar 2010 at 6:17

GoogleCodeExporter commented 9 years ago 
TestPSwingDynamicComponent has a PSwing wrapping a JPanel, which contains 3
JComponents.  I added this debug output to the ComponentListener added in
PSwing.initializeComponent:

System.out.println( "PSwing$ComponentAdapter.componentResized " +
e.getComponent().getClass().getName() );

When the text of one of the panel's JComponents is changed, PSwing's
ComponentListener.componentResized is called 4 times, here's the debug output:

PSwing$ComponentAdapter.componentResized
edu.colorado.phet.common.piccolophet.test.TestPSwingDynamicComponent$ComponentPa
nel
PSwing$ComponentAdapter.componentResized javax.swing.JLabel
PSwing$ComponentAdapter.componentResized javax.swing.JCheckBox
PSwing$ComponentAdapter.componentResized javax.swing.JRadioButton

So this is likely the source of the what I described as "jerky" behavior. It's
described elsewhere in descriptions of revalidate/validate/invalidate as 
"flickering".

Original comment by cmal...@pixelzoom.com on 9 Mar 2010 at 6:44

GoogleCodeExporter commented 9 years ago 
Chris, have you made any additional progress on this issue?

Original comment by heue...@gmail.com on 24 Aug 2010 at 3:08

GoogleCodeExporter commented 9 years ago 
Yes, Sam Reid and I worked on this and made some good progress.  But we have 
been delinquent in contributing our fixes back to Piccolo.  Sam and I will 
discuss tomorrow and try to get things contributed back asap.

Original comment by cmal...@pixelzoom.com on 24 Aug 2010 at 3:26

GoogleCodeExporter commented 9 years ago 
My collaborator cmalley & I resolved issue 163, by fixing PSwing and 
PSwingRepaintManager.  We tested that this resolves the problem in 
TestPSwingDynamicComponent (attached to the ticket description).  We have also 
been using this fix successfully in our work for several months with no serious 
new complications.  We had started a discussion of this in the piccolo 
developer forum, but it stalled out, please see 
http://groups.google.com/group/piccolo2d-dev/browse_thread/thread/a3370d05706bd9
3?fwc=1

Please let us know if you'd like us to check these changes into the 1.3 branch 
and/or trunk, or if you'd like someone else to review the changes first.  You 
can see the changes in files attached to aforementioned thread.

Original comment by reids%co...@gtempaccount.com on 25 Aug 2010 at 4:34

GoogleCodeExporter commented 9 years ago 
Great, commit to piccolo2d.java/branches/release-1.3, post the revision number 
here, and we'll review.

Original comment by heue...@gmail.com on 25 Aug 2010 at 4:41

GoogleCodeExporter commented 9 years ago 
This issue was closed by revision r1045.

Original comment by reids%co...@gtempaccount.com on 25 Aug 2010 at 5:07

GoogleCodeExporter commented 9 years ago 
In r1045, I committed our fixes to PSwing and PSwingRepaintManager, and also 
our latest version of TestPSwingDynamicComponent, which has been renamed to 
PSwingDynamicComponentTest for consistency with other tests in the package.  
The commit message is:

Added a test PSwingDynamicComponentTest for testing handling of dynamic pswing 
content.  Fixed issue 163 by improving support for dynamic resizing of pswing 
target components.  Refactoring of PSwing and PSwingRepaintManager to simplify 
and improve readability.

Google automatically parsed "fixed issue 163" to close the ticket, please note, 
however, that this fix still needs verification.  If this is incorrect, please 
feel free to reopen the ticket.

Original comment by reids%co...@gtempaccount.com on 25 Aug 2010 at 5:12

GoogleCodeExporter commented 9 years ago 
After review of the fixes in r1045, we should discuss whether they should also 
be copied to trunk.

Original comment by reids%co...@gtempaccount.com on 25 Aug 2010 at 5:13

GoogleCodeExporter commented 9 years ago 
Fixed is correct, after review it goes to Verified.  And yes, these changes 
should be copied to trunk after review.

Original comment by heue...@gmail.com on 25 Aug 2010 at 5:28

GoogleCodeExporter commented 9 years ago 
Based on huermh's review, we committed r1046:

PSwingDynamicComponentTest to examples/pswing and renamed to 
PSwingDynamicComponentExample.  Improved header javadoc, and added piccolo2d 
copyright header, see issue 163

Please review and let us know if there's anything else.  After you reverify, we 
will move all changes to trunk.

Original comment by reids%co...@gtempaccount.com on 25 Aug 2010 at 5:37

GoogleCodeExporter commented 9 years ago 
Looks good to me, please apply to trunk.

Original comment by heue...@gmail.com on 2 Mar 2011 at 4:14

GoogleCodeExporter commented 9 years ago 
In r1112 I committed: Applied fixes from the piccolo 1.3 branch to trunk, see 
issue 163

Can huermh or others please verify that the changes still look good? It looked 
like there were some repackagings done on trunk since Aug 2010 when 1.3 was 
originally fixed.

Thanks!
Sam

Original comment by reids%co...@gtempaccount.com on 7 Mar 2011 at 7:51

GoogleCodeExporter commented 9 years ago 
Verified issue on Mac OS X 64-bit, Win Vista 32-bit -- Intel Mac Pro. OpenJDK - 
Intel Dell Dimension.

Original comment by atdi...@gmail.com on 18 Mar 2011 at 11:22