GRAPHICS: Cleanup of ManagedSurface and Screen classes

4 files changed, 54 insertions, 24 deletions

diff --git a/engines/tsage/graphics.cpp b/engines/tsage/graphics.cpp
index 8620498c1e..58fa5b8094 100644
--- a/engines/tsage/graphics.cpp
+++ b/engines/tsage/graphics.cpp
@@ -245,7 +245,7 @@ GfxSurface::GfxSurface(const GfxSurface &s): Graphics::ManagedSurface() {
 GfxSurface::~GfxSurface() {
 	// Sanity check.. GfxSurface should always be just referencing _rawSurface,
 	// and not directly managing it's own surface
-	assert(!isManaged());
+	assert(disposeAfterUse() == DisposeAfterUse::NO);
 }
 
 void GfxSurface::create(uint16 width, uint16 height) {
@@ -287,7 +287,7 @@ void GfxSurface::synchronize(Serializer &s) {
 
 	if (s.isSaving()) {
 		// Save contents of the surface
-		if (isManaged()) {
+		if (disposeAfterUse() == DisposeAfterUse::YES) {
 			s.syncAsSint16LE(this->w);
 			s.syncAsSint16LE(this->h);
 			s.syncBytes((byte *)getPixels(), this->w * this->h);
diff --git a/graphics/managed_surface.cpp b/graphics/managed_surface.cpp
index 45db0bc52e..e493ab9f4e 100644
--- a/graphics/managed_surface.cpp
+++ b/graphics/managed_surface.cpp
@@ -30,30 +30,30 @@ const int SCALE_THRESHOLD = 0x100;
 
 ManagedSurface::ManagedSurface() : 
 		w(_innerSurface.w), h(_innerSurface.h), pitch(_innerSurface.pitch), format(_innerSurface.format),
-		_isManaged(false), _owner(nullptr) {
+		_disposeAfterUse(DisposeAfterUse::NO), _owner(nullptr) {
 }
 
 ManagedSurface::ManagedSurface(const ManagedSurface &surf) :
 		w(_innerSurface.w), h(_innerSurface.h), pitch(_innerSurface.pitch), format(_innerSurface.format),
-		_isManaged(false), _owner(nullptr) {
+		_disposeAfterUse(DisposeAfterUse::NO), _owner(nullptr) {
 	*this = surf;
 }
 
 ManagedSurface::ManagedSurface(int width, int height) :
 		w(_innerSurface.w), h(_innerSurface.h), pitch(_innerSurface.pitch), format(_innerSurface.format),
-		_isManaged(false), _owner(nullptr) {
+		_disposeAfterUse(DisposeAfterUse::NO), _owner(nullptr) {
 	create(width, height);
 }
 
 ManagedSurface::ManagedSurface(int width, int height, const Graphics::PixelFormat &pixelFormat) :
 		w(_innerSurface.w), h(_innerSurface.h), pitch(_innerSurface.pitch), format(_innerSurface.format),
-		_isManaged(false), _owner(nullptr) {
+		_disposeAfterUse(DisposeAfterUse::NO), _owner(nullptr) {
 	create(width, height, format);
 }
 
 ManagedSurface::ManagedSurface(ManagedSurface &surf, const Common::Rect &bounds) :
 		w(_innerSurface.w), h(_innerSurface.h), pitch(_innerSurface.pitch), format(_innerSurface.format),
-		_isManaged(false), _owner(nullptr) {
+		_disposeAfterUse(DisposeAfterUse::NO), _owner(nullptr) {
 	create(surf, bounds);
 }
 
@@ -62,17 +62,18 @@ ManagedSurface::~ManagedSurface() {
 }
 
 ManagedSurface &ManagedSurface::operator=(const ManagedSurface &surf) {
-	if (surf._isManaged) {
+	// Free any current surface
+	free();
+
+	if (surf._disposeAfterUse == DisposeAfterUse::YES) {
 		// Create a new surface and copy the pixels from the source surface
 		create(surf.w, surf.h, surf.format);
 		Common::copy((const byte *)surf.getPixels(), (const byte *)surf.getPixels() +
 			surf.w * surf.h * surf.format.bytesPerPixel, (byte *)this->getPixels());
 	} else {
-		// Source isn't managed, so simply copy it's fields
-		_isManaged = false;
+		// Source isn't managed, so simply copy its fields
 		_owner = surf._owner;
 		_offsetFromOwner = surf._offsetFromOwner;
-		
 		void *srcPixels = (void *)surf._innerSurface.getPixels();
 		_innerSurface.setPixels(srcPixels);
 		_innerSurface.w = surf.w;
@@ -97,7 +98,7 @@ void ManagedSurface::create(uint16 width, uint16 height, const PixelFormat &pixe
 	free();
 	_innerSurface.create(width, height, pixelFormat);
 
-	_isManaged = true;
+	_disposeAfterUse = DisposeAfterUse::YES;
 	markAllDirty();
 }
 
@@ -111,14 +112,14 @@ void ManagedSurface::create(ManagedSurface &surf, const Common::Rect &bounds) {
 	_innerSurface.w = bounds.width();
 	_innerSurface.h = bounds.height();
 	_owner = &surf;
-	_isManaged = false;
+	_disposeAfterUse = DisposeAfterUse::NO;
 }
 
 void ManagedSurface::free() {
-	if (_isManaged)
+	if (_disposeAfterUse == DisposeAfterUse::YES)
 		_innerSurface.free();
 
-	_isManaged = false;
+	_disposeAfterUse = DisposeAfterUse::NO;
 	_owner = nullptr;
 	_offsetFromOwner = Common::Point(0, 0);
 }
@@ -246,6 +247,7 @@ void ManagedSurface::markAllDirty() {
 void ManagedSurface::addDirtyRect(const Common::Rect &r) {
 	if (_owner) {
 		Common::Rect bounds = r;
+		bounds.clip(Common::Rect(0, 0, this->w, this->h));
 		bounds.translate(_offsetFromOwner.x, _offsetFromOwner.y);
 		_owner->addDirtyRect(bounds);
 	}
diff --git a/graphics/managed_surface.h b/graphics/managed_surface.h
index 1d837fd8cd..bd0632a493 100644
--- a/graphics/managed_surface.h
+++ b/graphics/managed_surface.h
@@ -26,6 +26,7 @@
 #include "graphics/pixelformat.h"
 #include "graphics/surface.h"
 #include "common/rect.h"
+#include "common/types.h"
 
 namespace Graphics {
 
@@ -38,9 +39,27 @@ class Font;
 class ManagedSurface {
 	friend class Font;
 private:
+	/**
+	 * The Graphics::Surface that the managed surface encapsulates
+	 */
 	Surface _innerSurface;
-	bool _isManaged;
+
+	/**
+	 * If set, the inner surface will be freed when the surface is recreated,
+	 * as well as when the surface is destroyed
+	 */
+	DisposeAfterUse::Flag _disposeAfterUse;
+
+	/**
+	 * Stores the owning surface if this If this managed surface represents
+	 * a sub-section of another
+	 */
 	ManagedSurface *_owner;
+
+	/**
+	 * For sub-section areas of an owning parent managed surface, this represents
+	 * the offset from the parent's top-left corner this sub-surface starts at
+	 */
 	Common::Point _offsetFromOwner;
 protected:
 	/**
@@ -65,8 +84,10 @@ public:
 	ManagedSurface();
 
 	/**
-	 * Create a managed surface from another one
-	 * Note that if the source has a managed surface, it will be duplicated
+	 * Create a managed surface from another one.
+	 * If the source surface is maintaining it's own surface data, then
+	 * this surface will create it's own surface of the same size and copy
+	 * the contents from the source surface
 	 */
 	ManagedSurface(const ManagedSurface &surf);
 
@@ -111,9 +132,9 @@ public:
 	bool empty() const { return w == 0 || h == 0 || _innerSurface.getPixels() == nullptr; }
 
 	/**
-	 * Returns true if the surface is managing it's own pixels
+	 * Returns true if the surface is managing its own pixels
 	 */
-	bool isManaged() const { return _isManaged; }
+	DisposeAfterUse::Flag disposeAfterUse() const { return _disposeAfterUse; }
 
 	/**
 	 * Return a pointer to the pixel at the specified point.
@@ -304,7 +325,7 @@ public:
 	 */
 	void drawThickLine(int x0, int y0, int x1, int y1, int penX, int penY, uint32 color) {
 		_innerSurface.drawThickLine(x0, y0, x1, y1, penX, penY, color);
-		addDirtyRect(Common::Rect(x0, y0, x1, y1));
+		addDirtyRect(Common::Rect(x0, y0, x1 + penX, y1 + penY));
 	}
 
 	/**
diff --git a/graphics/screen.h b/graphics/screen.h
index 3479ce847e..29816120f1 100644
--- a/graphics/screen.h
+++ b/graphics/screen.h
@@ -33,6 +33,12 @@ namespace Graphics {
 #define PALETTE_COUNT 256
 #define PALETTE_SIZE (256 * 3)
 
+/**
+ * Implements a specialised surface that represents the screen.
+ * It keeps track of any areas of itself that are updated by drawing
+ * calls, and provides an update that method that blits the affected 
+ * areas to the physical screen
+ */
 class Screen : virtual public ManagedSurface {
 private:
 	/**
@@ -61,12 +67,13 @@ public:
 	Screen(int width, int height, PixelFormat pixelFormat);
 
 	/**
-	 * Returns true if there are any pending screen updates
+	 * Returns true if there are any pending screen updates (dirty areas)
 	 */
-	bool isDirty() const { return _dirtyRects.empty(); }
+	bool isDirty() const { return !_dirtyRects.empty(); }
 
 	/**
-	 * Makes the whole screen dirty
+	 * Marks the whole screen as dirty. This forces the next call to update 
+	 * to copy the entire screen contents
 	 */
 	void makeAllDirty();