From c97a332a8a42cf365819048ae4514738d087c53e Mon Sep 17 00:00:00 2001 From: Alexandre Quessy Date: Sun, 24 May 2026 10:40:46 -0400 Subject: [PATCH] Fix crash-prone weak pointers, atomic save, and misc cleanup - Guard all toStrongRef() calls with null checks to prevent crashes - Save project files atomically (write to .tmp, then rename) - Fix macOS CI workflow typo that broke test discovery - Remove duplicate main.qrc entry in mapmap.pro - Upgrade C++ standard from c++11 to c++17 - Replace NULL with nullptr throughout our code - Remove stale FIXME comment and duplicate #include --- .github/workflows/macos-build.yml | 2 +- mapmap.pro | 5 ++--- src/app/main.cpp | 1 - src/control/OscInterface.h | 1 - src/core/Commands.cpp | 8 ++++++-- src/gui/ConsoleWindow.cpp | 4 ++-- src/gui/LayerGui.cpp | 6 +++--- src/gui/MainWindow.cpp | 28 +++++++++++++++++++------ src/gui/ShapeGraphicsItem.cpp | 34 ++++++++++++++++++++----------- 9 files changed, 58 insertions(+), 31 deletions(-) diff --git a/.github/workflows/macos-build.yml b/.github/workflows/macos-build.yml index a15c74ad..c02c708e 100644 --- a/.github/workflows/macos-build.yml +++ b/.github/workflows/macos-build.yml @@ -31,7 +31,7 @@ jobs: - name: Build and run tests run: | - export PATH="($brew --prefix=qt)/bin:$PATH" + export PATH="$(brew --prefix qt)/bin:$PATH" cd tests/ qmake6 tests.pro make -j$(sysctl -n hw.logicalcpu) diff --git a/mapmap.pro b/mapmap.pro index 4ce4515b..4444fe50 100644 --- a/mapmap.pro +++ b/mapmap.pro @@ -1,6 +1,6 @@ CONFIG += qt CONFIG += debug -CONFIG += c++11 +CONFIG += c++17 TEMPLATE = app @@ -27,8 +27,7 @@ RESOURCES = \ main.qrc \ translations/translation.qrc \ docs/documentation.qrc \ - resources/interface.qrc \ - main.qrc # Main resource file + resources/interface.qrc # Manage lrelease (for translations) isEmpty(QMAKE_LRELEASE) { diff --git a/src/app/main.cpp b/src/app/main.cpp index c5efc82e..d68f0944 100644 --- a/src/app/main.cpp +++ b/src/app/main.cpp @@ -14,7 +14,6 @@ #include "MetaObjectRegistry.h" #include -#include MM_USE_NAMESPACE diff --git a/src/control/OscInterface.h b/src/control/OscInterface.h index 5b7d1df0..36c73f60 100644 --- a/src/control/OscInterface.h +++ b/src/control/OscInterface.h @@ -41,7 +41,6 @@ class OscInterface { public: typedef QSharedPointer ptr; - // FIXME: change listen_port to a int OscInterface(int listen_port); ~OscInterface(); diff --git a/src/core/Commands.cpp b/src/core/Commands.cpp index 4d3df57f..7086ad46 100644 --- a/src/core/Commands.cpp +++ b/src/core/Commands.cpp @@ -95,12 +95,16 @@ TransformShapeCommand::TransformShapeCommand(MapperGLCanvas* canvas, TransformSh _option = option; // Clone shape before applying transform. - _originalShape.reset(_shape.toStrongRef()->clone()); + auto strong = _shape.toStrongRef(); + if (strong) + _originalShape.reset(strong->clone()); } void TransformShapeCommand::undo() { // Copy back shape. - _shape.toStrongRef()->copyFrom(*_originalShape); + auto strong = _shape.toStrongRef(); + if (!strong) return; + strong->copyFrom(*_originalShape); // Update everything. _canvas->currentShapeWasChanged(); diff --git a/src/gui/ConsoleWindow.cpp b/src/gui/ConsoleWindow.cpp index e22ccea0..bd12036e 100644 --- a/src/gui/ConsoleWindow.cpp +++ b/src/gui/ConsoleWindow.cpp @@ -22,7 +22,7 @@ namespace mmp { -ConsoleWindow* ConsoleWindow::instance = NULL; +ConsoleWindow* ConsoleWindow::instance = nullptr; ConsoleWindow::ConsoleWindow(QWidget *parent) : QMainWindow(parent) { @@ -142,7 +142,7 @@ void ConsoleWindow::kill() { if (instance) { delete instance; - instance = NULL; + instance = nullptr; } } diff --git a/src/gui/LayerGui.cpp b/src/gui/LayerGui.cpp index 024aad8b..aa18ad23 100644 --- a/src/gui/LayerGui.cpp +++ b/src/gui/LayerGui.cpp @@ -25,8 +25,8 @@ namespace mmp { LayerGui::LayerGui(Layer::ptr layer) : _layer(layer), - _graphicsItem(NULL), - _inputGraphicsItem(NULL) + _graphicsItem(nullptr), + _inputGraphicsItem(nullptr) { outputShape = layer->getShape(); Q_CHECK_PTR(outputShape); @@ -275,7 +275,7 @@ EllipseColorLayerGui::EllipseColorLayerGui(Layer::ptr layer) : ColorLayerGui(lay TextureLayerGui::TextureLayerGui(QSharedPointer mapping) : LayerGui(mapping), - _meshItem(NULL) + _meshItem(nullptr) { // Assign members pointers. textureLayer = qSharedPointerCast(_layer); diff --git a/src/gui/MainWindow.cpp b/src/gui/MainWindow.cpp index b47d3cef..ea0dd0ce 100644 --- a/src/gui/MainWindow.cpp +++ b/src/gui/MainWindow.cpp @@ -54,7 +54,7 @@ MainWindow::MainWindow() // TODO: not sure we need this anymore since we have NULL_UID _hasCurrentSource = false; _hasCurrentLayer = false; - currentSelectedItem = NULL; + currentSelectedItem = nullptr; // Frames per second. _framesPerSecond = (-1); @@ -2225,7 +2225,7 @@ void MainWindow::startFullScreen() void MainWindow::createMenus() { - QMenuBar *menuBar = NULL; + QMenuBar *menuBar = nullptr; #ifdef __MACOSX_CORE__ menuBar = new QMenuBar(0); @@ -2624,7 +2624,9 @@ bool MainWindow::loadFile(const QString &fileName) bool MainWindow::saveFile(const QString &fileName) { - QFile file(fileName); + // Write to a temporary file first, then rename for atomic save. + QString tmpFileName = fileName + ".tmp"; + QFile file(tmpFileName); if (! file.open(QFile::WriteOnly | QFile::Text)) { QMessageBox::warning(this, tr("Error saving mapping project"), @@ -2637,12 +2639,26 @@ bool MainWindow::saveFile(const QString &fileName) ProjectWriter writer(this); if (writer.writeFile(&file)) { + file.close(); + // Remove original and rename temp file over it. + QFile::remove(fileName); + if (!QFile::rename(tmpFileName, fileName)) + { + QMessageBox::warning(this, tr("Error saving mapping project"), + tr("Cannot rename temporary file to %1.") + .arg(fileName)); + return false; + } setCurrentFile(fileName); statusBar()->showMessage(tr("File saved"), 2000); return true; } else + { + file.close(); + QFile::remove(tmpFileName); return false; + } } void MainWindow::setCurrentFile(const QString &fileName) @@ -2972,7 +2988,7 @@ void MainWindow::addSourceItem(uid sourceId, const QIcon& icon, const QString& n void MainWindow::updateSourceItem(uid sourceId, const QIcon& icon, const QString& name) { QListWidgetItem* item = getItemFromId(*sourceList, sourceId); - if (item == NULL) { + if (item == nullptr) { // FIXME there was an assert that seemed to make MapMap crash, here. return; } @@ -3196,7 +3212,7 @@ void MainWindow::removeSourceItem(uid sourceId) Q_ASSERT( row >= 0 ); QListWidgetItem* item = sourceList->takeItem(row); if (item == currentSelectedItem) - currentSelectedItem = NULL; + currentSelectedItem = nullptr; delete item; // Update list. @@ -3601,7 +3617,7 @@ QListWidgetItem* MainWindow::getItemFromId(const QListWidget& list, uid id) { if (row >= 0) return list.item( row ); else - return NULL; + return nullptr; } int MainWindow::getItemRowFromId(const QListWidget& list, uid id) diff --git a/src/gui/ShapeGraphicsItem.cpp b/src/gui/ShapeGraphicsItem.cpp index a579739e..3f4aeb3f 100644 --- a/src/gui/ShapeGraphicsItem.cpp +++ b/src/gui/ShapeGraphicsItem.cpp @@ -136,8 +136,9 @@ PolygonColorGraphicsItem::PolygonColorGraphicsItem(Layer::ptr mapping, bool outp QPainterPath PolygonColorGraphicsItem::shape() const { QPainterPath path; - Polygon* poly = static_cast(_shape.toStrongRef().data()); - Q_ASSERT(poly); + auto strong = _shape.toStrongRef(); + if (!strong) return path; + Polygon* poly = static_cast(strong.data()); path.addPolygon(poly->toPolygon()); return mapFromScene(path); } @@ -146,8 +147,9 @@ void PolygonColorGraphicsItem::_doPaint(QPainter *painter, const QStyleOptionGraphicsItem *option) { Q_UNUSED(option); - Polygon* poly = static_cast(_shape.toStrongRef().data()); - Q_ASSERT(poly); + auto strong = _shape.toStrongRef(); + if (!strong) return; + Polygon* poly = static_cast(strong.data()); painter->drawPolygon(mapFromScene(poly->toPolygon())); } @@ -162,7 +164,9 @@ void MeshColorGraphicsItem::_doPaint(QPainter *painter, { Q_UNUSED(option); - Mesh* mesh = static_cast(_shape.toStrongRef().data()); + auto strong = _shape.toStrongRef(); + if (!strong) return; + Mesh* mesh = static_cast(strong.data()); QVector > quads = mesh->getQuads2d(); // Go through the mesh quad by quad. @@ -186,8 +190,9 @@ QPainterPath EllipseColorGraphicsItem::shape() const { // Create path for ellipse. QPainterPath path; - Ellipse* ellipse = static_cast(_shape.toStrongRef().data()); - Q_ASSERT(ellipse); + auto strong = _shape.toStrongRef(); + if (!strong) return path; + Ellipse* ellipse = static_cast(strong.data()); QTransform transform; transform.translate(ellipse->getCenter().x(), ellipse->getCenter().y()); transform.rotate(ellipse->getRotation()); @@ -313,14 +318,17 @@ void TextureGraphicsItem::_postPaint(QPainter* painter, QSharedPointer TextureGraphicsItem::_getTexture() { - return qSharedPointerCast(_textureMapping.toStrongRef()->getSource()); + auto strong = _textureMapping.toStrongRef(); + if (!strong) return QSharedPointer(); + return qSharedPointerCast(strong->getSource()); } QPainterPath PolygonTextureGraphicsItem::shape() const { QPainterPath path; - Polygon* poly = static_cast(_shape.toStrongRef().data()); - Q_ASSERT(poly); + auto strong = _shape.toStrongRef(); + if (!strong) return path; + Polygon* poly = static_cast(strong.data()); path.addPolygon(poly->toPolygon()); return mapFromScene(path); } @@ -335,6 +343,7 @@ void TriangleTextureGraphicsItem::_doDrawOutput(QPainter* painter) if (isOutput()) { MShape::ptr inputShape = _inputShape.toStrongRef(); + if (!inputShape) return; glBegin(GL_TRIANGLES); { for (int i=0; inVertices(); i++) @@ -563,8 +572,9 @@ QPainterPath EllipseTextureGraphicsItem::shape() const { // Create path for ellipse. QPainterPath path; - Ellipse* ellipse = static_cast(_shape.toStrongRef().data()); - Q_ASSERT(ellipse); + auto strong = _shape.toStrongRef(); + if (!strong) return path; + Ellipse* ellipse = static_cast(strong.data()); QTransform transform; transform.translate(ellipse->getCenter().x(), ellipse->getCenter().y()); transform.rotate(ellipse->getRotation());