Community
Participate
Working Groups
In my excitement to use the latest, I am in the process of upgrading our project software from Eclispe 3.1.2 to 3.2.1. I encountered a problem that I have distilled in the attached example and output. I posted a message on the GEF newsgroup, but received no answers. I'm not sure if I'm doing the right thing in opening a bug, but here goes... The example has a FigureCanvas in a Shell. The canvas has a root figure, and the root figure has 2 round rectangles. The example I have provided attempts to draw a transparent rectangle on the canvas. Drawing this transparent rectangle is as follows: when the user holds the left mouse button down and moves from point A to point B, the rectangle is drawn based on these 2 points. The result using the 3.1.2 libraries is as expected - shown in FigureCanvasTransparencyWith_3.1.2_correct.png The result using the 3.2.1 libraries is clipped - shown in FigureCanvasTransparencyWith_3.2.1_incorrect.png, and the transparent rectangle is only partially shown over the 2nd child figure added. Attached is also the source code for the snippet. The same incorrect behavior is exhibited using 3.2.0. Thank you! E.
Created attachment 51963 [details] Code snippet reproducing the anomaly Running the snippet with a 3.1 target platform works properly. Running it with a 3.2 target platform exhibits the anomaly.
Created attachment 51965 [details] PNG file showing expected results Running previous java snippet with the 3.1 platform produces expected results
Created attachment 51966 [details] PNG file showing unexpected results Running previous java snippet with the 3.2 platform produces unexpected results
Created attachment 52347 [details] Snippet with workaround From an exchange with Randy Hudson, regarding this issue, on the GEF newsgroup: "It seems like there is no isolation between two independent paint listeners on a Control. So, for some reason, the GC is being left in a different state in 3.2 than it was in 3.1. You can fix the GC's clip by setting it back to the event's clip area. Also, you could switch to using a figure on a transparent layer above your primary figures. This is how marquees are done." I can confirm than when I set the GC's clipping area, the transparent drawing worked as expected. Attached is the working snippet.
I reproduced this problem on 3.8. using the test code supplied. I think the solution could be related to the clipping region on SWTGraphics. I added the following code to org.eclipse.draw2d.test.GraphicsClipping and the test fails: public void testClipRestChanging() { Rectangle fullRect = new Rectangle(0, 0, 800, 600); Rectangle bigRect = new Rectangle(0, 0, 200, 200); Rectangle smallRect = new Rectangle(0, 0, 100, 100); // clip to big rect works fine graphics.clipRect(bigRect); Assert.assertEquals(200, graphics.getClip(fullRect.getCopy()).width); // clip to small rect works fine graphics.clipRect(smallRect); Assert.assertEquals(100, graphics.getClip(fullRect.getCopy()).width); // clip back to big rect does not work as I expected. graphics.clipRect(bigRect); Assert.assertEquals(200, graphics.getClip(fullRect.getCopy()).width); } Is this a valid test? Should it run successfully?
(In reply to comment #5) > I reproduced this problem on 3.8. using the test code supplied. I think the > solution could be related to the clipping region on SWTGraphics. > > I added the following code to org.eclipse.draw2d.test.GraphicsClipping and > the test fails: > > public void testClipRestChanging() { > Rectangle fullRect = new Rectangle(0, 0, 800, 600); > Rectangle bigRect = new Rectangle(0, 0, 200, 200); > Rectangle smallRect = new Rectangle(0, 0, 100, 100); > > // clip to big rect works fine > graphics.clipRect(bigRect); > Assert.assertEquals(200, graphics.getClip(fullRect.getCopy()).width); > > // clip to small rect works fine > graphics.clipRect(smallRect); > Assert.assertEquals(100, graphics.getClip(fullRect.getCopy()).width); > > // clip back to big rect does not work as I expected. > graphics.clipRect(bigRect); > Assert.assertEquals(200, graphics.getClip(fullRect.getCopy()).width); > } > > Is this a valid test? Should it run successfully? No, it isn't. There are actually two clipping methods, setClip and clipRect. clipRect intersects the current clipping of the graphics with the given rectangle and uses the resulting intersection rectangle. setClip takes the argument as new clipping region. If you use setClip instead of clipRect for resetting the clipping to the large rectangle, everything is fine.
Created attachment 224200 [details] Proposed patch for the problem Thanks for your help Alexander. I think I found the problem. SWTGraphics has a push and pop state feature. When you push, a copy of the gc state is made, and when you pop, the gc state should restore. However, the gc state is actually restored only when you call a 'draw' method on SWTGraphics. The problem is that the state was not restored if the client code uses GC directly (like in the snippet). So I fixed the problem by making sure the cg is updated when popping. I also added a unit test that initially failed and is now fixed.
(In reply to comment #7) > Created attachment 224200 [details] > Proposed patch for the problem > > Thanks for your help Alexander. > > I think I found the problem. SWTGraphics has a push and pop state feature. > When you push, a copy of the gc state is made, and when you pop, the gc > state should restore. However, the gc state is actually restored only when > you call a 'draw' method on SWTGraphics. The problem is that the state was > not restored if the client code uses GC directly (like in the snippet). > > So I fixed the problem by making sure the cg is updated when popping. > > I also added a unit test that initially failed and is now fixed. Yes, indeed that seems to be the problem. However, as checkGC affects the graphicsHints as well, I want to get sure this does not involve any side effects before applying it. Anyway, thanks for figuring that out!
> When you push, a copy of the gc state is made, and when you pop, the gc > state should restore. No, only the SWTGraphics state is restored. This is intentional. When popState() is called repeatedly, it is wasteful to update the real GC with different clip regions that never get used. > However, the gc state is actually restored only when > you call a 'draw' method on SWTGraphics. By design; this is an optimization. > The problem is that the state was > not restored if the client code uses GC directly (like in the snippet). It is invalid to use a mixture of the SWT graphics and GC that it wrappers. There are many methods on SWT graphics that have no immediate effect on the GC. > So I fixed the problem by making sure the cg is updated when popping. This would probably introduce a performance regression. > I also added a unit test that initially failed and is now fixed. The last part of the unit test is invalid: + graphics.popState(); + assertEquals(800, graphics.getClip(fullRect.getCopy()).width); + assertEquals(800, gc.getClipping().width); There's no guarantee that popState() will modify the wrappered GC in any way.
(In reply to comment #9) Thank you Randy. Considering your assertion: "It is invalid to use a mixture of the SWT graphics and GC that it wrappers."; it is clear that my test case is not valid. However, could this assertion also imply that the attached 'snippet that reproduces the anomaly' makes invalid use of draw2d? Assuming that the snippet is valid use and that changing GC on every pop incurs a performance penalty; here are two alternative fixes I tested: 1. Call checkGC() at the last pop (when stackPointer == 0) 2. Call checkGC() when the SWTGraphics instance is disposed. Would any of these be an acceptable solution? Is it better to call checkPaint() or something else than checkGC()?
Can't this problem be reproduced with just SWT? Add two Paint listeners to a Canvas, the first one modifies the GC's clip (or foreground color, line width, or anything else), while the second listener assumes the GC is unmodified and starts painting. Your PaintListener receives the region (x,y,width,height) in the PaintEvent, so it could reset the clip using that.
(In reply to comment #11) > Can't this problem be reproduced with just SWT? Add two Paint listeners to > a Canvas, the first one modifies the GC's clip (or foreground color, line > width, or anything else), while the second listener assumes the GC is > unmodified and starts painting. If I wrote both listeners, I can fix my code. And I would definitely consider this particular situation to be a bug in my code. In short, I think it is reasonable to expect that the painting logic within draw2d leaves the GC unchanged. If this is not a reasonable expectation, then any careful programmer must assume the worst. He now has to assume that the library may make arbitrary changes to any of the CG properties. To make his program 'future proof' he now has to reset all GC fields (clipping, foreground color, background color and so on) before every draw response. In other words, the client takes a performance knock because the library guarantees nothing in this regard. It is also a very real fact that the definition of what is 'reasonable expectation' is the responsibility of the informed designer of a library. And I (being a novice) have merely an opinion. Thus, I have to accept your call on the matter. It seems like the choice is between four options: a. We explore the question of whether this is a bug or not a bit further. b. We try and find alternative (and more correct) patches. c. We extend the public interface of the library that provides an alternative way a client would implement the functionality. d. We do nothing. Close the bug as 'won't fix'.
While this optimization within FigureCanvas might have been built-in by design, I agree that it is not transparent to clients that the underlying GC is left in a changed state after the SWTGraphics state has been fully restored. I think we should raise a bug against SWT and request that paint listeners get decoupled (which seems to have been the case up to 3.1, otherwise this regression would not have been reported). Having obtained the opinion of the SWT team, we can then decide what to do with SWTGraphics
(In reply to comment #13) > I think we should raise a bug against SWT and request that paint listeners get > decoupled (which seems to have been the case up to 3.1, otherwise this > regression would not have been reported). Having obtained the opinion of the SWT > team, we can then decide what to do with SWTGraphics Ok, I understand Randy's point of view a bit better now. Draw2d expects SWT to take care of this issue. I'll raise a bug there.
Related bug url: https://bugs.eclipse.org/bugs/show_bug.cgi?id=395849
