kerou / mt4j

Automatically exported from code.google.com/p/mt4j
GNU General Public License v2.0
0 stars 0 forks source link

StyleInfo objects expose references to private fields. #16

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
What steps will reproduce the problem?
1. Create a StyleInfo object and set the fill color to red.
2. Create two MTRectangles, and set their fill colors to blue.
3. Set the StyleInfo to the first MTRectangle.
4. Set the first MTRectangle's fill color to yellow.
5. Set the StyleInfo to the second MTRectangle.

What is the expected output? What do you see instead?
The second MTRectangle should end up red (what was set in the StyleInfo),
but it ends up yellow (what was set in the first rectangle after the StyleInfo 
was applied to it).

What version of the product are you using? On what operating system?
Both v0.95 and svn trunk exhibit the problem.  Ubuntu 10.4 x86_64, 
JavaSE-1.6(java-6-openjdk).

Please provide any additional information below.
It looks like when you set a StyleInfo to a shape, the fields of the StyleInfo 
are set to the shape by reference.  So when the shape's fill color is set 
manually (via .setFillColor()), it changes in the StyleInfo too.  This should 
probably be fixed by making defensive copies somewhere.  I'd put together a 
patch for this, but I'm unsure where the best place to make the copies is.  In 
the StyleInfo?  In the shape?  Somewhere else?  I can provide a code example to 
demonstrate if needed.

Original issue reported on code.google.com by jpkiffme...@gmail.com on 17 Sep 2010 at 12:04

GoogleCodeExporter commented 9 years ago
Well in Java all objects are passed by reference so thats normal behaviour. One 
could argue whether the behaviour you describe is a bug or not. A StyleInfo 
object is meant to be for one component and belongs to it and changes according 
to the component.
We could of course make copies of the color when setting it. But if we for 
example make a color animation we are creating many new color objects each 
frame which is to avoid in real time graphics and because of garbage collection.
So I have to think about this somemore. Feel free to add your thoughts.

Original comment by sirhc.f...@gmail.com on 13 Oct 2010 at 1:17

GoogleCodeExporter commented 9 years ago
Ah, good point.  Perhaps the shape could make a defensive copy of a StyleInfo 
it gets by setStyleInfo().  That shouldn't happen too often, right?  I think my 
expectation is that if I setup a StyleInfo object to apply to many shapes, 
setStyleInfo() should have the same effect every time.  This doesn't hold under 
the conditions described before.  On the other hand, if "A StyleInfo object is 
meant to be for one component and belongs to it," then I suppose it's 
reasonable for me to make a copy of my StyleInfo object for each call I make to 
setStyleInfo().  If that's the case, a copy constructor for StyleInfo would be 
super handy.  (e.g., "new StyleInfo(oldStyleInfo);")

Original comment by jpkiffme...@gmail.com on 13 Oct 2010 at 4:34