Skip to content

Conversation

HTRamsey
Copy link
Collaborator

Splits the worker and sql database management for cleaner code

@HTRamsey HTRamsey force-pushed the dev-qtlocationplugin-tilecacheworker-database branch 3 times, most recently from 66e73fb to fd4b6d3 Compare September 2, 2025 01:13
@HTRamsey HTRamsey marked this pull request as ready for review September 2, 2025 01:44
@HTRamsey HTRamsey requested a review from Copilot September 2, 2025 01:44
Copy link
Contributor

@Copilot Copilot AI left a 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 QGCTileCacheDatabase class to handle all SQL operations
  • Refactoring of QGCCacheWorker to 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_unique followed by release() defeats the purpose of smart pointers. Either use raw new directly or modify the API to accept std::unique_ptr to 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")
Copy link
Preview

Copilot AI Sep 2, 2025

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.

Suggested change
QGC_LOGGING_CATEGORY(QGCTileCacheWorkerLog, "test.qtlocationplugin.qgctilecacheworker")
QGC_LOGGING_CATEGORY(QGCTileCacheWorkerLog, "qgc.qtlocationplugin.qgctilecacheworker")

Copilot uses AI. Check for mistakes.

Comment on lines +532 to +554
tile->hash = info.hash;
tile->type = UrlFactory::getProviderTypeFromQtMapId(info.type);
tile->x = info.x;
tile->y = info.y;
tile->z = info.z;
Copy link
Preview

Copilot AI Sep 2, 2025

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.

Suggested change
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);

Copilot uses AI. Check for mistakes.

int counter = 1;
const QString baseName = setInfo.name;
do {
setInfo.name = u"%1 %02d"_s.arg(baseName, ++counter);
Copy link
Preview

Copilot AI Sep 2, 2025

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.

Suggested change
setInfo.name = u"%1 %02d"_s.arg(baseName, ++counter);
setInfo.name = u"%1 %2"_s.arg(baseName).arg(++counter, 2, 10, QLatin1Char('0'));

Copilot uses AI. Check for mistakes.

@HTRamsey HTRamsey force-pushed the dev-qtlocationplugin-tilecacheworker-database branch 3 times, most recently from a428dbb to 646a122 Compare September 3, 2025 11:51
@HTRamsey HTRamsey force-pushed the dev-qtlocationplugin-tilecacheworker-database branch from 646a122 to 88aa368 Compare September 15, 2025 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant