A test app revealed some bugs with regards to client provided clipping regions:

* It is necessary to store the local origin and scale of a view state, at
  least for the clipping region and especially because the scale affects the
  clipping region. Ie origin and scale of one state affect the clipping region,
  at the time the clipping is evaluated, ie
     SetOrigin(...);
     ConstrainClippingRegion(...);
  is equivalent to
     ConstrainClippingRegion(...);
     SetOrigin(...);
  The current client provided clipping region at the time of PushState()
  always constrains the region of the new state. The previous region
  and the current local clipping may have their own individual origin and
  scale.
  To support all this, I needed to store origin and scale of each state on
  the stack, but I do cache the combined origin and scale. Caching only
  the combined region is not possible though. So the new implementation
  is slower than the previous, but more correct.
* Refactored "copy constructor" from DrawState, which is not really a copy
  constructor anyways, made it private. It is only used by PushState().
* Removed some dead code from DrawState.

All this improves Firefox redraw issues a tiny bit, but does not fix them.
Either I am not covering Firefox needs with my test app, or the bug is
somewhere else. At least Haiku behaves like BeOS with regard to client
clipping regions and the state stack now...


git-svn-id: file:///srv/svn/repos/haiku/haiku/trunk@24428 a95241bf-73f2-0310-859d-f6bbb57e9c96

src/servers/app/DrawState.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright 2001-2005, Haiku.
+ * Copyright 2001-2008, Haiku.
  * Distributed under the terms of the MIT License.
  *
  * Authors:
@@ -10,9 +10,10 @@
  *		Michael Pfeiffer <[email protected]>
  */
 
-/**	Data classes for working with BView states and draw parameters */
+//!	Data classes for working with BView states and draw parameters
 
 
+#include <new>
 #include <stdio.h>
 
 #include <Region.h>
@@ -22,19 +23,27 @@
 
 #include "DrawState.h"
 
+using std::nothrow;
+
 
 DrawState::DrawState()
 	: fOrigin(0.0, 0.0),
+	  fCombinedOrigin(0.0, 0.0),
 	  fScale(1.0),
+	  fCombinedScale(1.0),
 	  fClippingRegion(NULL),
+
 	  fHighColor((rgb_color){ 0, 0, 0, 255 }),
 	  fLowColor((rgb_color){ 255, 255, 255, 255 }),
 	  fPattern(kSolidHigh),
+
 	  fDrawingMode(B_OP_COPY),
 	  fAlphaSrcMode(B_PIXEL_ALPHA),
 	  fAlphaFncMode(B_ALPHA_OVERLAY),
+
 	  fPenLocation(0.0, 0.0),
 	  fPenSize(1.0),
+
 	  fFontAliasing(false),
 	  fSubPixelPrecise(false),
 	  fLineCapMode(B_BUTT_CAP),
@@ -46,10 +55,39 @@ DrawState::DrawState()
 }
 
 
-DrawState::DrawState(const DrawState& from)
+DrawState::DrawState(DrawState* from)
-	: fClippingRegion(NULL)
+	: fOrigin(0.0, 0.0),
+	  fCombinedOrigin(from->fCombinedOrigin),
+	  fScale(1.0),
+	  fCombinedScale(from->fCombinedScale),
+	  fClippingRegion(NULL),
+
+	  fHighColor(from->fHighColor),
+	  fLowColor(from->fLowColor),
+	  fPattern(from->fPattern),
+
+	  fDrawingMode(from->fDrawingMode),
+	  fAlphaSrcMode(from->fAlphaSrcMode),
+	  fAlphaFncMode(from->fAlphaFncMode),
+
+	  fPenLocation(from->fPenLocation),
+	  fPenSize(from->fPenSize),
+
+	  fFont(from->fFont),
+	  fFontAliasing(from->fFontAliasing),
+
+	  fSubPixelPrecise(from->fSubPixelPrecise),
+
+	  fLineCapMode(from->fLineCapMode),
+	  fLineJoinMode(from->fLineJoinMode),
+	  fMiterLimit(from->fMiterLimit),
+
+	  // Since fScale is reset to 1.0, the unscaled
+	  // font size is the current size of the font
+	  // (which is from->fUnscaledFontSize * from->fCombinedScale)
+	  fUnscaledFontSize(from->fUnscaledFontSize),
+	  fPreviousState(from)
 {
-	*this = from;
 }
 
 
@@ -60,59 +98,10 @@ DrawState::~DrawState()
 }
 
 
-DrawState&
-DrawState::operator=(const DrawState& from)
-{
-	fOrigin	= from.fOrigin;
-	fScale	= from.fScale;
-
-	if (from.fClippingRegion) {
-		if (fClippingRegion)
-			*fClippingRegion = *from.fClippingRegion;
-		else
-			fClippingRegion = new BRegion(*from.fClippingRegion);
-	} else {
-		delete fClippingRegion;
-		fClippingRegion = NULL;
-	}
-
-	fHighColor			= from.fHighColor;
-	fLowColor			= from.fLowColor;
-	fPattern			= from.fPattern;
-
-	fDrawingMode		= from.fDrawingMode;
-	fAlphaSrcMode		= from.fAlphaSrcMode;
-	fAlphaFncMode		= from.fAlphaFncMode;
-
-	fPenLocation		= from.fPenLocation;
-	fPenSize			= from.fPenSize;
-
-	fFont				= from.fFont;
-	fFontAliasing		= from.fFontAliasing;
-
-	fSubPixelPrecise	= from.fSubPixelPrecise;
-
-	fLineCapMode		= from.fLineCapMode;
-	fLineJoinMode		= from.fLineJoinMode;
-	fMiterLimit			= from.fMiterLimit;
-
-	// Since fScale is reset to 1.0, the unscaled
-	// font size is the current size of the font
-	// (which is from->fUnscaledFontSize * from->fScale)
-	fUnscaledFontSize	= from.fUnscaledFontSize;
-	fPreviousState		= from.fPreviousState;
-
-	return *this;
-}
-
-
 DrawState*
 DrawState::PushState()
 {
-	DrawState* next = new DrawState(*this);
+	DrawState* next = new (nothrow) DrawState(this);
-		// this may throw an exception that our caller should handle
-
-	next->fPreviousState = this;
 	return next;
 }
 
@@ -213,6 +202,14 @@ DrawState::ReadFromLink(BPrivate::LinkReceiver& link)
 	link.Read<float>(&fScale);
 	link.Read<bool>(&fFontAliasing);
 
+	if (fPreviousState) {
+		fCombinedOrigin = fPreviousState->fCombinedOrigin + fOrigin;
+		fCombinedScale = fPreviousState->fCombinedScale * fScale;
+	} else {
+		fCombinedOrigin = fOrigin;
+		fCombinedScale = fScale;
+	}
+
 	fHighColor = highColor;
 	fLowColor = lowColor;
 	fPattern = patt;
@@ -283,78 +280,115 @@ void
 DrawState::SetOrigin(const BPoint& origin)
 {
 	fOrigin = origin;
-	// origin is given as a point in the
-	// "outer" coordinate system, therefore
-	// it has to be transformed
-	if (PreviousState() != NULL)
-		PreviousState()->Transform(&fOrigin);
-}
 
-
+	// NOTE: the origins of earlier states are never expected to
-void
+	// change, only the topmost state ever changes
-DrawState::OffsetOrigin(const BPoint& offset)
+	if (fPreviousState)
-{
+		fCombinedOrigin = fPreviousState->fCombinedOrigin + fOrigin;
-	if (PreviousState() == NULL)
+	else
-		fOrigin += offset;
+		fCombinedOrigin = fOrigin;
-	else {
-		// TODO this is necessary only if offset 
-		// is in the "outer" coordinate system
-		float scale = PreviousState()->Scale();
-		fOrigin.x += offset.x * scale;
-		fOrigin.y += offset.y * scale;
-	}		
 }
 
 
 void
 DrawState::SetScale(float scale)
 {
-	// the scale is multiplied with the scale of the previous state if any
+	if (fScale != scale) {
-	float localScale = scale;
+		fScale = scale;
-	if (PreviousState() != NULL)
-		localScale *= PreviousState()->Scale();
 
-	if (fScale != localScale) {
+		// NOTE: the scales of earlier states are never expected to
-		fScale = localScale;
+		// change, only the topmost state ever changes
+		if (fPreviousState)
+			fCombinedScale = fPreviousState->fCombinedScale * fScale;
+		else
+			fCombinedScale = fScale;
 
 		// update font size
-		// (pen size is currently calulated on the fly)
+		fFont.SetSize(fUnscaledFontSize * fCombinedScale);
-		fFont.SetSize(fUnscaledFontSize * fScale);
 	}
 }
 
-// Transform
+
+void
+DrawState::SetClippingRegion(const BRegion* region)
+{
+	if (region) {
+		if (fClippingRegion)
+			*fClippingRegion = *region;
+		else
+			fClippingRegion = new (nothrow) BRegion(*region);
+	} else {
+		delete fClippingRegion;
+		fClippingRegion = NULL;
+	}
+}
+
+
+bool
+DrawState::HasClipping() const
+{
+	if (fClippingRegion)
+		return true;
+	if (fPreviousState)
+		return fPreviousState->HasClipping();
+	return false;
+}
+
+
+bool
+DrawState::GetCombinedClippingRegion(BRegion* region) const
+{
+	if (fClippingRegion) {
+		BRegion localTransformedClipping(*fClippingRegion);
+		Transform(&localTransformedClipping);
+
+		if (fPreviousState && fPreviousState->GetCombinedClippingRegion(region))
+			localTransformedClipping.IntersectWith(region);
+		*region = localTransformedClipping;
+		return true;
+	} else {
+		if (fPreviousState)
+			return fPreviousState->GetCombinedClippingRegion(region);
+	}
+	return false;
+}
+
+
+// #pragma mark -
+
+
 void
 DrawState::Transform(float* x, float* y) const
 {
 	// scale relative to origin, therefore
 	// scale first then translate to
 	// origin
-	*x *= fScale;
+	*x *= fCombinedScale;
-	*y *= fScale;
+	*y *= fCombinedScale;
-	*x += fOrigin.x;
+	*x += fCombinedOrigin.x;
-	*y += fOrigin.y;
+	*y += fCombinedOrigin.y;
 }
 
-// InverseTransform
+
 void
 DrawState::InverseTransform(float* x, float* y) const
 {
-	// TODO: watch out for fScale = 0?
+	*x -= fCombinedOrigin.x;
-	*x -= fOrigin.x;
+	*y -= fCombinedOrigin.y;
-	*y -= fOrigin.y;
+	if (fCombinedScale != 0.0) {
-	*x /= fScale;
+		*x /= fCombinedScale;
-	*y /= fScale;
+		*y /= fCombinedScale;
+	}
 }
 
-// Transform
+
 void
 DrawState::Transform(BPoint* point) const
 {
 	Transform(&(point->x), &(point->y));
 }
 
-// Transform
+
 void
 DrawState::Transform(BRect* rect) const
 {
@@ -362,13 +396,12 @@ DrawState::Transform(BRect* rect) const
 	Transform(&(rect->right), &(rect->bottom));
 }
 
-// Transform
+
 void
 DrawState::Transform(BRegion* region) const
 {
-	if (fScale == 1.0) {
+	if (fCombinedScale == 1.0) {
-		if (fOrigin.x != 0.0 || fOrigin.y != 0.0)
+		region->OffsetBy(fCombinedOrigin.x, fCombinedOrigin.y);
-			region->OffsetBy((int32)fOrigin.x, (int32)fOrigin.y);
 	} else {
 		// TODO: optimize some more
 		BRegion converted;
@@ -384,8 +417,8 @@ DrawState::Transform(BRegion* region) const
 			Transform(&lt.x, &lt.y);
 			Transform(&rb.x, &rb.y);
 			// reset bottom right to pixel "index"
-			rb.x++;
+			rb.x--;
-			rb.y++;
+			rb.y--;
 			// add rect to converted region
 			// NOTE/TODO: the rect would not have to go
 			// through the whole intersection test process,
@@ -405,32 +438,7 @@ DrawState::InverseTransform(BPoint* point) const
 }
 
 
-void
+// #pragma mark -
-DrawState::SetClippingRegion(const BRegion* region)
-{
-	// reset clipping to that of previous state
-	// (that's the starting point)
-	if (PreviousState() != NULL && PreviousState()->ClippingRegion()) {
-		if (fClippingRegion)
-			*fClippingRegion = *(PreviousState()->ClippingRegion());
-		else
-			fClippingRegion = new BRegion(*(PreviousState()->ClippingRegion()));
-	} else {
-		delete fClippingRegion;
-		fClippingRegion = NULL;
-	}
-
-	// intersect with the clipping from the passed region
-	// (even if it is empty)
-	// passing NULL unsets this states additional region,
-	// it will then be the region of the previous state
-	if (region) {
-		if (fClippingRegion)
-			fClippingRegion->IntersectWith(region);
-		else 
-			fClippingRegion = new BRegion(*region);
-	}
-}
 
 
 void
@@ -490,8 +498,6 @@ DrawState::PenLocation() const
 void
 DrawState::SetPenSize(float size)
 {
-	// NOTE: since pensize is calculated on the fly,
-	// it is ok to set it here regardless of previous state
 	fPenSize = size;
 }
 
@@ -500,7 +506,7 @@ DrawState::SetPenSize(float size)
 float
 DrawState::PenSize() const
 {
-	float penSize = fPenSize * fScale;
+	float penSize = fPenSize * fCombinedScale;
 	// NOTE: As documented in the BeBook,
 	// pen size is never smaller than 1.0.
 	// This is supposed to be the smallest
@@ -530,7 +536,7 @@ DrawState::SetFont(const ServerFont& font, uint32 flags)
 	if (flags == B_FONT_ALL) {
 		fFont = font;
 		fUnscaledFontSize = font.Size();
-		fFont.SetSize(fUnscaledFontSize * fScale);
+		fFont.SetSize(fUnscaledFontSize * fCombinedScale);
 	} else {
 		// family & style
 		if (flags & B_FONT_FAMILY_AND_STYLE)
@@ -538,7 +544,7 @@ DrawState::SetFont(const ServerFont& font, uint32 flags)
 		// size
 		if (flags & B_FONT_SIZE) {
 			fUnscaledFontSize = font.Size();
-			fFont.SetSize(fUnscaledFontSize * fScale);
+			fFont.SetSize(fUnscaledFontSize * fCombinedScale);
 		}
 		// shear
 		if (flags & B_FONT_SHEAR)

src/servers/app/DrawState.h

@@ -1,5 +1,5 @@
 /*
- * Copyright 2001-2005, Haiku.
+ * Copyright 2001-2008, Haiku.
  * Distributed under the terms of the MIT License.
  *
  * Authors:
@@ -30,13 +30,13 @@ namespace BPrivate {
 
 
 class DrawState {
-	public:
+public:
 							DrawState();
-							DrawState(const DrawState& from);
+private:
+							DrawState(DrawState* from);
+public:
 		virtual				~DrawState();
 
-		DrawState&			operator=(const DrawState& from);
-
 		DrawState*			PushState();
 		DrawState*			PopState();
 		DrawState*			PreviousState() const { return fPreviousState; }
@@ -50,13 +50,22 @@ class DrawState {
 
 							// coordinate transformation
 		void				SetOrigin(const BPoint& origin);
-		void				OffsetOrigin(const BPoint& offset);
 		const BPoint&		Origin() const
 								{ return fOrigin; }
+		const BPoint&		CombinedOrigin() const
+								{ return fCombinedOrigin; }
 
 		void				SetScale(float scale);
 		float				Scale() const
 								{ return fScale; }
+		float				CombinedScale() const
+								{ return fCombinedScale; }
+
+							// additional clipping as requested by client
+		void				SetClippingRegion(const BRegion* region);
+
+		bool				HasClipping() const;
+		bool				GetCombinedClippingRegion(BRegion* region) const;
 
 							// coordinate transformations
 				void		Transform(float* x, float* y) const;
@@ -68,14 +77,6 @@ class DrawState {
 
 				void		InverseTransform(BPoint* point) const;
 
-							// additional clipping as requested by client
-		void				SetClippingRegion(const BRegion* region);
-		const BRegion*		ClippingRegion() const
-								{ return fClippingRegion; }
-/*	inline	int32				CountClippingRects() const
-									{ return fClippingRegion ? fClippingRegion.CountRects() : 0; }
-	inline	BRect				ClippingRectAt(int32 index) const;*/
-
 							// color
 		void				SetHighColor(const rgb_color& color);
 		const rgb_color&	HighColor() const
@@ -140,9 +141,11 @@ class DrawState {
 		bool				SubPixelPrecise() const
 								{ return fSubPixelPrecise; }
 
-	protected:
+protected:
 		BPoint				fOrigin;
+		BPoint				fCombinedOrigin;
 		float				fScale;
+		float				fCombinedScale;
 
 		BRegion*			fClippingRegion;
 
@@ -182,19 +185,4 @@ class DrawState {
 		DrawState*			fPreviousState;
 };
 
-
+#endif	// _DRAW_STATE_H_
-// inline implementations
-
-/*
-// ClippingRectAt
-BRect
-DrawState::ClippingRectAt(int32 index) const
-{
-	BRect r;
-	if (fClippingRegion) {
-		r = fClippingRegion.RectAt(index);
-	}
-	return r;
-}*/
-
-#endif	/* _DRAW_STATE_H_ */

src/servers/app/View.cpp

@@ -440,7 +440,7 @@ View::SetDrawingOrigin(BPoint origin)
 	fDrawState->SetOrigin(origin);
 
 	// rebuild clipping
-	if (fDrawState->ClippingRegion())
+	if (fDrawState->HasClipping())
 		RebuildClipping(false);
 }
 
@@ -464,7 +464,7 @@ View::SetScale(float scale)
 	fDrawState->SetScale(scale);
 
 	// rebuild clipping
-	if (fDrawState->ClippingRegion())
+	if (fDrawState->HasClipping())
 		RebuildClipping(false);
 }
 
@@ -1169,8 +1169,11 @@ View::CopyBits(IntRect src, IntRect dst, BRegion& windowContentClipping)
 void
 View::PushState()
 {
-	fDrawState = fDrawState->PushState();
+	DrawState* newState = fDrawState->PushState();
-	fDrawState->SetSubPixelPrecise(fFlags & B_SUBPIXEL_PRECISE);
+	if (newState) {
+		fDrawState = newState;
+		fDrawState->SetSubPixelPrecise(fFlags & B_SUBPIXEL_PRECISE);
+	}
 }
 
 
@@ -1523,9 +1526,10 @@ View::PrintToStream() const
 	fScreenClipping.PrintToStream();
 	printf("  valid:            %d\n", fScreenClippingValid);
 	printf("  state:\n");
-	printf("    user clipping:  %p\n", fDrawState->ClippingRegion());
+	printf("    user clipping:  %d\n", fDrawState->HasClipping());
-	printf("    origin:         BPoint(%.1f, %.1f)\n", fDrawState->Origin().x, fDrawState->Origin().y);
+	BPoint origin = fDrawState->CombinedOrigin();
-	printf("    scale:          %.2f\n", fDrawState->Scale());
+	printf("    origin:         BPoint(%.1f, %.1f)\n", origin.x, origin.y);
+	printf("    scale:          %.2f\n", fDrawState->CombinedScale());
 	printf("\n");
 }
 
@@ -1561,17 +1565,17 @@ View::RebuildClipping(bool deep)
 	}
 
 	// add the user clipping in case there is one
-	if (const BRegion* userClipping = fDrawState->ClippingRegion()) {
+	if (fDrawState->HasClipping()) {
 		// NOTE: in case the user sets a user defined clipping region,
 		// rebuilding the clipping is a bit more expensive because there
 		// is no separate "drawing region"... on the other
 		// hand, views for which this feature is actually used will
 		// probably not have any children, so it is not that expensive
 		// after all
-		BRegion* screenUserClipping = fWindow->GetRegion(*userClipping);
+		BRegion* screenUserClipping = fWindow->GetRegion();
 		if (!screenUserClipping)
 			return;
-		fDrawState->Transform(screenUserClipping);
+		fDrawState->GetCombinedClippingRegion(screenUserClipping);
 		fLocalClipping.IntersectWith(screenUserClipping);
 		fWindow->RecycleRegion(screenUserClipping);
 	}