From 6ae221e810bbbbac49879100e5abcdc6fab9382c Mon Sep 17 00:00:00 2001 From: Tadej Novak Date: Tue, 14 Nov 2023 21:14:51 +0100 Subject: [PATCH] Consolidate smart pointer usage --- src/core/map.cpp | 30 ++++++++-------- src/core/map.hpp | 2 +- src/core/map_observer.cpp | 6 ++-- src/core/map_observer_p.hpp | 2 +- src/core/settings.cpp | 65 ++++++++++++++++------------------- src/core/settings.hpp | 6 ++-- src/core/settings_p.hpp | 6 +++- src/location/qgeomap.cpp | 12 +++---- src/location/texture_node.cpp | 8 ++--- src/widgets/gl_widget.cpp | 11 +++--- src/widgets/gl_widget.hpp | 2 +- test/widgets/gl_tester.cpp | 8 ++--- test/widgets/gl_tester.hpp | 6 ++-- 13 files changed, 81 insertions(+), 83 deletions(-) diff --git a/src/core/map.cpp b/src/core/map.cpp index 7f6539e..b7a10e1 100644 --- a/src/core/map.cpp +++ b/src/core/map.cpp @@ -128,7 +128,7 @@ mbgl::MapOptions mapOptionsFromSettings(const QMapLibre::Settings &settings, con } mbgl::ResourceOptions resourceOptionsFromSettings(const QMapLibre::Settings &settings) { - if (settings.tileServerOptions() == nullptr) { + if (!settings.customTileServerOptions()) { return std::move(mbgl::ResourceOptions() .withAssetPath(settings.assetPath().toStdString()) .withCachePath(settings.cacheDatabasePath().toStdString()) @@ -138,7 +138,7 @@ mbgl::ResourceOptions resourceOptionsFromSettings(const QMapLibre::Settings &set return std::move(mbgl::ResourceOptions() .withApiKey(settings.apiKey().toStdString()) .withAssetPath(settings.assetPath().toStdString()) - .withTileServerOptions(*settings.tileServerOptions()) + .withTileServerOptions(settings.tileServerOptions()) .withCachePath(settings.cacheDatabasePath().toStdString()) .withMaximumCacheSize(settings.cacheDatabaseMaximumSize())); } @@ -312,15 +312,13 @@ Map::Map(QObject *parent_, const Settings &settings, const QSize &size, qreal pi loop.setLocalData(std::make_shared()); } - d_ptr = new MapPrivate(this, settings, size, pixelRatio); + d_ptr = std::make_unique(this, settings, size, pixelRatio); } /*! Destroys this QMapLibre::Map. */ -Map::~Map() { - delete d_ptr; -} +Map::~Map() = default; /*! \property QMapLibre::Map::styleJson @@ -921,16 +919,16 @@ void Map::updateSource(const QString &id, const QVariantMap ¶ms) { return; } - auto sourceGeoJSON = source->as(); - auto sourceImage = source->as(); - if (!sourceGeoJSON && !sourceImage) { + auto *sourceGeoJSON = source->as(); + auto *sourceImage = source->as(); + if (sourceGeoJSON == nullptr && sourceImage == nullptr) { qWarning() << "Unable to update source: only GeoJSON and Image sources are mutable."; return; } - if (sourceImage && params.contains("url")) { + if (sourceImage != nullptr && params.contains("url")) { sourceImage->setURL(params["url"].toString().toStdString()); - } else if (sourceGeoJSON && params.contains("data")) { + } else if (sourceGeoJSON != nullptr && params.contains("data")) { Error error; auto result = convert(params["data"], error); if (result) { @@ -1106,7 +1104,7 @@ void Map::setFilter(const QString &layer, const QVariant &filter) { using namespace mbgl::style::conversion; Layer *layer_ = d_ptr->mapObj->getStyle().getLayer(layer.toStdString()); - if (!layer_) { + if (layer_ == nullptr) { qWarning() << "Layer not found:" << layer; return; } @@ -1158,7 +1156,7 @@ QVariant Map::getFilter(const QString &layer) const { using namespace mbgl::style::conversion; Layer *layer_ = d_ptr->mapObj->getStyle().getLayer(layer.toStdString()); - if (!layer_) { + if (layer_ == nullptr) { qWarning() << "Layer not found:" << layer; return QVariant(); } @@ -1341,7 +1339,7 @@ void MapPrivate::update(std::shared_ptr parameters) { m_updateParameters = std::move(parameters); - if (!m_mapRenderer) { + if (m_mapRenderer == nullptr) { return; } @@ -1399,7 +1397,7 @@ void MapPrivate::render() { void MapPrivate::setFramebufferObject(quint32 fbo, const QSize &size) { std::lock_guard lock(m_mapRendererMutex); - if (!m_mapRenderer) { + if (m_mapRenderer == nullptr) { createRenderer(); } @@ -1419,7 +1417,7 @@ bool MapPrivate::setProperty(const PropertySetter &setter, using namespace mbgl::style; Layer *layerObject = mapObj->getStyle().getLayer(layer.toStdString()); - if (!layerObject) { + if (layerObject == nullptr) { qWarning() << "Layer not found:" << layer; return false; } diff --git a/src/core/map.hpp b/src/core/map.hpp index 9602d92..70c2ef2 100644 --- a/src/core/map.hpp +++ b/src/core/map.hpp @@ -188,7 +188,7 @@ public slots: private: Q_DISABLE_COPY(Map) - MapPrivate *d_ptr; + std::unique_ptr d_ptr; }; } // namespace QMapLibre diff --git a/src/core/map_observer.cpp b/src/core/map_observer.cpp index da5015c..3f846b4 100644 --- a/src/core/map_observer.cpp +++ b/src/core/map_observer.cpp @@ -14,9 +14,9 @@ namespace QMapLibre { MapObserver::MapObserver(MapPrivate *d) - : d_ptr(d) {} + : d_ptrRef(d) {} -MapObserver::~MapObserver() {} +MapObserver::~MapObserver() = default; void MapObserver::onCameraWillChange(mbgl::MapObserver::CameraChangeMode mode) { if (mode == mbgl::MapObserver::CameraChangeMode::Immediate) { @@ -99,7 +99,7 @@ void MapObserver::onDidFinishLoadingStyle() { void MapObserver::onSourceChanged(mbgl::style::Source &) { std::string attribution; - for (const auto &source : d_ptr->mapObj->getStyle().getSources()) { + for (const auto &source : d_ptrRef->mapObj->getStyle().getSources()) { // Avoid duplicates by using the most complete attribution HTML snippet. if (source->getAttribution() && (attribution.size() < source->getAttribution()->size())) attribution = *source->getAttribution(); diff --git a/src/core/map_observer_p.hpp b/src/core/map_observer_p.hpp index 45b931e..a3167cd 100644 --- a/src/core/map_observer_p.hpp +++ b/src/core/map_observer_p.hpp @@ -48,7 +48,7 @@ class MapObserver : public QObject, public mbgl::MapObserver { private: Q_DISABLE_COPY(MapObserver) - MapPrivate *d_ptr; + MapPrivate *d_ptrRef; }; } // namespace QMapLibre diff --git a/src/core/settings.cpp b/src/core/settings.cpp index e73d9c9..3335675 100644 --- a/src/core/settings.cpp +++ b/src/core/settings.cpp @@ -9,7 +9,6 @@ #include #include #include -#include #include #include @@ -149,37 +148,23 @@ namespace QMapLibre { but a provider template can be provided. */ Settings::Settings(ProviderTemplate provider) - : d_ptr(new SettingsPrivate) { + : d_ptr(std::make_unique()) { d_ptr->setProviderTemplate(provider); } -Settings::~Settings() { - delete d_ptr; -} +Settings::~Settings() = default; Settings::Settings(const Settings &s) - : d_ptr(new SettingsPrivate) { - *d_ptr = *s.d_ptr; -} + : d_ptr(std::make_unique(*s.d_ptr)) {} -Settings::Settings(Settings &&s) noexcept - : d_ptr(s.d_ptr) { - s.d_ptr = nullptr; -} +Settings::Settings(Settings &&s) noexcept = default; Settings &Settings::operator=(const Settings &s) { - if (d_ptr != nullptr) delete d_ptr; - d_ptr = new SettingsPrivate; - *d_ptr = *s.d_ptr; + d_ptr = std::make_unique(*s.d_ptr); return *this; } -Settings &Settings::operator=(Settings &&s) noexcept { - if (d_ptr != nullptr) delete d_ptr; - d_ptr = s.d_ptr; - s.d_ptr = nullptr; - return *this; -} +Settings &Settings::operator=(Settings &&s) noexcept = default; /*! Returns the OpenGL context mode. This is specially important when mixing @@ -340,11 +325,11 @@ void Settings::setApiKey(const QString &key) { Returns the API base URL. */ QString Settings::apiBaseUrl() const { - if (tileServerOptions() == nullptr) { + if (!customTileServerOptions()) { return {}; } - return QString::fromStdString(tileServerOptions()->baseURL()); + return QString::fromStdString(tileServerOptions().baseURL()); } /*! @@ -463,11 +448,11 @@ void Settings::setStyles(const Styles &styles) { */ Styles Settings::providerStyles() const { Styles styles; - if (tileServerOptions() == nullptr) { + if (!customTileServerOptions()) { return styles; } - for (const auto &style : tileServerOptions()->defaultStyles()) { + for (const auto &style : tileServerOptions().defaultStyles()) { styles.append(Style(QString::fromStdString(style.getUrl()), QString::fromStdString(style.getName()))); } return styles; @@ -501,12 +486,19 @@ void Settings::setDefaultZoom(double zoom) { d_ptr->m_defaultZoom = zoom; } +/*! + Returns whether the tile server options have been set by the user. +*/ +bool Settings::customTileServerOptions() const { + return d_ptr->m_customTileServerOptions; +} + /*! Returns the provider tile server options. Note that this is mainly for internal use. */ -mbgl::TileServerOptions *Settings::tileServerOptions() const { +const mbgl::TileServerOptions &Settings::tileServerOptions() const { return d_ptr->m_tileServerOptions; } @@ -523,29 +515,30 @@ SettingsPrivate::SettingsPrivate() m_apiKey(qgetenv("MLN_API_KEY")) {} void SettingsPrivate::setProviderTemplate(Settings::ProviderTemplate providerTemplate) { - if (m_tileServerOptions) { - delete m_tileServerOptions; - m_tileServerOptions = nullptr; - } - m_providerTemplate = providerTemplate; if (providerTemplate == Settings::MapLibreProvider) { - m_tileServerOptions = new mbgl::TileServerOptions(mbgl::TileServerOptions::MapLibreConfiguration()); + m_tileServerOptions = mbgl::TileServerOptions::MapLibreConfiguration(); + m_customTileServerOptions = true; } else if (providerTemplate == Settings::MapTilerProvider) { - m_tileServerOptions = new mbgl::TileServerOptions(mbgl::TileServerOptions::MapTilerConfiguration()); + m_tileServerOptions = mbgl::TileServerOptions::MapTilerConfiguration(); + m_customTileServerOptions = true; } else if (providerTemplate == Settings::MapboxProvider) { - m_tileServerOptions = new mbgl::TileServerOptions(mbgl::TileServerOptions::MapboxConfiguration()); + m_tileServerOptions = mbgl::TileServerOptions::MapboxConfiguration(); + m_customTileServerOptions = true; + } else { + m_tileServerOptions = mbgl::TileServerOptions(); + m_customTileServerOptions = false; } } void SettingsPrivate::setProviderApiBaseUrl(const QString &url) { - if (m_tileServerOptions == nullptr) { + if (!m_customTileServerOptions) { qWarning() << "No provider set so not setting API URL."; return; } - m_tileServerOptions = &m_tileServerOptions->withBaseURL(url.toStdString()); + m_tileServerOptions = std::move(m_tileServerOptions.withBaseURL(url.toStdString())); } } // namespace QMapLibre diff --git a/src/core/settings.hpp b/src/core/settings.hpp index f30b690..760b154 100644 --- a/src/core/settings.hpp +++ b/src/core/settings.hpp @@ -13,6 +13,7 @@ #include #include +#include // TODO: this will be wrapped at some point namespace mbgl { @@ -110,10 +111,11 @@ class Q_MAPLIBRE_CORE_EXPORT Settings { double defaultZoom() const; void setDefaultZoom(double); - mbgl::TileServerOptions *tileServerOptions() const; + bool customTileServerOptions() const; + const mbgl::TileServerOptions &tileServerOptions() const; private: - SettingsPrivate *d_ptr; + std::unique_ptr d_ptr; }; } // namespace QMapLibre diff --git a/src/core/settings_p.hpp b/src/core/settings_p.hpp index 20ee672..60f393b 100644 --- a/src/core/settings_p.hpp +++ b/src/core/settings_p.hpp @@ -8,10 +8,13 @@ #include "settings.hpp" #include "types.hpp" +#include + #include #include #include +#include namespace mbgl { class TileServerOptions; @@ -47,7 +50,8 @@ class SettingsPrivate { std::function m_resourceTransform; - mbgl::TileServerOptions *m_tileServerOptions{}; + bool m_customTileServerOptions{}; + mbgl::TileServerOptions m_tileServerOptions{}; }; } // namespace QMapLibre diff --git a/src/location/qgeomap.cpp b/src/location/qgeomap.cpp index 60c1507..2477f85 100644 --- a/src/location/qgeomap.cpp +++ b/src/location/qgeomap.cpp @@ -49,7 +49,7 @@ namespace QMapLibre { QGeoMapMapLibrePrivate::QGeoMapMapLibrePrivate(QGeoMappingManagerEngine *engine) : QGeoMapPrivate(engine, new QGeoProjectionWebMercator) {} -QGeoMapMapLibrePrivate::~QGeoMapMapLibrePrivate() {} +QGeoMapMapLibrePrivate::~QGeoMapMapLibrePrivate() = default; QSGNode *QGeoMapMapLibrePrivate::updateSceneGraph(QSGNode *node, QQuickWindow *window) { Q_Q(QGeoMapMapLibre); @@ -60,9 +60,9 @@ QSGNode *QGeoMapMapLibrePrivate::updateSceneGraph(QSGNode *node, QQuickWindow *w } Map *map{}; - if (!node) { + if (node == nullptr) { QOpenGLContext *currentCtx = QOpenGLContext::currentContext(); - if (!currentCtx) { + if (currentCtx == nullptr) { qWarning("QOpenGLContext is NULL!"); qWarning() << "You are running on QSG backend " << QSGContext::backend(); qWarning("The MapLibre plugin works with both Desktop and ES 2.0+ OpenGL versions."); @@ -74,10 +74,10 @@ QSGNode *QGeoMapMapLibrePrivate::updateSceneGraph(QSGNode *node, QQuickWindow *w return node; } - auto *mbglNode = new TextureNode(m_settings, m_viewportSize, window->devicePixelRatio(), q); + auto mbglNode = std::make_unique(m_settings, m_viewportSize, window->devicePixelRatio(), q); QObject::connect(mbglNode->map(), &Map::mapChanged, q, &QGeoMapMapLibre::onMapChanged); m_syncState = MapTypeSync | CameraDataSync | ViewportSync | VisibleAreaSync; - node = mbglNode; + node = mbglNode.release(); } map = static_cast(node)->map(); @@ -354,7 +354,7 @@ QGeoMapMapLibre::QGeoMapMapLibre(QGeoMappingManagerEngine *engine, QObject *pare d->m_refresh.setInterval(250); } -QGeoMapMapLibre::~QGeoMapMapLibre() {} +QGeoMapMapLibre::~QGeoMapMapLibre() = default; void QGeoMapMapLibre::setSettings(const Settings &settings) { Q_D(QGeoMapMapLibre); diff --git a/src/location/texture_node.cpp b/src/location/texture_node.cpp index e5a2287..ac52ca1 100644 --- a/src/location/texture_node.cpp +++ b/src/location/texture_node.cpp @@ -27,7 +27,7 @@ TextureNode::TextureNode(const Settings &settings, const QSize &size, qreal pixe setTextureCoordinatesTransform(QSGSimpleTextureNode::MirrorVertically); setFiltering(QSGTexture::Linear); - m_map.reset(new Map(nullptr, settings, size.expandedTo(minTextureSize), pixelRatio)); + m_map = std::make_unique(nullptr, settings, size.expandedTo(minTextureSize), pixelRatio); QObject::connect(m_map.get(), &Map::needsRendering, geoMap, &QGeoMap::sgNodeChanged); } @@ -43,7 +43,7 @@ void TextureNode::resize(const QSize &size, qreal pixelRatio) m_map->resize(minSize); - m_fbo.reset(new QOpenGLFramebufferObject(fbSize, QOpenGLFramebufferObject::CombinedDepthStencil)); + m_fbo = std::make_unique(fbSize, QOpenGLFramebufferObject::CombinedDepthStencil); m_map->setFramebufferObject(m_fbo->handle(), fbSize); #if QT_VERSION >= QT_VERSION_CHECK(6, 0, 0) @@ -52,7 +52,7 @@ void TextureNode::resize(const QSize &size, qreal pixelRatio) setOwnsTexture(true); #else auto *fboTexture = static_cast(texture()); - if (!fboTexture) { + if (fboTexture == nullptr) { fboTexture = new QSGPlainTexture; fboTexture->setHasAlphaChannel(true); } @@ -60,7 +60,7 @@ void TextureNode::resize(const QSize &size, qreal pixelRatio) fboTexture->setTextureId(m_fbo->texture()); fboTexture->setTextureSize(fbSize); - if (!texture()) { + if (texture() == nullptr) { setTexture(fboTexture); setOwnsTexture(true); } diff --git a/src/widgets/gl_widget.cpp b/src/widgets/gl_widget.cpp index 096bcde..c1a6d19 100644 --- a/src/widgets/gl_widget.cpp +++ b/src/widgets/gl_widget.cpp @@ -10,15 +10,14 @@ namespace QMapLibre { -GLWidget::GLWidget(const Settings &settings) { - d_ptr = new GLWidgetPrivate(this, settings); -} +GLWidget::GLWidget(const Settings &settings) + : d_ptr(std::make_unique(this, settings)) {} GLWidget::~GLWidget() { // Make sure we have a valid context so we // can delete the Map. makeCurrent(); - delete d_ptr; + d_ptr.reset(); } Map *GLWidget::map() { @@ -38,7 +37,7 @@ void GLWidget::wheelEvent(QWheelEvent *event) { } void GLWidget::initializeGL() { - d_ptr->m_map.reset(new Map(nullptr, d_ptr->m_settings, size(), devicePixelRatioF())); + d_ptr->m_map = std::make_unique(nullptr, d_ptr->m_settings, size(), devicePixelRatioF()); connect(d_ptr->m_map.get(), SIGNAL(needsRendering()), this, SLOT(update())); // Set default location @@ -64,7 +63,7 @@ GLWidgetPrivate::GLWidgetPrivate(QObject *parent, const Settings &settings) : QObject(parent), m_settings(settings) {} -GLWidgetPrivate::~GLWidgetPrivate() {} +GLWidgetPrivate::~GLWidgetPrivate() = default; void GLWidgetPrivate::handleMousePressEvent(QMouseEvent *event) { #if QT_VERSION >= QT_VERSION_CHECK(6, 0, 0) diff --git a/src/widgets/gl_widget.hpp b/src/widgets/gl_widget.hpp index 20a7866..6637bb6 100644 --- a/src/widgets/gl_widget.hpp +++ b/src/widgets/gl_widget.hpp @@ -48,7 +48,7 @@ class Q_MAPLIBRE_WIDGETS_EXPORT GLWidget : public QOpenGLWidget { private: Q_DISABLE_COPY(GLWidget) - GLWidgetPrivate *d_ptr; + std::unique_ptr d_ptr; }; } // namespace QMapLibre diff --git a/test/widgets/gl_tester.cpp b/test/widgets/gl_tester.cpp index ecb9a6e..41c740e 100644 --- a/test/widgets/gl_tester.cpp +++ b/test/widgets/gl_tester.cpp @@ -16,11 +16,11 @@ GLTester::GLTester(const QMapLibre::Settings &settings) : GLWidget(settings) {} void GLTester::initializeAnimation() { - m_bearingAnimation = new QPropertyAnimation(map(), "bearing"); - m_zoomAnimation = new QPropertyAnimation(map(), "zoom"); + m_bearingAnimation = std::make_unique(map(), "bearing"); + m_zoomAnimation = std::make_unique(map(), "zoom"); - connect(m_zoomAnimation, &QPropertyAnimation::finished, this, &GLTester::animationFinished); - connect(m_zoomAnimation, &QPropertyAnimation::valueChanged, this, &GLTester::animationValueChanged); + connect(m_zoomAnimation.get(), &QPropertyAnimation::finished, this, &GLTester::animationFinished); + connect(m_zoomAnimation.get(), &QPropertyAnimation::valueChanged, this, &GLTester::animationValueChanged); } int GLTester::selfTest() { diff --git a/test/widgets/gl_tester.hpp b/test/widgets/gl_tester.hpp index 29ff8d5..10b9ad2 100644 --- a/test/widgets/gl_tester.hpp +++ b/test/widgets/gl_tester.hpp @@ -8,6 +8,8 @@ #include +#include + namespace QMapLibre { class GLTester : public GLWidget { @@ -26,8 +28,8 @@ private slots: private: void paintGL() final; - QPropertyAnimation *m_bearingAnimation{}; - QPropertyAnimation *m_zoomAnimation{}; + std::unique_ptr m_bearingAnimation{}; + std::unique_ptr m_zoomAnimation{}; unsigned m_animationTicks{}; unsigned m_frameDraws{};