Merge pull request #1245 from Febbe/master

Fixed Memory leak in BackgroundImage #1198

src/model/BackgroundImage.cpp

@@ -1,185 +1,153 @@
 #include "BackgroundImage.h"
 
-#include "BackgroundImageContents.h"
-#include <Stacktrace.h>
+#include "Stacktrace.h"
 
-BackgroundImage::BackgroundImage()
-{
-	XOJ_INIT_TYPE(BackgroundImage);
-}
+/*
+ * The contents of a background image
+ *
+ * Internal impl object, dont move this to an external header/source file due this is the best way to reduce code
+ * bloat and increase encapsulation. This object is only used in this source scope and is a RAII Container for the
+ * GdkPixbuf*
+ * No xournal memory leak tests necessary, because we use smart ptrs to ensure memory correctness
+ */
 
-BackgroundImage::BackgroundImage(const BackgroundImage& img)
+struct BackgroundImage::Content
 {
-	XOJ_INIT_TYPE(BackgroundImage);
-	XOJ_CHECK_TYPE_OBJ((&img), BackgroundImage);
-
-	this->img = img.img;
-	if (this->img)
+	Content(string filename, GError** error)
+			: filename(std::move(filename)), pixbuf(gdk_pixbuf_new_from_file(this->filename.c_str(), error))
 	{
-		this->img->reference();
 	}
-}
-
-BackgroundImage::~BackgroundImage()
-{
-	XOJ_CHECK_TYPE(BackgroundImage);
 
-	if (this->img)
+	Content(GInputStream* stream, string filename, GError** error)
+			: filename(std::move(filename)), pixbuf(gdk_pixbuf_new_from_stream(stream, nullptr, error))
 	{
-		this->img->unreference();
 	}
-	this->img = NULL;
 
-	XOJ_RELEASE_TYPE(BackgroundImage);
-}
+	~Content()
+	{
+		g_object_unref(this->pixbuf);
+		this->pixbuf = nullptr;
+	};
 
-string BackgroundImage::getFilename()
-{
-	XOJ_CHECK_TYPE(BackgroundImage);
+	Content(const Content&) = delete;
+	Content(Content&&) = default;
+	Content& operator=(const Content&) = delete;
+	Content& operator=(Content&&) = default;
 
-	if (this->img != NULL)
-	{
-		return this->img->getFilename();
-	}
-	return "";
-}
+	string filename;
+	GdkPixbuf* pixbuf = nullptr;
+	int pageId = -1;
+	bool attach = false;
+};
 
-void BackgroundImage::loadFile(string filename, GError** error)
+BackgroundImage::BackgroundImage()
 {
-	XOJ_CHECK_TYPE(BackgroundImage);
-
-	if (this->img != NULL)
-	{
-		this->img->unreference();
-	}
-	this->img = new BackgroundImageContents(filename, error);
+	XOJ_INIT_TYPE(BackgroundImage);
 }
 
-void BackgroundImage::loadFile(GInputStream* stream, string filename, GError** error)
+BackgroundImage::BackgroundImage(const BackgroundImage& img) : img(img.img)
 {
-	XOJ_CHECK_TYPE(BackgroundImage);
-
-	if (this->img != NULL)
-	{
-		this->img->unreference();
-	}
-	this->img = new BackgroundImageContents(stream, filename, error);
+	XOJ_INIT_TYPE(BackgroundImage);
+	XOJ_CHECK_TYPE_OBJ((&img), BackgroundImage);
 }
 
-void BackgroundImage::setAttach(bool attach)
+BackgroundImage::BackgroundImage(BackgroundImage&& img) noexcept : img(std::move(img.img))
 {
-	XOJ_CHECK_TYPE(BackgroundImage);
-
-	if (this->img != NULL)
-	{
-		this->img->setAttach(true);
-	}
-	else
-	{
-		g_warning("BackgroundImage::setAttach:please load first an image before call setAttach!");
-		Stacktrace::printStracktrace();
-	}
+	XOJ_INIT_TYPE(BackgroundImage);
+	XOJ_CHECK_TYPE_OBJ((&img), BackgroundImage);
 }
 
-void BackgroundImage::operator=(BackgroundImage& img)
+BackgroundImage::~BackgroundImage()
 {
 	XOJ_CHECK_TYPE(BackgroundImage);
-
-	if (this->img)
-	{
-		this->img->unreference();
-	}
-	this->img = img.img;
-	if (this->img)
-	{
-		this->img->reference();
-	}
+	XOJ_RELEASE_TYPE(BackgroundImage);
 }
 
 bool BackgroundImage::operator==(const BackgroundImage& img)
 {
 	XOJ_CHECK_TYPE(BackgroundImage);
-
 	return this->img == img.img;
 }
 
 void BackgroundImage::free()
 {
 	XOJ_CHECK_TYPE(BackgroundImage);
-
-	if (this->img)
-	{
-		this->img->unreference();
-	}
-	this->img = NULL;
+	this->img.reset();
 }
 
-void BackgroundImage::clearSaveState()
+void BackgroundImage::loadFile(string filename, GError** error)
 {
 	XOJ_CHECK_TYPE(BackgroundImage);
+	this->img = std::make_shared<Content>(std::move(filename), error);
+}
 
-	if (this->img)
-	{
-		this->img->setPageId(-1);
-	}
+void BackgroundImage::loadFile(GInputStream* stream, string filename, GError** error)
+{
+	XOJ_CHECK_TYPE(BackgroundImage);
+	this->img = std::make_shared<Content>(stream, std::move(filename), error);
 }
 
 int BackgroundImage::getCloneId()
 {
 	XOJ_CHECK_TYPE(BackgroundImage);
-
-	if (this->img)
-	{
-		return this->img->getPageId();
-	}
-	return -1;
+	return this->img ? this->img->pageId : -1;
 }
 
 void BackgroundImage::setCloneId(int id)
 {
 	XOJ_CHECK_TYPE(BackgroundImage);
-
 	if (this->img)
 	{
-		this->img->setPageId(id);
+		this->img->pageId = id;
 	}
 }
 
-void BackgroundImage::setFilename(string filename)
+void BackgroundImage::clearSaveState()
+{
+	this->setCloneId(-1);
+}
+
+string BackgroundImage::getFilename()
 {
 	XOJ_CHECK_TYPE(BackgroundImage);
+	return this->img ? this->img->filename : "";
+}
 
+void BackgroundImage::setFilename(string filename)
+{
+	XOJ_CHECK_TYPE(BackgroundImage);
 	if (this->img)
 	{
-		this->img->setFilename(filename);
+		this->img->filename = std::move(filename);
 	}
 }
 
 bool BackgroundImage::isAttached()
 {
 	XOJ_CHECK_TYPE(BackgroundImage);
+	return this->img ? this->img->attach : false;
+}
 
-	if (this->img)
+void BackgroundImage::setAttach(bool attach)
+{
+	XOJ_CHECK_TYPE(BackgroundImage);
+	if (!this->img)
 	{
-		return this->img->isAttach();
+		g_warning("BackgroundImage::setAttach: please load first an image before call setAttach!");
+		Stacktrace::printStracktrace();
+		return;
 	}
-	return false;
+	this->img->attach = attach;
 }
 
-bool BackgroundImage::isEmpty()
+GdkPixbuf* BackgroundImage::getPixbuf()
 {
 	XOJ_CHECK_TYPE(BackgroundImage);
-
-	return this->img == NULL;
+	return this->img ? this->img->pixbuf : nullptr;
 }
 
-GdkPixbuf* BackgroundImage::getPixbuf()
+bool BackgroundImage::isEmpty()
 {
 	XOJ_CHECK_TYPE(BackgroundImage);
-
-	if (this->img)
-	{
-		return this->img->getPixbuf();
-	}
-	return NULL;
+	return !this->img;
 }

src/model/BackgroundImage.h

@@ -11,44 +11,47 @@
 
 #pragma once
 
-#include <XournalType.h>
+#include "XournalType.h"
 
 #include <gtk/gtk.h>
 
-class BackgroundImageContents;
+#include <memory>
 
-class BackgroundImage
+struct BackgroundImage
 {
-public:
 	BackgroundImage();
 	BackgroundImage(const BackgroundImage& img);
-	virtual ~BackgroundImage();
+	BackgroundImage(BackgroundImage&& img) noexcept;
+	~BackgroundImage();
 
-public:
-	string getFilename();
-	void loadFile(string filename, GError** error);
-	void loadFile(GInputStream* stream, string filename, GError** error);
+	BackgroundImage& operator=(const BackgroundImage& img) = default;
+	BackgroundImage& operator=(BackgroundImage&& img) = default;
 
-	void setAttach(bool attach);
 
-	void operator=(BackgroundImage& img);
 	bool operator==(const BackgroundImage& img);
 
 	void free();
 
-	void clearSaveState();
+	void loadFile(string filename, GError** error);
+	void loadFile(GInputStream* stream, string filename, GError** error);
+
 	int getCloneId();
 	void setCloneId(int id);
+	void clearSaveState();
 
+	string getFilename();
 	void setFilename(string filename);
 
 	bool isAttached();
-	bool isEmpty();
+	void setAttach(bool attach);
 
 	GdkPixbuf* getPixbuf();
 
+	bool isEmpty();
+
 private:
-	XOJ_TYPE_ATTRIB;
+	struct Content;
 
-	BackgroundImageContents* img = NULL;
+	XOJ_TYPE_ATTRIB;
+	std::shared_ptr<Content> img;
 };

src/model/BackgroundImageContents.cpp

@@ -1,94 +0,0 @@
-#include "BackgroundImageContents.h"
-
-BackgroundImageContents::BackgroundImageContents(string filename, GError** error)
-{
-	XOJ_INIT_TYPE(BackgroundImageContents);
-
-	this->filename = filename;
-	this->pixbuf = gdk_pixbuf_new_from_file(filename.c_str(), error);
-}
-
-BackgroundImageContents::BackgroundImageContents(GInputStream* stream, string filename, GError** error)
-{
-	XOJ_INIT_TYPE(BackgroundImageContents);
-
-	this->filename = filename;
-	this->pixbuf = gdk_pixbuf_new_from_stream(stream, nullptr, error);
-}
-
-BackgroundImageContents::~BackgroundImageContents()
-{
-	XOJ_CHECK_TYPE(BackgroundImageContents);
-
-	g_object_unref(this->pixbuf);
-	this->pixbuf = NULL;
-
-	XOJ_RELEASE_TYPE(BackgroundImageContents);
-}
-
-void BackgroundImageContents::unreference()
-{
-	XOJ_CHECK_TYPE(BackgroundImageContents);
-
-	this->ref--;
-	if (this->ref < 0)
-	{
-		delete this;
-	}
-}
-
-void BackgroundImageContents::reference()
-{
-	XOJ_CHECK_TYPE(BackgroundImageContents);
-
-	this->ref++;
-}
-
-string BackgroundImageContents::getFilename()
-{
-	XOJ_CHECK_TYPE(BackgroundImageContents);
-
-	return this->filename;
-}
-
-void BackgroundImageContents::setFilename(string filename)
-{
-	XOJ_CHECK_TYPE(BackgroundImageContents);
-
-	this->filename = filename;
-}
-
-bool BackgroundImageContents::isAttach()
-{
-	XOJ_CHECK_TYPE(BackgroundImageContents);
-
-	return this->attach;
-}
-
-void BackgroundImageContents::setAttach(bool attach)
-{
-	XOJ_CHECK_TYPE(BackgroundImageContents);
-
-	this->attach = attach;
-}
-
-int BackgroundImageContents::getPageId()
-{
-	XOJ_CHECK_TYPE(BackgroundImageContents);
-
-	return this->pageId;
-}
-
-void BackgroundImageContents::setPageId(int id)
-{
-	XOJ_CHECK_TYPE(BackgroundImageContents);
-
-	this->pageId = id;
-}
-
-GdkPixbuf* BackgroundImageContents::getPixbuf()
-{
-	XOJ_CHECK_TYPE(BackgroundImageContents);
-
-	return this->pixbuf;
-}

src/model/BackgroundImageContents.h

@@ -1,64 +0,0 @@
-/*
- * Xournal++
- *
- * The contents of a background image
- *
- * @author Xournal++ Team
- * https://github.com/xournalpp/xournalpp
- *
- * @license GNU GPLv2 or later
- */
-
-#pragma once
-
-#include <XournalType.h>
-
-#include <gtk/gtk.h>
-
-class BackgroundImageContents
-{
-public:
-	BackgroundImageContents(string filename, GError** error);
-	BackgroundImageContents(GInputStream* stream, string filename, GError** error);
-
-private:
-	BackgroundImageContents();
-	BackgroundImageContents(const BackgroundImageContents& contents);
-	void operator=(const BackgroundImageContents& contents);
-
-private:
-	virtual ~BackgroundImageContents();
-
-public:
-	void unreference();
-	void reference();
-
-public:
-	string getFilename();
-	void setFilename(string filename);
-
-	bool isAttach();
-	void setAttach(bool attach);
-
-	int getPageId();
-	void setPageId(int id);
-
-	GdkPixbuf* getPixbuf();
-
-private:
-	XOJ_TYPE_ATTRIB;
-
-	/**
-	 * Reference counter
-	 */
-	int ref = 1;
-
-	string filename;
-	bool attach = false;
-
-	/**
-	 *
-	 */
-	int pageId = -1;
-	GdkPixbuf* pixbuf = NULL;
-};