fixed a crash when trying to open a document for the 3rd time after closing it 2 times

Sat, 04 Aug 2018 21:46:58 +0300

author
Teemu Piippo <teemu@hecknology.net>
date
Sat, 04 Aug 2018 21:46:58 +0300
changeset 1428
ece049033adc
parent 1427
b7ba2af33c13
child 1429
80e8aaabeeed

fixed a crash when trying to open a document for the 3rd time after closing it 2 times

CMakeLists.txt file | annotate | diff | comparison | revisions
src/basics.h file | annotate | diff | comparison | revisions
src/documentmanager.cpp file | annotate | diff | comparison | revisions
src/documentmanager.h file | annotate | diff | comparison | revisions
src/generics/oneof.h file | annotate | diff | comparison | revisions
src/glcompiler.cpp file | annotate | diff | comparison | revisions
src/glrenderer.cpp file | annotate | diff | comparison | revisions
src/main.h file | annotate | diff | comparison | revisions
src/mainwindow.cpp file | annotate | diff | comparison | revisions
src/mainwindow.h file | annotate | diff | comparison | revisions
src/primitives.cpp file | annotate | diff | comparison | revisions
src/toolsets/algorithmtoolset.cpp file | annotate | diff | comparison | revisions
src/toolsets/filetoolset.cpp file | annotate | diff | comparison | revisions
--- a/CMakeLists.txt	Fri Jul 13 20:48:55 2018 +0300
+++ b/CMakeLists.txt	Sat Aug 04 21:46:58 2018 +0300
@@ -155,6 +155,7 @@
 	src/generics/enums.h
 	src/generics/functions.h
 	src/generics/migrate.h
+	src/generics/oneof.h
 	src/generics/range.h
 	src/generics/reverse.h
 	src/generics/ring.h
--- a/src/basics.h	Fri Jul 13 20:48:55 2018 +0300
+++ b/src/basics.h	Sat Aug 04 21:46:58 2018 +0300
@@ -19,6 +19,7 @@
 #pragma once
 #include <cstdio>
 #include <cstdlib>
+#include <memory>
 #include <QDate>
 #include <QFile>
 #include <QLineF>
@@ -98,6 +99,12 @@
 	return vector.length();
 }
 
+template<typename T>
+unsigned int qHash(const std::unique_ptr<T>& pointer)
+{
+	return qHash(pointer.get());
+}
+
 qreal determinant(qreal a, qreal b, qreal c, qreal d);
 qreal determinant(qreal a, qreal b, qreal c, qreal d, qreal e, qreal f, qreal g, qreal h, qreal i);
 qreal determinant(const QMatrix2x2& matrix);
--- a/src/documentmanager.cpp	Fri Jul 13 20:48:55 2018 +0300
+++ b/src/documentmanager.cpp	Sat Aug 04 21:46:58 2018 +0300
@@ -46,12 +46,6 @@
 
 void DocumentManager::clear()
 {
-	for (LDDocument* document : m_documents)
-	{
-		document->close();
-		delete document;
-	}
-
 	m_documents.clear();
 }
 
@@ -59,17 +53,20 @@
 {
 	if (not filename.isEmpty())
 	{
-		LDDocument* doc = findDocumentByName (filename);
+		auto iterator = findDocumentByName (filename);
 
-		if (doc == nullptr)
+		if (iterator == end())
 		{
 			bool tmp = m_loadingMainFile;
 			m_loadingMainFile = false;
-			doc = openDocument (filename, true, true);
+			LDDocument *doc = openDocument(filename, true, true);
 			m_loadingMainFile = tmp;
+			return doc;
 		}
-
-		return doc;
+		else
+		{
+			return iterator->get();
+		}
 	}
 	else
 	{
@@ -79,36 +76,20 @@
 
 void DocumentManager::openMainModel (QString path)
 {
-	// If there's already a file with the same name, this file must replace it.
-	LDDocument* documentToReplace = nullptr;
-	LDDocument* file = nullptr;
-	QString shortName = LDDocument::shortenName (path);
+	// If there's already a file with the same name, this file must replace it. Thus, we cannot open this file if the
+	// document this would replace is not safe to close.
+	auto documentToReplace = findDocumentByName(LDDocument::shortenName(path));
+	if (documentToReplace != end())
+	{
+		if (not (*documentToReplace)->isSafeToClose())
+			return;
 
-	for (LDDocument* doc : m_documents)
-	{
-		if (doc->name() == shortName)
-		{
-			documentToReplace = doc;
-			break;
-		}
+		(*documentToReplace)->close();
+		m_documents.erase(documentToReplace);
 	}
 
-	// We cannot open this file if the document this would replace is not
-	// safe to close.
-	if (documentToReplace and not documentToReplace->isSafeToClose())
-		return;
-
 	m_loadingMainFile = true;
-
-	// If we're replacing an existing document, clear the document and
-	// make it ready for being loaded to.
-	if (documentToReplace)
-	{
-		file = documentToReplace;
-		file->clear();
-	}
-
-	file = openDocument (path, false, false, file);
+	LDDocument* file = openDocument(path, false, false);
 
 	if (file == nullptr)
 	{
@@ -155,18 +136,18 @@
 	}
 }
 
-LDDocument* DocumentManager::findDocumentByName (QString name)
+DocumentManager::iterator DocumentManager::findDocumentByName(const QString& name)
 {
 	if (not name.isEmpty())
 	{
-		for (LDDocument* document : m_documents)
+		for (auto it = begin(); it != end(); ++it)
 		{
-			if (isOneOf (name, document->name(), document->defaultName()))
-				return document;
+			if (name == oneOf((*it)->name(), (*it)->defaultName()))
+				return it;
 		}
 	}
 
-	return nullptr;
+	return end();
 }
 
 QString DocumentManager::findDocument(QString name) const
@@ -192,12 +173,8 @@
 	print(message);
 }
 
-LDDocument* DocumentManager::openDocument(
-	QString path,
-	bool search,
-	bool implicit,
-	LDDocument* fileToOverride
-) {
+LDDocument* DocumentManager::openDocument(QString path, bool search, bool implicit)
+{
 	if (search and not QFileInfo {path}.exists())
 	{
 		// Convert the file name to lowercase when searching because some parts contain subfile
@@ -210,10 +187,7 @@
 
 	if (file.open(QIODevice::ReadOnly))
 	{
-		LDDocument* load = fileToOverride;
-
-		if (fileToOverride == nullptr)
-			load = m_window->newDocument(implicit);
+		LDDocument* load = createNew(implicit);
 
 		// Loading the file shouldn't count as actual edits to the document.
 		load->history()->setIgnoring (true);
@@ -275,9 +249,19 @@
 	m_window->updateRecentFilesMenu();
 }
 
+const DocumentManager::Documents& DocumentManager::allDocuments() const
+{
+	return m_documents;
+}
+
+DocumentManager::Documents::iterator DocumentManager::begin()
+{
+	return m_documents.begin();
+}
+
 bool DocumentManager::isSafeToCloseAll()
 {
-	for (LDDocument* document : m_documents)
+	for (const std::unique_ptr<LDDocument>& document : m_documents)
 	{
 		if (not document->isSafeToClose())
 			return false;
@@ -324,9 +308,14 @@
 	return false;
 }
 
-LDDocument* DocumentManager::createNew()
+LDDocument* DocumentManager::createNew(bool implicit)
 {
-	LDDocument* document = new LDDocument (this);
-	m_documents.insert (document);
-	return document;
+	auto pair = m_documents.emplace(std::make_unique<LDDocument>(this));
+	emit documentCreated(pair.first->get(), implicit);
+	return pair.first->get();
 }
+
+DocumentManager::Documents::iterator DocumentManager::end()
+{
+	return m_documents.end();
+}
--- a/src/documentmanager.h	Fri Jul 13 20:48:55 2018 +0300
+++ b/src/documentmanager.h	Sat Aug 04 21:46:58 2018 +0300
@@ -17,7 +17,7 @@
  */
 
 #pragma once
-#include <QSet>
+#include <set>
 #include "main.h"
 #include "hierarchyelement.h"
 
@@ -28,36 +28,35 @@
 	Q_OBJECT
 
 public:
-	using Documents = QSet<LDDocument*>;
+	using Documents = std::set<std::unique_ptr<LDDocument>>;
+	using iterator = Documents::iterator;
 
 	DocumentManager (QObject* parent = nullptr);
 	~DocumentManager();
 
 	void addRecentFile (QString path);
-	const Documents& allDocuments() const { return m_documents; }
+	const Documents& allDocuments() const;
+	Documents::iterator begin();
 	void clear();
-	LDDocument* createNew();
+	LDDocument* createNew(bool implicit);
+	Documents::iterator end();
 	QString findDocument(QString name) const;
-	LDDocument* findDocumentByName (QString name);
+	iterator findDocumentByName(const QString& name);
 	LDDocument* getDocumentByName (QString filename);
 	bool isSafeToCloseAll();
 	void loadLogoedStuds();
-	LDDocument* openDocument(
-		QString path,
-		bool search,
-		bool implicit,
-		LDDocument* fileToOverride = nullptr
-	);
+	LDDocument* openDocument(QString path, bool search, bool implicit);
 	void openMainModel (QString path);
 	bool preInline (LDDocument* doc, Model& model, bool deep, bool renderinline);
 
 signals:
+	void documentCreated(LDDocument* document, bool cache);
 	void documentClosed(LDDocument* document);
 
 private:
 	Q_SLOT void printParseErrorMessage(QString message);
 
-	Documents m_documents;
+	std::set<std::unique_ptr<LDDocument>> m_documents;
 	bool m_loadingMainFile;
 	bool m_isLoadingLogoedStuds;
 	LDDocument* m_logoedStud;
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/src/generics/oneof.h	Sat Aug 04 21:46:58 2018 +0300
@@ -0,0 +1,65 @@
+#pragma once
+#include <array>
+
+namespace detail
+{
+	template<typename T, typename Vt>
+	constexpr bool oneOfCompare(const T& value, Vt *first, Vt *last)
+	{
+		return (first == last)
+			? false
+			: (*first) == value
+				? true
+				: detail::oneOfCompare(value, first + 1, last);
+	}
+
+	template<typename Vt, std::size_t N>
+	struct OneOfClass
+	{
+		std::array<Vt, N> parameters;
+
+		template<typename... Ts>
+		constexpr OneOfClass(Ts&&... parameters) :
+			parameters {{parameters...}} {}
+
+		template<typename T>
+		constexpr bool operator==(const T& value) const
+		{
+			return detail::oneOfCompare(value, &parameters[0], &parameters[N]);
+		}
+
+		template<typename T>
+		constexpr bool operator!=(const T& value) const
+		{
+			return !(*this == value);
+		}
+
+		template<typename T>
+		constexpr friend bool operator==(const T& value, const OneOfClass<Vt, N>& array)
+		{
+			return array == value;
+		}
+
+		template<typename T>
+		constexpr friend bool operator!=(const T& value, const OneOfClass<Vt, N>& array)
+		{
+			return array != value;
+		}
+	};
+};
+
+/*
+ * Returns such an object that compares equal to some value if and only if any of the parameters do.
+ * Example: (x == any(1, 2, 3)) is equivalent to (x == 1 || x == 2 || x == 3) other than the lack of lazy-evaluation
+ */
+template<typename... Ts>
+constexpr auto oneOf(Ts&&... parameters)
+{
+	using Vt = typename std::common_type<Ts...>::type;
+	return detail::OneOfClass<Vt, sizeof...(Ts)>({parameters...});
+}
+
+static_assert(oneOf(1,2,3) == 3, "oneOf unit test");
+static_assert(oneOf(1,2,3) != 5, "oneOf unit test");
+static_assert(3 == oneOf(1,2,3), "oneOf unit test");
+static_assert(5 != oneOf(1,2,3), "oneOf unit test");
--- a/src/glcompiler.cpp	Fri Jul 13 20:48:55 2018 +0300
+++ b/src/glcompiler.cpp	Sat Aug 04 21:46:58 2018 +0300
@@ -571,8 +571,10 @@
 
 void GLCompiler::handleRowRemoval(const QModelIndex&, int first, int last)
 {
-	for (int row = last; row >= first; row -= 1)
-		forgetObject(m_renderer->model()->index(row));
+	for (int row = last; row >= first; row -= 1) {
+		auto index = m_renderer->model()->index(row);
+		forgetObject(index);
+	}
 
 	this->needBoundingBoxRebuild = true;
 	emit sceneChanged();
--- a/src/glrenderer.cpp	Fri Jul 13 20:48:55 2018 +0300
+++ b/src/glrenderer.cpp	Sat Aug 04 21:46:58 2018 +0300
@@ -64,6 +64,7 @@
         {"Free camera", GLCamera::FreeCamera}, // free
     }
 {
+	Q_ASSERT(model != nullptr);
 	m_camera = (Camera) config::camera();
 	m_compiler = new GLCompiler (this);
 	m_toolTipTimer = new QTimer (this);
--- a/src/main.h	Fri Jul 13 20:48:55 2018 +0300
+++ b/src/main.h	Sat Aug 04 21:46:58 2018 +0300
@@ -27,4 +27,5 @@
 #include "types/library.h"
 #include "configuration.h"
 #include "generics/range.h"
+#include "generics/oneof.h"
 #include "types/vertex.h"
--- a/src/mainwindow.cpp	Fri Jul 13 20:48:55 2018 +0300
+++ b/src/mainwindow.cpp	Sat Aug 04 21:46:58 2018 +0300
@@ -86,6 +86,7 @@
 
 	connect (m_tabs, SIGNAL (currentChanged(int)), this, SLOT (tabSelected()));
 	connect (m_tabs, SIGNAL (tabCloseRequested (int)), this, SLOT (closeTab (int)));
+	connect(m_documents, &DocumentManager::documentCreated, this, &MainWindow::newDocument);
 	connect(m_documents, SIGNAL(documentClosed(LDDocument*)), this, SLOT(documentClosed(LDDocument*)));
 
 	m_quickColors = m_guiUtilities->loadQuickColorList();
@@ -613,14 +614,14 @@
 	while (m_tabs->count() > 0)
 		m_tabs->removeTab (0);
 
-	for (LDDocument* document : m_documents->allDocuments())
+	for (auto& document : *m_documents)
 	{
 		if (not document->isFrozen())
 		{
 			// Add an item to the list for this file and store the tab index
 			// in the document so we can find documents by tab index.
 			document->setTabIndex (m_tabs->addTab (""));
-			updateDocumentListItem (document);
+			updateDocumentListItem(document.get());
 		}
 	}
 
@@ -671,11 +672,11 @@
 	int tabIndex = m_tabs->currentIndex();
 
 	// Find the file pointer of the item that was selected.
-	for (LDDocument* document : m_documents->allDocuments())
+	for (auto& document : *m_documents)
 	{
 		if (not document->isFrozen() and document->tabIndex() == tabIndex)
 		{
-			switchee = document;
+			switchee = document.get();
 			break;
 		}
 	}
@@ -721,6 +722,7 @@
 //
 Canvas* MainWindow::renderer()
 {
+	Q_ASSERT(ui.rendererStack->count() > 0);
 	return static_cast<Canvas*>(ui.rendererStack->currentWidget());
 }
 
@@ -736,10 +738,10 @@
 //
 void MainWindow::closeTab (int tabindex)
 {
-	LDDocument* doc = m_documents->findDocumentByName (m_tabs->tabData (tabindex).toString());
+	auto iterator = m_documents->findDocumentByName(m_tabs->tabData (tabindex).toString());
 
-	if (doc)
-		doc->close();
+	if (iterator != m_documents->end())
+		(*iterator)->close();
 }
 
 // ---------------------------------------------------------------------------------------------------------------------
@@ -804,26 +806,23 @@
 void MainWindow::createBlankDocument()
 {
 	// Create a new anonymous file and set it to our current
-	LDDocument* f = newDocument();
-	f->setName ("");
-	changeDocument (f);
-	doFullRefresh();
+	LDDocument* document = m_documents->createNew(false);
+	document->setName ("");
+	document->setFrozen(false);
+	changeDocument (document);
 	updateActions();
 }
 
 // ---------------------------------------------------------------------------------------------------------------------
 //
-LDDocument* MainWindow::newDocument (bool cache)
+void MainWindow::newDocument(LDDocument* document, bool cache)
 {
-	LDDocument* document = m_documents->createNew();
 	connect (document->history(), SIGNAL (undone()), this, SLOT (historyTraversed()));
 	connect (document->history(), SIGNAL (redone()), this, SLOT (historyTraversed()));
 	connect (document->history(), SIGNAL (stepAdded()), this, SLOT (updateActions()));
 
 	if (not cache)
 		openDocumentForEditing(document);
-
-	return document;
 }
 
 void MainWindow::openDocumentForEditing(LDDocument* document)
@@ -945,11 +944,11 @@
 	LDDocument* old = currentDocument();
 
 	// Find a replacement document to use
-	for (LDDocument* doc : m_documents->allDocuments())
+	for (auto &iterator : m_documents->allDocuments())
 	{
-		if (doc != old and not doc->isFrozen())
+		if (iterator.get() != old and not iterator->isFrozen())
 		{
-			changeDocument (doc);
+			changeDocument(iterator.get());
 			break;
 		}
 	}
--- a/src/mainwindow.h	Fri Jul 13 20:48:55 2018 +0300
+++ b/src/mainwindow.h	Sat Aug 04 21:46:58 2018 +0300
@@ -77,7 +77,6 @@
 	Grid* grid();
 	class GuiUtilities* guiUtilities();
 	void loadShortcuts();
-	LDDocument* newDocument (bool cache = false);
 	void openDocumentForEditing(LDDocument* document);
 	PrimitiveManager* primitives();
 	Canvas* renderer();
@@ -112,6 +111,7 @@
 	void tabSelected();
 	void documentClosed(LDDocument* document);
 	void updateTitle();
+	void newDocument (LDDocument* document, bool cache = false);
 
 protected:
 	void closeEvent (QCloseEvent* event);
--- a/src/primitives.cpp	Fri Jul 13 20:48:55 2018 +0300
+++ b/src/primitives.cpp	Sat Aug 04 21:46:58 2018 +0300
@@ -622,7 +622,7 @@
 	else if (spec.divisions == LowResolution)
 		description.insert (0, "Lo-Res ");
 
-	LDDocument* document = m_window->newDocument();
+	LDDocument* document = m_documents->createNew(false);
 	document->setDefaultName(fileName);
 
 	if (not config::defaultName().isEmpty())
--- a/src/toolsets/algorithmtoolset.cpp	Fri Jul 13 20:48:55 2018 +0300
+++ b/src/toolsets/algorithmtoolset.cpp	Sat Aug 04 21:46:58 2018 +0300
@@ -569,13 +569,13 @@
 			fullSubfilePath = subfileDirectory.filePath(subfileName);
 			subfileIndex += 1;
 		} while (
-			m_documents->findDocumentByName("s\\" + subfileName) != nullptr
+			m_documents->findDocumentByName("s\\" + subfileName) != m_documents->end()
 			or QFileInfo {fullSubfilePath}.exists()
 		);
 	}
 
 	// Create the new subfile document
-	LDDocument* subfile = m_window->newDocument();
+	LDDocument* subfile = m_documents->createNew(false);
 	subfile->setFullPath(fullSubfilePath);
 	subfile->header.description = subfileTitle;
 	subfile->header.type = LDHeader::Subpart;
--- a/src/toolsets/filetoolset.cpp	Fri Jul 13 20:48:55 2018 +0300
+++ b/src/toolsets/filetoolset.cpp	Sat Aug 04 21:46:58 2018 +0300
@@ -72,8 +72,8 @@
 
 void FileToolset::saveAll()
 {
-	for (LDDocument* document : m_documents->allDocuments())
-		m_window->save(document, false);
+	for (auto& document : *m_documents)
+		m_window->save(document.get(), false);
 }
 
 void FileToolset::close()

mercurial