mono / libgdiplus

C-based implementation of the GDI+ API
http://www.mono-project.com/
MIT License
334 stars 171 forks source link

Keep clipping inside containers. #639

Closed PreferLinux closed 4 years ago

PreferLinux commented 4 years ago

Containers need to maintain the clipping that exists outside them.

I have implemented this by storing the overall clip region outside the current container (graphics->previous_clip, NULL if not in a container or if no clipping outside) in addition to any clip region set (graphics->clip), and then intersecting them in cairo_SetGraphicsClip(), GdipBeginContainer2() and GdipGetVisibleClipBounds() (the three places that need the overall clip). previous_clip goes on the save stack on saving, in addition to clip.

Alternatively the intersection could be done when modifying the clip and when restoring it off the save stack (four places), but that would mean storing the resulting region as well as the other two, although the overall clip wouldn't need to be stored on the save stack. (GdipTranslateClip(), GdipGetClip() and GdipGetClipBounds() need the clip that was set.)

Also, I modified GdipResetClip() to call cairo_SetGraphicsClip() rather than cairo_ResetClip() when previous_clip exists, as it needs to keep that. Alternatively, it would be possible to modify cairo_ResetClip() to set the clip to previous_clip itself.

I have implemented this into GdipGetVisibleClipBounds(), as that should provide the overall clip bounds. And I changed GdipIsVisiblePoint() and GdipIsVisibleRect() to use that instead of the graphics->bounds (they should take clipping into account). I won't mention the insane check previously used in the latter of those two methods, but the replacement was taken from region.c – ideally I'd have added a declaration for it to region-private.h, but I didn't think gdip_intersects() was a descriptive enough name for that, and I didn't want to do a rename.

I guess I should write some tests too, so I'll try to get around to that some time soon. But I'll put this out there now for any comments.

PreferLinux commented 4 years ago

I realised shortly after I posted this yesterday that GdipIsVisiblePoint() and GdipIsVisibleRect() should not be using the clip bounds, they should be using the clip itself... So I'll sort that out. I'll also change to the alternative I mentioned above, because that'll result in less intersecting of clip with previous_clip.

filipnavara commented 4 years ago

Sorry it is taking me so long to review this. I'm still going through the ASAN reports (after applying @akoeplinger's test patches). There are some leaks reported and I have to figure out whether they are false positives, pre-existing leaks, or something introduced by this PR (likely mix of all of this).

PreferLinux commented 4 years ago

Thanks for picking that up. Your suggested fix looks good to me, so I've committed it.