-
Notifications
You must be signed in to change notification settings - Fork 4.4k
QtLocationPlugin: Split CacheWorker & CacheDatabase #13341
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
QtLocationPlugin: Split CacheWorker & CacheDatabase #13341
Conversation
66e73fb to
fd4b6d3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements a refactoring of the QtLocationPlugin map cache system to split database and worker functionality for cleaner code organization. The main purpose is to extract database operations into a separate class (QGCTileCacheDatabase) while maintaining the existing worker thread architecture.
Key changes include:
- Introduction of a new
QGCTileCacheDatabaseclass to handle all SQL operations - Refactoring of
QGCCacheWorkerto use the new database class instead of direct SQL - Modernization of code style including enum classes, better error handling, and improved const-correctness
Reviewed Changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/UI/AppSettings/MapSettings.qml | Updates enum references to use scoped enum syntax |
| src/QtLocationPlugin/QGCTileCacheDatabase.h | Defines new database abstraction class with structured data types |
| src/QtLocationPlugin/QGCTileCacheDatabase.cpp | Implements comprehensive database operations with error handling |
| src/QtLocationPlugin/QGCTileCacheWorker.h | Refactored worker header to use database abstraction |
| src/QtLocationPlugin/QGCTileCacheWorker.cpp | Updated worker implementation to delegate to database class |
| Various other files | Code style improvements, enum updates, and header organization |
Comments suppressed due to low confidence (2)
src/QtLocationPlugin/QGCTileCacheDatabase.cpp:1
- Using
std::make_uniquefollowed byrelease()defeats the purpose of smart pointers. Either use rawnewdirectly or modify the API to acceptstd::unique_ptrto maintain RAII benefits.
/****************************************************************************
src/QtLocationPlugin/QGCTileCacheWorker.cpp:1
- Inconsistent member access - mixing direct member access (
hash,format,type) with what appears to be a method call (img). All should use consistent accessor patterns.
/****************************************************************************
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| using namespace Qt::StringLiterals; | ||
|
|
||
| QGC_LOGGING_CATEGORY(QGCTileCacheWorkerLog, "test.qtlocationplugin.qgctilecacheworker") |
Copilot
AI
Sep 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logging category name contains 'test.' prefix which appears to be a debugging leftover. It should be 'qgc.qtlocationplugin.qgctilecacheworker' to match the naming convention used in other files.
| QGC_LOGGING_CATEGORY(QGCTileCacheWorkerLog, "test.qtlocationplugin.qgctilecacheworker") | |
| QGC_LOGGING_CATEGORY(QGCTileCacheWorkerLog, "qgc.qtlocationplugin.qgctilecacheworker") |
| tile->hash = info.hash; | ||
| tile->type = UrlFactory::getProviderTypeFromQtMapId(info.type); | ||
| tile->x = info.x; | ||
| tile->y = info.y; | ||
| tile->z = info.z; |
Copilot
AI
Sep 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is accessing QGCTile members as direct public variables, but the original class used accessor methods. This inconsistency will cause compilation errors if QGCTile hasn't been updated to use public member variables everywhere.
| tile->hash = info.hash; | |
| tile->type = UrlFactory::getProviderTypeFromQtMapId(info.type); | |
| tile->x = info.x; | |
| tile->y = info.y; | |
| tile->z = info.z; | |
| tile->setHash(info.hash); | |
| tile->setType(UrlFactory::getProviderTypeFromQtMapId(info.type)); | |
| tile->setX(info.x); | |
| tile->setY(info.y); | |
| tile->setZ(info.z); |
| int counter = 1; | ||
| const QString baseName = setInfo.name; | ||
| do { | ||
| setInfo.name = u"%1 %02d"_s.arg(baseName, ++counter); |
Copilot
AI
Sep 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The arg() method expects a string for the second argument when using %02d format, but counter is an integer. This should use QString::arg(baseName).arg(counter, 2, 10, QLatin1Char('0')) or change the format specifier.
| setInfo.name = u"%1 %02d"_s.arg(baseName, ++counter); | |
| setInfo.name = u"%1 %2"_s.arg(baseName).arg(++counter, 2, 10, QLatin1Char('0')); |
a428dbb to
646a122
Compare
646a122 to
88aa368
Compare
Splits the worker and sql database management for cleaner code