QtLocationPlugin: Qt Integration Improvements & Fixes #13636

HTRamsey · 2025-11-13T21:18:34Z

Some pretty significant QtLocation integration improvements

Copilot

Pull Request Overview

This PR significantly improves the QtLocation plugin integration in QGroundControl by replacing assertions with defensive error handling, fixing SQL injection vulnerabilities, improving concurrency safety, and enhancing database integrity through transactions and foreign key constraints.

Key Changes:

Replaced Q_ASSERT statements with proper null checks and error logging throughout the codebase
Migrated all SQL queries from string concatenation to parameterized queries, preventing SQL injection
Added database transactions to ensure atomicity of multi-step operations and foreign key constraints for data integrity

Reviewed Changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
QGeoTiledMappingManagerEngineQGC.h/cpp	Added tile size constant and replaced assertions with defensive null checks
QGeoTileFetcherQGC.h/cpp	Added network timeout constant, improved error handling with proper logging, fixed HTTP header spelling (Referer), enabled compression
QGeoServiceProviderPluginQGC.cpp	Replaced assertions with thread-safety and null checks
QGeoMapReplyQGC.cpp	Enhanced error handling with null checks and better error messages
QGeoFileTileCacheQGC.h/cpp	Migrated cache location, added override methods, improved validation, implemented cache migration logic
QGCTileCacheWorker.h/cpp	Converted from shared_ptr to Qt database management, added transactions, implemented batch delete with SQL variable limit handling, added foreign key constraints
QGCMapUrlEngine.h/cpp	Added default map ID constants, enabled Google Maps on iOS, fixed null return to empty string
QGCMapEngineManager.cc	Added null check for tile set iteration, removed redundant empty string return
QGCCachedTileSet.h/cpp	Added mutex for thread-safe reply handling, fixed division-by-zero check, improved concurrent download logic
CMakeLists.txt	Removed iOS-specific Google Maps exclusion
OfflineMapEditor.qml	Enhanced delete button logic to prevent deleting default set with no tiles

Copilot · 2025-11-13T21:30:27Z

src/QtLocationPlugin/QGeoFileTileCacheQGC.cpp

    QString cacheDir = QStandardPaths::writableLocation(QStandardPaths::AppDataLocation);
 #else
-    QString cacheDir = QStandardPaths::writableLocation(QStandardPaths::GenericCacheLocation);
+    QString cacheDir = QStandardPaths::writableLocation(QStandardPaths::CacheLocation);


The cache path lookup was changed from QStandardPaths::writableLocation(QStandardPaths::GenericCacheLocation) to QStandardPaths::writableLocation(QStandardPaths::CacheLocation) on non-mobile platforms. On desktop platforms, GenericCacheLocation and CacheLocation may point to different directories. This is a breaking change that will cause existing users' cached tiles to not be found. The migration logic in _migrateCache() should handle this, but verify that all existing cache data is properly migrated on first run after this update.

Copilot · 2025-11-13T21:30:28Z

src/QtLocationPlugin/QGCMapEngineManager.cc

            return name;
        }
    }
-
-    return QString("");
 }


The function getUniqueName() previously had an explicit return QString(""); at the end as a fallback, which has been removed. The function now has no explicit return statement if all 99 attempts to generate a unique name fail. This will cause undefined behavior. The return statement should be restored or an exception/error should be thrown when all attempts are exhausted.

src/QtLocationPlugin/QGCTileCacheWorker.cpp

Copilot · 2025-11-13T21:30:28Z

src/QtLocationPlugin/QGCCachedTileSet.h

    QQueue<QGCTile*> _tilesToDownload;
    QGCMapEngineManager *_manager = nullptr;
    QNetworkAccessManager *_networkManager = nullptr;
+    QMutex _repliesMutex;


A QMutex _repliesMutex was added to protect access to the _replies hash in QGCCachedTileSet. The mutex is properly used when inserting and removing replies. However, verify that all other accesses to _replies throughout the class are also protected by this mutex to prevent race conditions.

Copilot · 2025-11-13T21:30:29Z

src/QtLocationPlugin/QGCTileCacheWorker.cpp

+            "setID INTEGER NOT NULL, "
+            "tileID INTEGER NOT NULL, "
+            "PRIMARY KEY (setID, tileID), "
+            "FOREIGN KEY (setID) REFERENCES TileSets(setID) ON DELETE CASCADE, "
+            "FOREIGN KEY (tileID) REFERENCES Tiles(tileID) ON DELETE CASCADE)")) {


The database schema was updated to include foreign key constraints with ON DELETE CASCADE for SetTiles and TilesDownload tables. However, foreign keys are only enabled if PRAGMA foreign_keys = ON is executed after opening the database connection. The code does enable this in _connectDB() and _createDB(), but if the database is accessed before these pragmas are set, foreign keys will not work. Ensure that the pragma is always executed immediately after opening any database connection to guarantee constraint enforcement.

Copilot · 2025-11-13T21:30:29Z

src/QtLocationPlugin/QGeoFileTileCacheQGC.cpp

+    std::call_once(cacheInit, []() {
+        _initCache();
+    });
+
+    directory_ = _getCachePath(parameters);
+


The std::call_once lambda was changed from capturing this ([this]()) to capturing nothing ([]()). This is correct since _initCache() is a static method and doesn't require this. However, verify that all code inside _initCache() and its callees can safely execute without instance state, as this is now called before the instance is fully initialized (moved before directory_ assignment).

Suggested change

std::call_once(cacheInit, []() {

_initCache();

});

directory_ = _getCachePath(parameters);

directory_ = _getCachePath(parameters);

std::call_once(cacheInit, []() {

_initCache();

});

src/QtLocationPlugin/QGCMapUrlEngine.cpp

Copilot · 2025-11-13T21:30:30Z

src/QtLocationPlugin/QGCTileCacheWorker.cpp

+            } else {
+                (void) query.exec("CREATE INDEX IF NOT EXISTS idx_tilesdownload_setid_state ON TilesDownload ( setID, state )");
+
+                // Clean up any duplicate entries in SetTiles from older database versions


A cleanup query was added to remove duplicate entries in SetTiles from older database versions. However, this query uses rowid which is SQLite-specific and may not work if the database type is changed in the future. Consider using a more portable approach or documenting that this code is SQLite-specific.

Suggested change

// Clean up any duplicate entries in SetTiles from older database versions

// Clean up any duplicate entries in SetTiles from older database versions

// NOTE: The following query uses SQLite's 'rowid' and is SQLite-specific.

// If the database backend is changed, this query will need to be updated.

Copilot · 2025-11-13T21:30:30Z

src/QtLocationPlugin/QGeoFileTileCacheQGC.cpp


 QGeoFileTileCacheQGC::QGeoFileTileCacheQGC(const QVariantMap &parameters, QObject *parent)
-    : QGeoFileTileCache(baseCacheDirectory(), parent)
+    : QGeoFileTileCache(QString(), parent)


The constructor was changed from QGeoFileTileCache(baseCacheDirectory(), parent) to QGeoFileTileCache(QString(), parent), passing an empty string instead of baseCacheDirectory(). Later in the constructor, directory_ is set via _getCachePath(parameters). This defers directory initialization, but the base class may expect a valid directory in the constructor. Verify that the base class QGeoFileTileCache properly handles an empty directory string in its constructor.

Copilot · 2025-11-13T21:30:31Z

src/QtLocationPlugin/QGCTileCacheWorker.cpp

+    if (_stopping) {
+        task->setError(tr("Worker Stopping"));
+        task->deleteLater();
+        return false;
+    }


The _stopping flag check in enqueueTask() prevents new tasks from being enqueued after stop() is called. However, there's a potential race condition: between checking _stopping and acquiring the mutex in line 65, another thread could set _stopping = true and delete queued tasks. Consider moving the _stopping check inside the mutex-protected section to ensure atomicity.

Copilot

Pull Request Overview

Copilot reviewed 36 out of 36 changed files in this pull request and generated 3 comments.

Copilot · 2025-11-15T07:51:22Z

src/QtLocationPlugin/QGeoFileTileCacheQGC.cpp

+    QString cacheDir = QStandardPaths::writableLocation(QStandardPaths::CacheLocation);
 #endif
-    cacheDir += QStringLiteral("/QGCMapCache") + QString(kCachePathVersion);
+    cacheDir += QStringLiteral("/QGCMapCache");


Cache path versioning removed without migration path. The PR removes the version suffix from cache directory (previously QGCMapCache300), but the _wipeOldCaches() function uses a wildcard pattern that may not properly identify and migrate versioned caches. According to QGC guidelines, cache operations should handle backward compatibility and ensure data isn't lost during upgrades.

Copilot · 2025-11-15T07:51:23Z

src/QtLocationPlugin/QGCTileCacheWorker.cpp

    }

-    return 1L;
+    return UrlFactory::defaultTileSetId();


Inconsistent default tile set ID handling. This returns UrlFactory::defaultTileSetId() (value 1) but in multiple other places the code compares against UINT64_MAX (e.g., lines 269, 1012). Consider using a consistent constant throughout or documenting why different sentinel values are used in different contexts.

Suggested change

return UrlFactory::defaultTileSetId();

return UINT64_MAX;

Copilot · 2025-11-15T07:51:23Z

src/QtLocationPlugin/QGCTileCacheWorker.cpp

+
+    int testCount = 0;
+    while (testCount < kMaxNameGenerationAttempts) {
+        const QString testName = QString::asprintf("%s %03d", name.toLatin1().constData(), ++testCount);


Using QString::asprintf with toLatin1() can cause data loss for non-ASCII characters in tile set names. Consider using QString::arg() instead: QString(\"%1 %2\").arg(name).arg(testCount, 3, 10, QChar('0')) to properly handle Unicode characters.

Suggested change

const QString testName = QString::asprintf("%s %03d", name.toLatin1().constData(), ++testCount);

const QString testName = QString("%1 %2").arg(name).arg(++testCount, 3, 10, QChar('0'));

Copilot

Pull Request Overview

Copilot reviewed 44 out of 44 changed files in this pull request and generated no new comments.

HTRamsey requested a review from Copilot November 13, 2025 21:25

Copilot started reviewing on behalf of HTRamsey November 13, 2025 21:25 View session

Copilot finished reviewing on behalf of HTRamsey November 13, 2025 21:27

Copilot AI reviewed Nov 13, 2025

View reviewed changes

HTRamsey force-pushed the dev-qtlocationplugin-fixes branch 2 times, most recently from 659d3fa to 73b240e Compare November 15, 2025 06:36

HTRamsey requested a review from Copilot November 15, 2025 07:49

HTRamsey marked this pull request as ready for review November 15, 2025 07:49

Copilot started reviewing on behalf of HTRamsey November 15, 2025 07:49 View session

Copilot finished reviewing on behalf of HTRamsey November 15, 2025 07:50

Copilot AI reviewed Nov 15, 2025

View reviewed changes

HTRamsey force-pushed the dev-qtlocationplugin-fixes branch 3 times, most recently from 0cf1f55 to a8d644d Compare November 18, 2025 05:12

HTRamsey requested a review from Copilot November 18, 2025 05:38

Copilot started reviewing on behalf of HTRamsey November 18, 2025 05:38 View session

Copilot finished reviewing on behalf of HTRamsey November 18, 2025 05:39

Copilot AI reviewed Nov 18, 2025

View reviewed changes

HTRamsey force-pushed the dev-qtlocationplugin-fixes branch from a8d644d to e03690f Compare November 18, 2025 05:56

QtLocationPlugin: Qt Integration Improvements & Fixes

e26d490

HTRamsey force-pushed the dev-qtlocationplugin-fixes branch from e03690f to e26d490 Compare November 18, 2025 07:47

HTRamsey marked this pull request as draft November 19, 2025 11:52

	const QString testName = QString::asprintf("%s %03d", name.toLatin1().constData(), ++testCount);
	const QString testName = QString("%1 %2").arg(name).arg(++testCount, 3, 10, QChar('0'));

QtLocationPlugin: Qt Integration Improvements & Fixes #13636

Are you sure you want to change the base?

QtLocationPlugin: Qt Integration Improvements & Fixes #13636

Conversation

HTRamsey commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

HTRamsey commented Nov 13, 2025 •

edited

Loading