From 4a47868b893f513332f92de05db7d6f95c58f006 Mon Sep 17 00:00:00 2001 From: Mathieu Pellerin Date: Sat, 24 Feb 2024 17:23:01 +0700 Subject: [PATCH 01/39] [network] Enable strict transport security to fix http->https WMS (et al) data sources --- src/core/network/qgsnetworkaccessmanager.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/core/network/qgsnetworkaccessmanager.cpp b/src/core/network/qgsnetworkaccessmanager.cpp index 4ab2e97fee22..cf297b32101e 100644 --- a/src/core/network/qgsnetworkaccessmanager.cpp +++ b/src/core/network/qgsnetworkaccessmanager.cpp @@ -216,6 +216,8 @@ QgsNetworkAccessManager::QgsNetworkAccessManager( QObject *parent ) { setProxyFactory( new QgsNetworkProxyFactory() ); setCookieJar( new QgsNetworkCookieJar( this ) ); + enableStrictTransportSecurityStore( true ); + setStrictTransportSecurityEnabled( true ); } void QgsNetworkAccessManager::setSslErrorHandler( std::unique_ptr handler ) From b203f78f958a54bdd17cd2b6504cb991b54df161 Mon Sep 17 00:00:00 2001 From: Alessandro Pasotti Date: Fri, 23 Feb 2024 13:58:37 +0100 Subject: [PATCH 02/39] Fix bookmarks manager model signals emitted twice rows inserted/added was fired twice (once by the manager and once by the model), confusing the proxy model. Also count was wrong by 1 in beginInsertRows( parent, oldCount, oldCount + count ) and beginRemoveRows, but now these two calls have been removed completely. Fix #56493 --- src/core/qgsbookmarkmodel.cpp | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/core/qgsbookmarkmodel.cpp b/src/core/qgsbookmarkmodel.cpp index b1a2f04a99e5..cc2ffaaf4d60 100644 --- a/src/core/qgsbookmarkmodel.cpp +++ b/src/core/qgsbookmarkmodel.cpp @@ -222,11 +222,9 @@ bool QgsBookmarkManagerModel::setData( const QModelIndex &index, const QVariant return false; } -bool QgsBookmarkManagerModel::insertRows( int, int count, const QModelIndex &parent ) +bool QgsBookmarkManagerModel::insertRows( int, int count, const QModelIndex & ) { // append - const int oldCount = mManager->bookmarks().count(); - beginInsertRows( parent, oldCount, oldCount + count ); bool result = true; for ( int i = 0; i < count; ++i ) { @@ -236,14 +234,11 @@ bool QgsBookmarkManagerModel::insertRows( int, int count, const QModelIndex &par mBlocked = false; result &= res; } - endInsertRows(); return result; } -bool QgsBookmarkManagerModel::removeRows( int row, int count, const QModelIndex &parent ) +bool QgsBookmarkManagerModel::removeRows( int row, int count, const QModelIndex & ) { - beginRemoveRows( parent, row, row + count ); - const QList< QgsBookmark > appBookmarks = mManager->bookmarks(); const QList< QgsBookmark > projectBookmarks = mProjectManager->bookmarks(); for ( int r = row + count - 1; r >= row; --r ) @@ -253,7 +248,6 @@ bool QgsBookmarkManagerModel::removeRows( int row, int count, const QModelIndex else mManager->removeBookmark( appBookmarks.at( r ).id() ); } - endRemoveRows(); return true; } From feb61209a53db86aa1dc063dd12c1581175fc417 Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Tue, 27 Feb 2024 12:22:05 +1000 Subject: [PATCH 03/39] Use pixel based scrolling instead of rows for identify results This behaves much nicer when row heights grow large, eg when there's multiline content in a field or when json fields are present --- src/app/qgsidentifyresultsdialog.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/app/qgsidentifyresultsdialog.cpp b/src/app/qgsidentifyresultsdialog.cpp index 90f022923e43..81ff2ef18d1d 100644 --- a/src/app/qgsidentifyresultsdialog.cpp +++ b/src/app/qgsidentifyresultsdialog.cpp @@ -354,6 +354,8 @@ QgsIdentifyResultsDialog::QgsIdentifyResultsDialog( QgsMapCanvas *canvas, QWidge mOpenFormAction->setDisabled( true ); + lstResults->setVerticalScrollMode( QListView::ScrollMode::ScrollPerPixel ); + QgsSettings mySettings; mDock = new QgsDockWidget( tr( "Identify Results" ), QgisApp::instance() ); mDock->setObjectName( QStringLiteral( "IdentifyResultsDock" ) ); From 0247ce5a03453163648a5a3661743d5d46d9fe88 Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Wed, 28 Feb 2024 09:14:48 +1000 Subject: [PATCH 04/39] Use better python repr for null QgsRectangle --- .../PyQt6/core/auto_generated/geometry/qgsrectangle.sip.in | 6 +++++- python/core/auto_generated/geometry/qgsrectangle.sip.in | 6 +++++- src/core/geometry/qgsrectangle.h | 6 +++++- tests/src/python/test_python_repr.py | 2 ++ 4 files changed, 17 insertions(+), 3 deletions(-) diff --git a/python/PyQt6/core/auto_generated/geometry/qgsrectangle.sip.in b/python/PyQt6/core/auto_generated/geometry/qgsrectangle.sip.in index 04122d80871f..d6b61a836026 100644 --- a/python/PyQt6/core/auto_generated/geometry/qgsrectangle.sip.in +++ b/python/PyQt6/core/auto_generated/geometry/qgsrectangle.sip.in @@ -389,7 +389,11 @@ the specified ``spacing`` between the grid lines. SIP_PYOBJECT __repr__(); %MethodCode - QString str = QStringLiteral( "" ).arg( sipCpp->asWktCoordinates() ); + QString str; + if ( sipCpp->isNull() ) + str = QStringLiteral( "" ); + else + str = QStringLiteral( "" ).arg( sipCpp->asWktCoordinates() ); sipRes = PyUnicode_FromString( str.toUtf8().constData() ); %End diff --git a/python/core/auto_generated/geometry/qgsrectangle.sip.in b/python/core/auto_generated/geometry/qgsrectangle.sip.in index 04122d80871f..d6b61a836026 100644 --- a/python/core/auto_generated/geometry/qgsrectangle.sip.in +++ b/python/core/auto_generated/geometry/qgsrectangle.sip.in @@ -389,7 +389,11 @@ the specified ``spacing`` between the grid lines. SIP_PYOBJECT __repr__(); %MethodCode - QString str = QStringLiteral( "" ).arg( sipCpp->asWktCoordinates() ); + QString str; + if ( sipCpp->isNull() ) + str = QStringLiteral( "" ); + else + str = QStringLiteral( "" ).arg( sipCpp->asWktCoordinates() ); sipRes = PyUnicode_FromString( str.toUtf8().constData() ); %End diff --git a/src/core/geometry/qgsrectangle.h b/src/core/geometry/qgsrectangle.h index f78be34aa062..a88ad9fef475 100644 --- a/src/core/geometry/qgsrectangle.h +++ b/src/core/geometry/qgsrectangle.h @@ -641,7 +641,11 @@ class CORE_EXPORT QgsRectangle #ifdef SIP_RUN SIP_PYOBJECT __repr__(); % MethodCode - QString str = QStringLiteral( "" ).arg( sipCpp->asWktCoordinates() ); + QString str; + if ( sipCpp->isNull() ) + str = QStringLiteral( "" ); + else + str = QStringLiteral( "" ).arg( sipCpp->asWktCoordinates() ); sipRes = PyUnicode_FromString( str.toUtf8().constData() ); % End #endif diff --git a/tests/src/python/test_python_repr.py b/tests/src/python/test_python_repr.py index 43cc0a46ebd2..677005b801a3 100644 --- a/tests/src/python/test_python_repr.py +++ b/tests/src/python/test_python_repr.py @@ -177,6 +177,8 @@ def testQgsPolygonRepr(self): def testQgsRectangleRepr(self): r = QgsRectangle(1, 2, 3, 4) self.assertEqual(r.__repr__(), '') + r = QgsRectangle() + self.assertEqual(r.__repr__(), '') def testQgsReferencedRectangleRepr(self): r = QgsReferencedRectangle(QgsRectangle(1, 2, 3, 4), QgsCoordinateReferenceSystem('EPSG:4326')) From ef98c93721ef2ec5d02db50fab0f5534410775b7 Mon Sep 17 00:00:00 2001 From: Julien Cabieces Date: Thu, 26 Oct 2023 10:42:51 +0200 Subject: [PATCH 05/39] [Postgres] Fixes #54260 : Remove listener wait condition --- src/providers/postgres/qgspostgresconn.h | 2 - .../postgres/qgspostgreslistener.cpp | 88 +++++-------------- src/providers/postgres/qgspostgreslistener.h | 8 +- 3 files changed, 29 insertions(+), 69 deletions(-) diff --git a/src/providers/postgres/qgspostgresconn.h b/src/providers/postgres/qgspostgresconn.h index 48700a6d8f36..721be1bcd61f 100644 --- a/src/providers/postgres/qgspostgresconn.h +++ b/src/providers/postgres/qgspostgresconn.h @@ -318,9 +318,7 @@ class QgsPostgresConn : public QObject QString uniqueCursorName(); -#if 0 PGconn *pgConnection() { return mConn; } -#endif // // libpq wrapper diff --git a/src/providers/postgres/qgspostgreslistener.cpp b/src/providers/postgres/qgspostgreslistener.cpp index d314b396f4b0..af54f081f897 100644 --- a/src/providers/postgres/qgspostgreslistener.cpp +++ b/src/providers/postgres/qgspostgreslistener.cpp @@ -19,6 +19,7 @@ #include "qgsdatasourceuri.h" #include "qgscredentials.h" #include "qgslogger.h" +#include "qgspostgresconn.h" #ifdef Q_OS_WIN #include @@ -26,8 +27,6 @@ #include #endif -const int PG_CONNECT_TIMEOUT = 30; - extern "C" { #include @@ -37,17 +36,27 @@ std::unique_ptr< QgsPostgresListener > QgsPostgresListener::create( const QStrin { std::unique_ptr< QgsPostgresListener > res( new QgsPostgresListener( connString ) ); QgsDebugMsgLevel( QStringLiteral( "starting notification listener" ), 2 ); - res->start(); - res->mMutex.lock(); - res->mIsReadyCondition.wait( &res->mMutex ); - res->mMutex.unlock(); + res->start(); return res; } QgsPostgresListener::QgsPostgresListener( const QString &connString ) - : mConnString( connString ) { + mConn = QgsPostgresConn::connectDb( connString, true, false ); + if ( mConn ) + { + mConn->moveToThread( this ); + + QgsPostgresResult result( mConn->LoggedPQexec( "QgsPostgresListener", QStringLiteral( "LISTEN qgis" ) ) ); + if ( result.PQresultStatus() != PGRES_COMMAND_OK ) + { + QgsDebugError( QStringLiteral( "error in listen" ) ); + + mConn->unref(); + mConn = nullptr; + } + } } QgsPostgresListener::~QgsPostgresListener() @@ -56,73 +65,27 @@ QgsPostgresListener::~QgsPostgresListener() QgsDebugMsgLevel( QStringLiteral( "stopping the loop" ), 2 ); wait(); QgsDebugMsgLevel( QStringLiteral( "notification listener stopped" ), 2 ); + + if ( mConn ) + mConn->unref(); } void QgsPostgresListener::run() { - PGconn *conn = nullptr; - QString connectString = mConnString; - - connectString += QStringLiteral( " connect_timeout=%1" ).arg( PG_CONNECT_TIMEOUT ); - conn = PQconnectdb( connectString.toUtf8() ); - - if ( PQstatus( conn ) != CONNECTION_OK ) - { - QgsDataSourceUri uri( connectString ); - QString username = uri.username(); - QString password = uri.password(); - - PQfinish( conn ); - - QgsCredentials::instance()->lock(); - - if ( QgsCredentials::instance()->get( mConnString, username, password, PQerrorMessage( conn ) ) ) - { - uri.setUsername( username ); - uri.setPassword( password ); - connectString = uri.connectionInfo( false ); - connectString += QStringLiteral( " connect_timeout=%1" ).arg( PG_CONNECT_TIMEOUT ); - - conn = PQconnectdb( connectString.toUtf8() ); - if ( PQstatus( conn ) == CONNECTION_OK ) - QgsCredentials::instance()->put( mConnString, username, password ); - } - - QgsCredentials::instance()->unlock(); - - if ( PQstatus( conn ) != CONNECTION_OK ) - { - PQfinish( conn ); - QgsDebugMsgLevel( QStringLiteral( "LISTENer not started" ), 2 ); - return; - } - } - - - PGresult *res = PQexec( conn, "LISTEN qgis" ); - if ( PQresultStatus( res ) != PGRES_COMMAND_OK ) + if ( !mConn ) { QgsDebugError( QStringLiteral( "error in listen" ) ); - PQclear( res ); - PQfinish( conn ); - mMutex.lock(); - mIsReadyCondition.wakeOne(); - mMutex.unlock(); return; } - PQclear( res ); - mMutex.lock(); - mIsReadyCondition.wakeOne(); - mMutex.unlock(); - const int sock = PQsocket( conn ); + const int sock = PQsocket( mConn->pgConnection() ); if ( sock < 0 ) { QgsDebugError( QStringLiteral( "error in socket" ) ); - PQfinish( conn ); return; } + PGconn *pgconn = mConn->pgConnection(); forever { fd_set input_mask; @@ -139,8 +102,8 @@ void QgsPostgresListener::run() break; } - PQconsumeInput( conn ); - PGnotify *n = PQnotifies( conn ); + PQconsumeInput( pgconn ); + PGnotify *n = PQnotifies( pgconn ); if ( n ) { const QString msg( n->extra ); @@ -155,7 +118,4 @@ void QgsPostgresListener::run() break; } } - PQfinish( conn ); } - - diff --git a/src/providers/postgres/qgspostgreslistener.h b/src/providers/postgres/qgspostgreslistener.h index 9f81386aa7d0..da546407e340 100644 --- a/src/providers/postgres/qgspostgreslistener.h +++ b/src/providers/postgres/qgspostgreslistener.h @@ -24,6 +24,9 @@ #include #include +class QgsPostgresConn; + + /** * \class QgsPostgresListener * \brief Launch a thread to listen on postgres notifications on the "qgis" channel, the notify signal is emitted on postgres notify. @@ -52,9 +55,8 @@ class QgsPostgresListener : public QThread private: volatile bool mStop = false; - const QString mConnString; - QWaitCondition mIsReadyCondition; - QMutex mMutex; + + QgsPostgresConn *mConn = nullptr; QgsPostgresListener( const QString &connString ); From 84b8ac4e24d1e79e8c618a7269ce4862b32aeea0 Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Wed, 28 Feb 2024 13:21:01 +1000 Subject: [PATCH 06/39] Consistently return invalid QVariant for QgsVectorDataProvider::defaultValue Previously we mixed invalid QVariant values with null QVariants, resulting in a mix of results between Qt 5 and Qt 6 and across different data providers --- src/core/providers/ogr/qgsogrprovider.cpp | 4 ++-- src/providers/mssql/qgsmssqlprovider.cpp | 4 +++- src/providers/spatialite/qgsspatialiteprovider.cpp | 4 ++-- tests/src/python/test_provider_ogr.py | 3 +-- tests/src/python/test_provider_spatialite.py | 5 ++--- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/core/providers/ogr/qgsogrprovider.cpp b/src/core/providers/ogr/qgsogrprovider.cpp index 1b7a39481a38..dda333b8f7f2 100644 --- a/src/core/providers/ogr/qgsogrprovider.cpp +++ b/src/core/providers/ogr/qgsogrprovider.cpp @@ -1258,8 +1258,8 @@ QVariant QgsOgrProvider::defaultValue( int fieldId ) const } } - ( void )mAttributeFields.at( fieldId ).convertCompatible( resultVar ); - return resultVar; + const bool compatible = mAttributeFields.at( fieldId ).convertCompatible( resultVar ); + return compatible && !QgsVariantUtils::isNull( resultVar ) ? resultVar : QVariant(); } QString QgsOgrProvider::defaultValueClause( int fieldIndex ) const diff --git a/src/providers/mssql/qgsmssqlprovider.cpp b/src/providers/mssql/qgsmssqlprovider.cpp index 2aa5af324224..4cf649118868 100644 --- a/src/providers/mssql/qgsmssqlprovider.cpp +++ b/src/providers/mssql/qgsmssqlprovider.cpp @@ -21,6 +21,7 @@ #include "qgsmssqlproviderconnection.h" #include "qgsfeedback.h" #include "qgsdbquerylog.h" +#include "qgsvariantutils.h" #include #include @@ -685,7 +686,8 @@ QVariant QgsMssqlProvider::defaultValue( int fieldId ) const return QVariant(); } - return query.value( 0 ); + const QVariant res = query.value( 0 ); + return QgsVariantUtils::isNull( res ) ? QVariant() : res; } QString QgsMssqlProvider::storageType() const diff --git a/src/providers/spatialite/qgsspatialiteprovider.cpp b/src/providers/spatialite/qgsspatialiteprovider.cpp index fa4609dca806..ee69772f3292 100644 --- a/src/providers/spatialite/qgsspatialiteprovider.cpp +++ b/src/providers/spatialite/qgsspatialiteprovider.cpp @@ -1103,8 +1103,8 @@ QVariant QgsSpatiaLiteProvider::defaultValue( int fieldId ) const } } - ( void )mAttributeFields.at( fieldId ).convertCompatible( resultVar ); - return resultVar; + const bool compatible = mAttributeFields.at( fieldId ).convertCompatible( resultVar ); + return compatible && !QgsVariantUtils::isNull( resultVar ) ? resultVar : QVariant(); } QString QgsSpatiaLiteProvider::defaultValueClause( int fieldIndex ) const diff --git a/tests/src/python/test_provider_ogr.py b/tests/src/python/test_provider_ogr.py index 35db32061cb9..d90d4bd3c875 100644 --- a/tests/src/python/test_provider_ogr.py +++ b/tests/src/python/test_provider_ogr.py @@ -1146,8 +1146,7 @@ def testSpatialiteDefaultValues(self): # Test default values dp = vl.dataProvider() - # FIXME: should it be None? - self.assertTrue(dp.defaultValue(0).isNull()) + self.assertEqual(dp.defaultValue(0), NULL) self.assertIsNone(dp.defaultValue(1)) # FIXME: This fails because there is no backend-side evaluation in this provider # self.assertTrue(dp.defaultValue(2).startswith(now.strftime('%Y-%m-%d'))) diff --git a/tests/src/python/test_provider_spatialite.py b/tests/src/python/test_provider_spatialite.py index 156d3e357cd4..498a13f98e71 100644 --- a/tests/src/python/test_provider_spatialite.py +++ b/tests/src/python/test_provider_spatialite.py @@ -1552,9 +1552,8 @@ def testSpatialiteDefaultValues(self): # Test default values dp = vl.dataProvider() - # FIXME: should it be None? - self.assertTrue(dp.defaultValue(0).isNull()) - self.assertIsNone(dp.defaultValue(1)) + self.assertEqual(dp.defaultValue(0), NULL) + self.assertEqual(dp.defaultValue(1), NULL) # FIXME: This fails because there is no backend-side evaluation in this provider # self.assertTrue(dp.defaultValue(2).startswith(now.strftime('%Y-%m-%d'))) self.assertTrue(dp.defaultValue( From 7e2296f054add2e9536cfdbcfea3036d4b9237c8 Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Wed, 28 Feb 2024 22:17:49 +0100 Subject: [PATCH 07/39] Update test_provider_spatialite.py: add NULL import --- tests/src/python/test_provider_spatialite.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/src/python/test_provider_spatialite.py b/tests/src/python/test_provider_spatialite.py index 498a13f98e71..938185e72abf 100644 --- a/tests/src/python/test_provider_spatialite.py +++ b/tests/src/python/test_provider_spatialite.py @@ -39,6 +39,7 @@ QgsVectorLayerExporter, QgsVectorLayerUtils, QgsWkbTypes, + NULL, ) import unittest from qgis.testing import start_app, QgisTestCase From b1ac747f51d7bd3c366304f08d4bee21958ca137 Mon Sep 17 00:00:00 2001 From: Mathieu Pellerin Date: Mon, 4 Mar 2024 12:21:01 +0700 Subject: [PATCH 08/39] [map canvas] Fix clearing of temporal and elevation triggered cache for group layers --- src/gui/qgsmapcanvas.cpp | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/src/gui/qgsmapcanvas.cpp b/src/gui/qgsmapcanvas.cpp index fc450fd3744f..0f26fbe7457f 100644 --- a/src/gui/qgsmapcanvas.cpp +++ b/src/gui/qgsmapcanvas.cpp @@ -48,6 +48,7 @@ email : sherman at mrcc.com #include "qgsapplication.h" #include "qgsexception.h" #include "qgsfeatureiterator.h" +#include "qgsgrouplayer.h" #include "qgslogger.h" #include "qgsmapcanvas.h" #include "qgsmapcanvasmap.h" @@ -1052,6 +1053,21 @@ void QgsMapCanvas::clearTemporalCache() if ( !alreadyInvalidatedThisLayer ) mCache->invalidateCacheForLayer( layer ); } + else if ( QgsGroupLayer *gl = qobject_cast( layer ) ) + { + const QList childLayerList = gl->childLayers(); + for ( QgsMapLayer *childLayer : childLayerList ) + { + if ( childLayer->temporalProperties() && childLayer->temporalProperties()->isActive() ) + { + if ( childLayer->temporalProperties()->flags() & QgsTemporalProperty::FlagDontInvalidateCachedRendersWhenRangeChanges ) + continue; + + mCache->invalidateCacheForLayer( layer ); + break; + } + } + } } if ( invalidateLabels ) @@ -1083,6 +1099,21 @@ void QgsMapCanvas::clearElevationCache() mCache->invalidateCacheForLayer( layer ); } + else if ( QgsGroupLayer *gl = qobject_cast( layer ) ) + { + const QList childLayerList = gl->childLayers(); + for ( QgsMapLayer *childLayer : childLayerList ) + { + if ( childLayer->elevationProperties() && childLayer->elevationProperties()->hasElevation() ) + { + if ( childLayer->elevationProperties()->flags() & QgsMapLayerElevationProperties::FlagDontInvalidateCachedRendersWhenRangeChanges ) + continue; + + mCache->invalidateCacheForLayer( layer ); + break; + } + } + } } if ( invalidateLabels ) From 8367ef43e1204997123f9047c790d50cc1d29b33 Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Mon, 4 Mar 2024 01:12:42 +0100 Subject: [PATCH 09/39] QgsGeos::asGeos(): only error out on invalid subgeometry only if requested (typically from QgsGeometryCollection::isValid()) --- src/core/geometry/qgsgeometrycollection.cpp | 2 +- src/core/geometry/qgsgeos.cpp | 20 ++++++++++++++------ src/core/geometry/qgsgeos.h | 8 +++++--- 3 files changed, 20 insertions(+), 10 deletions(-) diff --git a/src/core/geometry/qgsgeometrycollection.cpp b/src/core/geometry/qgsgeometrycollection.cpp index 029b0c47bad8..1bd3dd723ac6 100644 --- a/src/core/geometry/qgsgeometrycollection.cpp +++ b/src/core/geometry/qgsgeometrycollection.cpp @@ -908,7 +908,7 @@ bool QgsGeometryCollection::isValid( QString &error, Qgis::GeometryValidityFlags return error.isEmpty(); } - QgsGeos geos( this ); + QgsGeos geos( this, /* precision = */ 0, /* allowInvalidSubGeom = */ false ); bool res = geos.isValid( &error, flags & Qgis::GeometryValidityFlag::AllowSelfTouchingHoles, nullptr ); if ( flags == 0 ) { diff --git a/src/core/geometry/qgsgeos.cpp b/src/core/geometry/qgsgeos.cpp index 22db5ced5573..f2492478c838 100644 --- a/src/core/geometry/qgsgeos.cpp +++ b/src/core/geometry/qgsgeos.cpp @@ -139,12 +139,12 @@ void geos::GeosDeleter::operator()( GEOSCoordSequence *sequence ) const ///@endcond -QgsGeos::QgsGeos( const QgsAbstractGeometry *geometry, double precision ) +QgsGeos::QgsGeos( const QgsAbstractGeometry *geometry, double precision, bool allowInvalidSubGeom ) : QgsGeometryEngine( geometry ) , mGeos( nullptr ) , mPrecision( precision ) { - cacheGeos(); + cacheGeos( allowInvalidSubGeom ); } QgsGeometry QgsGeos::geometryFromGeos( GEOSGeometry *geos ) @@ -246,7 +246,7 @@ void QgsGeos::geometryChanged() { mGeos.reset(); mGeosPrepared.reset(); - cacheGeos(); + cacheGeos( false ); } void QgsGeos::prepareGeometry() @@ -262,7 +262,7 @@ void QgsGeos::prepareGeometry() } } -void QgsGeos::cacheGeos() const +void QgsGeos::cacheGeos( bool allowInvalidSubGeom ) const { if ( mGeos ) { @@ -274,7 +274,7 @@ void QgsGeos::cacheGeos() const return; } - mGeos = asGeos( mGeometry, mPrecision ); + mGeos = asGeos( mGeometry, mPrecision, allowInvalidSubGeom ); } QgsAbstractGeometry *QgsGeos::intersection( const QgsAbstractGeometry *geom, QString *errorMsg, const QgsGeometryParameters ¶meters ) const @@ -1624,7 +1624,7 @@ QgsPoint QgsGeos::coordSeqPoint( const GEOSCoordSequence *cs, int i, bool hasZ, return QgsPoint( t, x, y, z, m ); } -geos::unique_ptr QgsGeos::asGeos( const QgsAbstractGeometry *geom, double precision ) +geos::unique_ptr QgsGeos::asGeos( const QgsAbstractGeometry *geom, double precision, bool allowInvalidSubGeom ) { if ( !geom ) return nullptr; @@ -1675,6 +1675,14 @@ geos::unique_ptr QgsGeos::asGeos( const QgsAbstractGeometry *geom, double precis for ( int i = 0; i < c->numGeometries(); ++i ) { geomVector[i] = asGeos( c->geometryN( i ), precision ).release(); + if ( !allowInvalidSubGeom && !geomVector[i] ) + { + for ( int j = 0; j < i; ++j ) + { + GEOSGeom_destroy_r( geosinit()->ctxt, geomVector[j] ); + } + return nullptr; + } } return createGeosCollection( geosType, geomVector ); } diff --git a/src/core/geometry/qgsgeos.h b/src/core/geometry/qgsgeos.h index 062f4730fc43..a47010ae531b 100644 --- a/src/core/geometry/qgsgeos.h +++ b/src/core/geometry/qgsgeos.h @@ -103,8 +103,9 @@ class CORE_EXPORT QgsGeos: public QgsGeometryEngine * GEOS geometry engine constructor * \param geometry The geometry * \param precision The precision of the grid to which to snap the geometry vertices. If 0, no snapping is performed. + * \param allowInvalidSubGeom Whether invalid sub-geometries should be skipped without error (since QGIS 3.38) */ - QgsGeos( const QgsAbstractGeometry *geometry, double precision = 0 ); + QgsGeos( const QgsAbstractGeometry *geometry, double precision = 0, bool allowInvalidSubGeom = true ); /** * Creates a new QgsGeometry object, feeding in a geometry in GEOS format. @@ -608,8 +609,9 @@ class CORE_EXPORT QgsGeos: public QgsGeometryEngine * Returns a geos geometry - caller takes ownership of the object (should be deleted with GEOSGeom_destroy_r) * \param geometry geometry to convert to GEOS representation * \param precision The precision of the grid to which to snap the geometry vertices. If 0, no snapping is performed. + * \param allowInvalidSubGeom Whether invalid sub-geometries should be skipped without error (since QGIS 3.38) */ - static geos::unique_ptr asGeos( const QgsAbstractGeometry *geometry, double precision = 0 ); + static geos::unique_ptr asGeos( const QgsAbstractGeometry *geometry, double precision = 0, bool allowInvalidSubGeom = true ); static QgsPoint coordSeqPoint( const GEOSCoordSequence *cs, int i, bool hasZ, bool hasM ); static GEOSContextHandle_t getGEOSHandler(); @@ -640,7 +642,7 @@ class CORE_EXPORT QgsGeos: public QgsGeometryEngine }; //geos util functions - void cacheGeos() const; + void cacheGeos( bool allowInvalidSubGeom ) const; /** * Returns a geometry representing the overlay operation with \a geom. From eb25c1c70ee277cfeba48183a7da8edb2cd99275 Mon Sep 17 00:00:00 2001 From: Mathieu Pellerin Date: Fri, 1 Mar 2024 11:26:33 +0700 Subject: [PATCH 10/39] [vector layer] Fix QgsVectorLayerFeatureIterator::isValid() value when initiated against editing-enabled vector layer --- .../vector/qgsvectorlayerfeatureiterator.cpp | 2 +- tests/src/python/test_qgsfeatureiterator.py | 23 +++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/src/core/vector/qgsvectorlayerfeatureiterator.cpp b/src/core/vector/qgsvectorlayerfeatureiterator.cpp index e1cf3fb8998e..b5374883663c 100644 --- a/src/core/vector/qgsvectorlayerfeatureiterator.cpp +++ b/src/core/vector/qgsvectorlayerfeatureiterator.cpp @@ -622,7 +622,7 @@ void QgsVectorLayerFeatureIterator::setInterruptionChecker( QgsFeedback *interru bool QgsVectorLayerFeatureIterator::isValid() const { - return mProviderIterator.isValid(); + return mChangedFeaturesIterator.isValid() || mProviderIterator.isValid(); } bool QgsVectorLayerFeatureIterator::fetchNextAddedFeature( QgsFeature &f ) diff --git a/tests/src/python/test_qgsfeatureiterator.py b/tests/src/python/test_qgsfeatureiterator.py index 93b58e859cc3..898a06e58614 100644 --- a/tests/src/python/test_qgsfeatureiterator.py +++ b/tests/src/python/test_qgsfeatureiterator.py @@ -116,6 +116,29 @@ def addFeatures(self, vl): feat['Staff'] = 2 vl.addFeature(feat) + def test_VectorLayerEditing(self): + ogr_layer = QgsVectorLayer(os.path.join(TEST_DATA_DIR, 'points.shp'), 'Points', 'ogr') + self.assertTrue(ogr_layer.isValid()) + + request = QgsFeatureRequest() + iterator = ogr_layer.getFeatures(request) + self.assertTrue(iterator.isValid()) + + self.assertTrue(ogr_layer.startEditing()) + iterator = ogr_layer.getFeatures(request) + self.assertTrue(iterator.isValid()) + + memory_layer = QgsVectorLayer("Point?field=x:string&field=y:integer&field=z:integer", "layer", "memory") + self.assertTrue(memory_layer.isValid()) + + request = QgsFeatureRequest() + iterator = memory_layer.getFeatures(request) + self.assertTrue(iterator.isValid()) + + self.assertTrue(memory_layer.startEditing()) + iterator = memory_layer.getFeatures(request) + self.assertTrue(iterator.isValid()) + def test_ExpressionFieldNested(self): myShpFile = os.path.join(TEST_DATA_DIR, 'points.shp') layer = QgsVectorLayer(myShpFile, 'Points', 'ogr') From 5c56196092db90e5808f985c8ce000d8223e7150 Mon Sep 17 00:00:00 2001 From: Andrea Giudiceandrea Date: Mon, 4 Mar 2024 19:09:30 +0100 Subject: [PATCH 11/39] [processing] Fix "Random points along line" alg Avoid various errors in case of empty input layer, empty/null geometries, invalid geometries, linestrings with zero-length segments... --- .../algs/qgis/RandomPointsAlongLines.py | 24 +++++++++++++++---- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/python/plugins/processing/algs/qgis/RandomPointsAlongLines.py b/python/plugins/processing/algs/qgis/RandomPointsAlongLines.py index 2ddfae8fd2a5..d97fee89f417 100644 --- a/python/plugins/processing/algs/qgis/RandomPointsAlongLines.py +++ b/python/plugins/processing/algs/qgis/RandomPointsAlongLines.py @@ -100,7 +100,6 @@ def processAlgorithm(self, parameters, context, feedback): nPoints = 0 nIterations = 0 maxIterations = pointCount * 200 - featureCount = source.featureCount() total = 100.0 / pointCount if pointCount else 1 index = QgsSpatialIndex() @@ -116,14 +115,22 @@ def processAlgorithm(self, parameters, context, feedback): ids = source.allFeatureIds() - while nIterations < maxIterations and nPoints < pointCount: + while ids and nIterations < maxIterations and nPoints < pointCount: if feedback.isCanceled(): break # pick random feature fid = random.choice(ids) - f = next(source.getFeatures(request.setFilterFid(fid).setSubsetOfAttributes([]))) + try: + f = next(source.getFeatures(request.setFilterFid(fid).setSubsetOfAttributes([]))) + except: + ids.remove(fid) + continue + fGeom = f.geometry() + if fGeom.isEmpty(): + ids.remove(fid) + continue if fGeom.isMultipart(): lines = fGeom.asMultiPolyline() @@ -134,13 +141,20 @@ def processAlgorithm(self, parameters, context, feedback): vertices = fGeom.asPolyline() # pick random segment - if len(vertices) == 2: + nVertices = len(vertices) + if nVertices < 2: + nIterations += 1 + continue + if nVertices == 2: vid = 0 else: - vid = random.randint(0, len(vertices) - 2) + vid = random.randint(0, nVertices - 2) startPoint = vertices[vid] endPoint = vertices[vid + 1] length = da.measureLine(startPoint, endPoint) + if length == 0: + nIterations += 1 + continue dist = length * random.random() d = dist / (length - dist) From 4bfc618311d3712862dc4c3a66b3006c3ac7d936 Mon Sep 17 00:00:00 2001 From: Andrea Giudiceandrea Date: Mon, 4 Mar 2024 21:43:28 +0100 Subject: [PATCH 12/39] Apply suggestion from code review --- python/plugins/processing/algs/qgis/RandomPointsAlongLines.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/plugins/processing/algs/qgis/RandomPointsAlongLines.py b/python/plugins/processing/algs/qgis/RandomPointsAlongLines.py index d97fee89f417..6fdb5d5b9c3a 100644 --- a/python/plugins/processing/algs/qgis/RandomPointsAlongLines.py +++ b/python/plugins/processing/algs/qgis/RandomPointsAlongLines.py @@ -123,7 +123,7 @@ def processAlgorithm(self, parameters, context, feedback): fid = random.choice(ids) try: f = next(source.getFeatures(request.setFilterFid(fid).setSubsetOfAttributes([]))) - except: + except StopIteration: ids.remove(fid) continue From b606fbab18c6f3e78c4067754f565913a41b499a Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Wed, 6 Mar 2024 14:04:16 +1000 Subject: [PATCH 13/39] [gps] Don't emit stateChanged for incomplete nmea messages If we only have receieved the first part of a message, don't emit stateChanged until we've received the rest. Possibly refs #56460 --- src/core/gps/qgsnmeaconnection.cpp | 6 +++- tests/src/core/testqgsnmeaconnection.cpp | 35 ++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/src/core/gps/qgsnmeaconnection.cpp b/src/core/gps/qgsnmeaconnection.cpp index 4460f7c684d8..69e742977bdf 100644 --- a/src/core/gps/qgsnmeaconnection.cpp +++ b/src/core/gps/qgsnmeaconnection.cpp @@ -68,7 +68,11 @@ void QgsNmeaConnection::parseData() //append new data to the remaining results from last parseData() call mStringBuffer.append( mSource->read( numBytes ) ); processStringBuffer(); - emit stateChanged( mLastGPSInformation ); + + if ( mStatus == GPSDataReceived ) + { + emit stateChanged( mLastGPSInformation ); + } } } diff --git a/tests/src/core/testqgsnmeaconnection.cpp b/tests/src/core/testqgsnmeaconnection.cpp index 7618f7d97ba2..64feb5a916cf 100644 --- a/tests/src/core/testqgsnmeaconnection.cpp +++ b/tests/src/core/testqgsnmeaconnection.cpp @@ -50,6 +50,14 @@ class ReplayNmeaConnection : public QgsNmeaConnection return spy.constLast().at( 0 ).value< QgsGpsInformation >(); } + void pushString( const QString &string ) + { + const qint64 pos = mBuffer->pos(); + mBuffer->write( string.toLocal8Bit().constData() ); + mBuffer->seek( pos ); + parseData(); + } + private: QBuffer *mBuffer = nullptr; @@ -74,6 +82,7 @@ class TestQgsNmeaConnection : public QgsTest void testConstellation(); void testPosition(); void testComponent(); + void testIncompleteMessage(); }; @@ -468,5 +477,31 @@ void TestQgsNmeaConnection::testComponent() QCOMPARE( info.componentValue( Qgis::GpsInformationComponent::Bearing ).toDouble(), 2 ); } +void TestQgsNmeaConnection::testIncompleteMessage() +{ + ReplayNmeaConnection connection; + QSignalSpy stateChangedSpy( &connection, &QgsNmeaConnection::stateChanged ); + + QCOMPARE( connection.status(), QgsGpsConnection::Status::Connected ); + + // start with an incomplete message + connection.pushString( QStringLiteral( "$GPGGA," ) ); + // status should be "data received", we don't have the full sentence yet + QCOMPARE( connection.status(), QgsGpsConnection::Status::DataReceived ); + // should be no stateChanged signal yet, we are still waiting on more data + QCOMPARE( stateChangedSpy.size(), 0 ); + + connection.pushString( QStringLiteral( "084112.185,6900.0,N,01800.0,E,1,04,1.4,35.0,M,29.4,M,,0000*63\r\n" ) ); + // got a full sentence now, status should be "data received" + QCOMPARE( connection.status(), QgsGpsConnection::Status::GPSDataReceived ); + QCOMPARE( stateChangedSpy.size(), 1 ); + const QgsGpsInformation info = stateChangedSpy.at( 0 ).at( 0 ).value< QgsGpsInformation >(); + + QCOMPARE( info.componentValue( Qgis::GpsInformationComponent::Location ).value< QgsPointXY >(), QgsPointXY( 18, 69 ) ); + QCOMPARE( info.componentValue( Qgis::GpsInformationComponent::Altitude ).toDouble(), 35 ); + QCOMPARE( info.componentValue( Qgis::GpsInformationComponent::GroundSpeed ).toDouble(), 0 ); + QCOMPARE( info.componentValue( Qgis::GpsInformationComponent::Bearing ).toDouble(), 0 ); +} + QGSTEST_MAIN( TestQgsNmeaConnection ) #include "testqgsnmeaconnection.moc" From 9e4b8ef0d04ae3a937f78e777234f2d2d645aab0 Mon Sep 17 00:00:00 2001 From: Jean Felder Date: Tue, 5 Mar 2024 16:11:11 +0100 Subject: [PATCH 14/39] qgsshadowrenderingframegraph: Fix blendequation for transparent entities See discussion from https://github.com/qgis/QGIS/pull/55943#issuecomment-1972177646 --- src/3d/qgsshadowrenderingframegraph.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/3d/qgsshadowrenderingframegraph.cpp b/src/3d/qgsshadowrenderingframegraph.cpp index 70e49e5da83e..83b8d5e34a51 100644 --- a/src/3d/qgsshadowrenderingframegraph.cpp +++ b/src/3d/qgsshadowrenderingframegraph.cpp @@ -186,10 +186,8 @@ Qt3DRender::QFrameGraphNode *QgsShadowRenderingFrameGraph::constructForwardRende transparentObjectsRenderStateSet->addRenderState( blendEquation ); Qt3DRender::QBlendEquationArguments *blenEquationArgs = new Qt3DRender::QBlendEquationArguments; - blenEquationArgs->setSourceRgb( Qt3DRender::QBlendEquationArguments::Blending::One ); - blenEquationArgs->setDestinationRgb( Qt3DRender::QBlendEquationArguments::Blending::OneMinusSource1Alpha ); - blenEquationArgs->setSourceAlpha( Qt3DRender::QBlendEquationArguments::Blending::One ); - blenEquationArgs->setDestinationAlpha( Qt3DRender::QBlendEquationArguments::Blending::OneMinusSource1Alpha ); + blenEquationArgs->setSourceRgb( Qt3DRender::QBlendEquationArguments::Blending::SourceAlpha ); + blenEquationArgs->setDestinationRgb( Qt3DRender::QBlendEquationArguments::Blending::OneMinusSourceAlpha ); transparentObjectsRenderStateSet->addRenderState( blenEquationArgs ); } From f4a0aba8c6ca2d8a5a621d754edc28ba80adbeef Mon Sep 17 00:00:00 2001 From: Jacky Volpes Date: Wed, 6 Mar 2024 11:40:19 +0100 Subject: [PATCH 15/39] Fix global QGIS setting key in check validity algorithm --- python/plugins/processing/algs/qgis/CheckValidity.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/plugins/processing/algs/qgis/CheckValidity.py b/python/plugins/processing/algs/qgis/CheckValidity.py index 855662bb40d0..e5220f88faa9 100644 --- a/python/plugins/processing/algs/qgis/CheckValidity.py +++ b/python/plugins/processing/algs/qgis/CheckValidity.py @@ -44,7 +44,7 @@ QgsProcessingParameterBoolean) from processing.algs.qgis.QgisAlgorithm import QgisAlgorithm -settings_method_key = "/qgis/digitizing/validate_geometries" +settings_method_key = "/digitizing/validate-geometries" pluginPath = os.path.split(os.path.split(os.path.dirname(__file__))[0])[0] From b51f985da7a91924c07776f1bae5b10da6d14270 Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Thu, 7 Mar 2024 03:47:18 +0100 Subject: [PATCH 16/39] [Server] QgsWmsRenderContext::isValidWidthHeight(): rewrite sanity checks in a foolproof way The check for ``width * depth + 31`` not overflowing INT_MAX was done after computing it, which would be undefined behavior if that happened. Nowadays compilers can use "impossible situations" in smart ways, and could very well discard the check if it is done afterwards. Be on the safe sife and check before, as done in https://code.qt.io/cgit/qt/qtbase.git/tree/src/gui/image/qimage_p.h#n89 --- .../services/wms/qgswmsrendercontext.cpp | 16 +++++--- tests/src/python/test_qgsserver_wms_getmap.py | 38 +++++++++++++++++++ 2 files changed, 48 insertions(+), 6 deletions(-) diff --git a/src/server/services/wms/qgswmsrendercontext.cpp b/src/server/services/wms/qgswmsrendercontext.cpp index 259904771f30..9a86c5873199 100644 --- a/src/server/services/wms/qgswmsrendercontext.cpp +++ b/src/server/services/wms/qgswmsrendercontext.cpp @@ -636,6 +636,9 @@ bool QgsWmsRenderContext::isValidWidthHeight() const bool QgsWmsRenderContext::isValidWidthHeight( int width, int height ) const { + if ( width <= 0 || height <= 0 ) + return false; + //test if maxWidth / maxHeight are set in the project or as an env variable //and WIDTH / HEIGHT parameter is in the range allowed range //WIDTH @@ -678,14 +681,15 @@ bool QgsWmsRenderContext::isValidWidthHeight( int width, int height ) const return false; } - // Sanity check from internal QImage checks (see qimage.cpp) + // Sanity check from internal QImage checks + // (see QImageData::calculateImageParameters() in qimage_p.h) // this is to report a meaningful error message in case of // image creation failure and to differentiate it from out // of memory conditions. // depth for now it cannot be anything other than 32, but I don't like // to hardcode it: I hope we will support other depths in the future. - uint depth = 32; + int depth = 32; switch ( mParameters.format() ) { case QgsWmsParameters::Format::JPG: @@ -694,12 +698,12 @@ bool QgsWmsRenderContext::isValidWidthHeight( int width, int height ) const depth = 32; } + if ( width > ( std::numeric_limits::max() - 31 ) / depth ) + return false; + const int bytes_per_line = ( ( width * depth + 31 ) >> 5 ) << 2; // bytes per scanline (must be multiple of 4) - if ( std::numeric_limits::max() / depth < static_cast( width ) - || bytes_per_line <= 0 - || height <= 0 - || std::numeric_limits::max() / static_cast( bytes_per_line ) < static_cast( height ) + if ( std::numeric_limits::max() / bytes_per_line < height || std::numeric_limits::max() / sizeof( uchar * ) < static_cast( height ) ) { return false; diff --git a/tests/src/python/test_qgsserver_wms_getmap.py b/tests/src/python/test_qgsserver_wms_getmap.py index a4a57460af9c..6956bff2cc64 100644 --- a/tests/src/python/test_qgsserver_wms_getmap.py +++ b/tests/src/python/test_qgsserver_wms_getmap.py @@ -336,6 +336,25 @@ def test_wms_getmap_invalid_parameters(self): err = b"HEIGHT (\'FOO\') cannot be converted into int" in r self.assertTrue(err) + # height should be > 0 + qs = "?" + "&".join(["%s=%s" % i for i in list({ + "MAP": urllib.parse.quote(self.projectPath), + "SERVICE": "WMS", + "VERSION": "1.1.1", + "REQUEST": "GetMap", + "LAYERS": "Country", + "STYLES": "", + "FORMAT": "image/png", + "BBOX": "-16817707,-4710778,5696513,14587125", + "HEIGHT": "-1", + "WIDTH": "1", + "CRS": "EPSG:3857" + }.items())]) + + r, h = self._result(self._execute_request(qs)) + err = b"The requested map size is too large" in r + self.assertTrue(err) + # width should be an int qs = "?" + "&".join(["%s=%s" % i for i in list({ "MAP": urllib.parse.quote(self.projectPath), @@ -355,6 +374,25 @@ def test_wms_getmap_invalid_parameters(self): err = b"WIDTH (\'FOO\') cannot be converted into int" in r self.assertTrue(err) + # width should be > 0 + qs = "?" + "&".join(["%s=%s" % i for i in list({ + "MAP": urllib.parse.quote(self.projectPath), + "SERVICE": "WMS", + "VERSION": "1.1.1", + "REQUEST": "GetMap", + "LAYERS": "Country", + "STYLES": "", + "FORMAT": "image/png", + "BBOX": "-16817707,-4710778,5696513,14587125", + "HEIGHT": "1", + "WIDTH": "-1", + "CRS": "EPSG:3857" + }.items())]) + + r, h = self._result(self._execute_request(qs)) + err = b"The requested map size is too large" in r + self.assertTrue(err) + # bbox should be formatted like "double,double,double,double" qs = "?" + "&".join(["%s=%s" % i for i in list({ "MAP": urllib.parse.quote(self.projectPath), From 7994e9324325c736afd3cca5386253f376cf2ebc Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Thu, 7 Mar 2024 08:33:08 +1000 Subject: [PATCH 17/39] [console] Don't needlessly store QgsSettings objects These are cheap to construct by design, so we can avoid storing them and the messy cross-class access to member variables --- python/console/console.py | 29 +++++++++++++++-------------- python/console/console_editor.py | 23 ++++++++++------------- python/console/console_output.py | 4 +--- python/console/console_sci.py | 2 -- 4 files changed, 26 insertions(+), 32 deletions(-) diff --git a/python/console/console.py b/python/console/console.py index db96ba844034..ba2d96ba3a74 100644 --- a/python/console/console.py +++ b/python/console/console.py @@ -145,8 +145,6 @@ def __init__(self, parent=None): QWidget.__init__(self, parent) self.setWindowTitle(QCoreApplication.translate("PythonConsole", "Python Console")) - self.settings = QgsSettings() - self.shell = ShellScintilla(self) self.setFocusProxy(self.shell) self.shellOut = ShellOutputScintilla(self) @@ -310,7 +308,7 @@ def __init__(self, parent=None): objList = QCoreApplication.translate("PythonConsole", "Object Inspector…") self.objectListButton = QAction(self) self.objectListButton.setCheckable(True) - self.objectListButton.setEnabled(self.settings.value("pythonConsole/enableObjectInsp", + self.objectListButton.setEnabled(QgsSettings().value("pythonConsole/enableObjectInsp", False, type=bool)) self.objectListButton.setIcon(QgsApplication.getThemeIcon("console/iconClassBrowserConsole.svg")) self.objectListButton.setMenuRole(QAction.PreferencesRole) @@ -675,7 +673,8 @@ def openScriptFileExtEditor(self): QDesktopServices.openUrl(QUrl.fromLocalFile(path)) def openScriptFile(self): - lastDirPath = self.settings.value("pythonConsole/lastDirPath", QDir.homePath()) + settings = QgsSettings() + lastDirPath = settings.value("pythonConsole/lastDirPath", QDir.homePath()) openFileTr = QCoreApplication.translate("PythonConsole", "Open File") fileList, selected_filter = QFileDialog.getOpenFileNames( self, openFileTr, lastDirPath, "Script file (*.py)") @@ -691,7 +690,7 @@ def openScriptFile(self): self.tabEditorWidget.newTabEditor(tabName, pyFile) lastDirPath = QFileInfo(pyFile).path() - self.settings.setValue("pythonConsole/lastDirPath", pyFile) + settings.setValue("pythonConsole/lastDirPath", pyFile) self.updateTabListScript(pyFile, action='append') def saveScriptFile(self): @@ -710,7 +709,7 @@ def saveAsScriptFile(self, index=None): index = self.tabEditorWidget.currentIndex() if not tabWidget.path: fileName = self.tabEditorWidget.tabText(index).replace('*', '') + '.py' - folder = self.settings.value("pythonConsole/lastDirPath", QDir.homePath()) + folder = QgsSettings().value("pythonConsole/lastDirPath", QDir.homePath()) pathFileName = os.path.join(folder, fileName) fileNone = True else: @@ -776,22 +775,24 @@ def updateTabListScript(self, script, action=None): self.tabListScript.append(script) else: self.tabListScript = [] - self.settings.setValue("pythonConsole/tabScripts", + QgsSettings().setValue("pythonConsole/tabScripts", self.tabListScript) def saveSettingsConsole(self): - self.settings.setValue("pythonConsole/splitterConsole", self.splitter.saveState()) - self.settings.setValue("pythonConsole/splitterObj", self.splitterObj.saveState()) - self.settings.setValue("pythonConsole/splitterEditor", self.splitterEditor.saveState()) + settings = QgsSettings() + settings.setValue("pythonConsole/splitterConsole", self.splitter.saveState()) + settings.setValue("pythonConsole/splitterObj", self.splitterObj.saveState()) + settings.setValue("pythonConsole/splitterEditor", self.splitterEditor.saveState()) self.shell.writeHistoryFile() def restoreSettingsConsole(self): - storedTabScripts = self.settings.value("pythonConsole/tabScripts", []) + settings = QgsSettings() + storedTabScripts = settings.value("pythonConsole/tabScripts", []) self.tabListScript = storedTabScripts - self.splitter.restoreState(self.settings.value("pythonConsole/splitterConsole", QByteArray())) - self.splitterEditor.restoreState(self.settings.value("pythonConsole/splitterEditor", QByteArray())) - self.splitterObj.restoreState(self.settings.value("pythonConsole/splitterObj", QByteArray())) + self.splitter.restoreState(settings.value("pythonConsole/splitterConsole", QByteArray())) + self.splitterEditor.restoreState(settings.value("pythonConsole/splitterEditor", QByteArray())) + self.splitterObj.restoreState(settings.value("pythonConsole/splitterObj", QByteArray())) if __name__ == '__main__': diff --git a/python/console/console_editor.py b/python/console/console_editor.py index 8c20b2792868..c7295f08019a 100644 --- a/python/console/console_editor.py +++ b/python/console/console_editor.py @@ -63,7 +63,6 @@ def __init__(self, parent=None): self.path = None # recent modification time self.lastModified = 0 - self.settings = QgsSettings() self.setMinimumHeight(120) self.setVerticalScrollBarPolicy(Qt.ScrollBarAsNeeded) @@ -186,7 +185,7 @@ def contextMenuEvent(self, e): redoAction.setEnabled(True) if QApplication.clipboard().text(): pasteAction.setEnabled(True) - if self.settings.value("pythonConsole/enableObjectInsp", + if QgsSettings().value("pythonConsole/enableObjectInsp", False, type=bool): showCodeInspection.setEnabled(True) menu.exec_(self.mapToGlobal(e.pos())) @@ -239,7 +238,7 @@ def objectListEditor(self): self.pythonconsole.objectListButton.setChecked(True) def shareOnGist(self, is_public): - ACCESS_TOKEN = self.settings.value("pythonConsole/accessTokenGithub", '', type=QByteArray) + ACCESS_TOKEN = QgsSettings().value("pythonConsole/accessTokenGithub", '', type=QByteArray) if not ACCESS_TOKEN: msg_text = QCoreApplication.translate( 'PythonConsole', 'GitHub personal access token must be generated (see Console Options)') @@ -307,7 +306,7 @@ def createTempFile(self): return name def runScriptCode(self): - autoSave = self.settings.value("pythonConsole/autoSaveScript", False, type=bool) + autoSave = QgsSettings().value("pythonConsole/autoSaveScript", False, type=bool) tabWidget = self.tabwidget.currentWidget() filename = tabWidget.path msgEditorBlank = QCoreApplication.translate('PythonConsole', @@ -408,7 +407,7 @@ def save(self, filename=None): if self.isReadOnly(): return - if self.pythonconsole.settings.value("pythonConsole/formatOnSave", False, type=bool): + if QgsSettings().value("pythonConsole/formatOnSave", False, type=bool): self.reformatCode() tabwidget = self.tabwidget @@ -418,7 +417,7 @@ def save(self, filename=None): if self.path is None: saveTr = QCoreApplication.translate('PythonConsole', 'Python Console: Save file') - folder = self.pythonconsole.settings.value("pythonConsole/lastDirPath", QDir.homePath()) + folder = QgsSettings().value("pythonConsole/lastDirPath", QDir.homePath()) self.path, filter = QFileDialog().getSaveFileName(self, saveTr, os.path.join(folder, tabwidget.tabText(index).replace('*', '') + '.py'), @@ -444,7 +443,7 @@ def save(self, filename=None): self.pythonconsole.updateTabListScript(self.path, action='append') tabwidget.listObject(self.parent) lastDirPath = str(Path(self.path).parent) - self.pythonconsole.settings.setValue("pythonConsole/lastDirPath", lastDirPath) + QgsSettings().setValue("pythonConsole/lastDirPath", lastDirPath) def event(self, e): """ Used to override the Application shortcuts when the editor has focus """ @@ -553,8 +552,6 @@ def __init__(self, parent): super().__init__(parent=None) self.parent = parent - self.settings = QgsSettings() - self.idx = -1 # Layout for top frame (restore tabs) self.layoutTopFrame = QGridLayout(self) @@ -641,7 +638,7 @@ def __init__(self, parent): self.newTabButton.clicked.connect(self.newTabEditor) def _currentWidgetChanged(self, tab): - if self.settings.value("pythonConsole/enableObjectInsp", + if QgsSettings().value("pythonConsole/enableObjectInsp", False, type=bool): self.listObject(tab) self.changeLastDirPath(tab) @@ -788,7 +785,7 @@ def restoreTabsOrAddNew(self): Restore tabs if they are found in the settings. If none are found it will add a new empty tab. """ # Restore scripts from the previous session - tabScripts = self.settings.value("pythonConsole/tabScripts", []) + tabScripts = QgsSettings().value("pythonConsole/tabScripts", []) self.restoreTabList = tabScripts if self.restoreTabList: @@ -911,7 +908,7 @@ def listObject(self, tab): self.parent.listClassMethod.addTopLevelItem(msgItem) def refreshSettingsEditor(self): - objInspectorEnabled = self.settings.value("pythonConsole/enableObjectInsp", + objInspectorEnabled = QgsSettings().value("pythonConsole/enableObjectInsp", False, type=bool) listObj = self.parent.objectListButton if self.parent.listClassMethod.isVisible(): @@ -926,7 +923,7 @@ def refreshSettingsEditor(self): def changeLastDirPath(self, tab): tabWidget = self.widget(tab) if tabWidget and tabWidget.path: - self.settings.setValue("pythonConsole/lastDirPath", tabWidget.path) + QgsSettings().setValue("pythonConsole/lastDirPath", tabWidget.path) def showMessage(self, text, level=Qgis.Info, timeout=-1, title=""): currWidget = self.currentWidget() diff --git a/python/console/console_output.py b/python/console/console_output.py index cd5c3b513c21..d3e3a71552ff 100644 --- a/python/console/console_output.py +++ b/python/console/console_output.py @@ -54,7 +54,7 @@ def write(self, m): if self.style == "_traceback": # Show errors in red - stderrColor = QColor(self.sO.settings.value("pythonConsole/stderrFontColor", QColor(self.ERROR_COLOR))) + stderrColor = QColor(QgsSettings().value("pythonConsole/stderrFontColor", QColor(self.ERROR_COLOR))) self.sO.SendScintilla(QsciScintilla.SCI_STYLESETFORE, 0o01, stderrColor) self.sO.SendScintilla(QsciScintilla.SCI_STYLESETITALIC, 0o01, True) self.sO.SendScintilla(QsciScintilla.SCI_STYLESETBOLD, 0o01, True) @@ -122,8 +122,6 @@ def __init__(self, parent=None): self.parent = parent self.shell = self.parent.shell - self.settings = QgsSettings() - # Creates layout for message bar self.layout = QGridLayout(self) self.layout.setContentsMargins(0, 0, 0, 0) diff --git a/python/console/console_sci.py b/python/console/console_sci.py index 9f9034a59c96..7ce92310a553 100644 --- a/python/console/console_sci.py +++ b/python/console/console_sci.py @@ -288,8 +288,6 @@ def __init__(self, parent=None): self.opening = ['(', '{', '[', "'", '"'] self.closing = [')', '}', ']', "'", '"'] - self.settings = QgsSettings() - self.setHistoryFilePath( os.path.join(QgsApplication.qgisSettingsDirPath(), "console_history.txt")) From e20e793c3ed6479e15d7f7c26357f93dfcd6d570 Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Thu, 7 Mar 2024 08:47:20 +1000 Subject: [PATCH 18/39] [console] Ensure stored last dir path settings is always a folder This prevents a bug where the save action in the Python console keeps defaulting back to the binary QGIS install folder --- python/console/console_editor.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/python/console/console_editor.py b/python/console/console_editor.py index c7295f08019a..c02197cb1caf 100644 --- a/python/console/console_editor.py +++ b/python/console/console_editor.py @@ -923,7 +923,8 @@ def refreshSettingsEditor(self): def changeLastDirPath(self, tab): tabWidget = self.widget(tab) if tabWidget and tabWidget.path: - QgsSettings().setValue("pythonConsole/lastDirPath", tabWidget.path) + QgsSettings().setValue("pythonConsole/lastDirPath", + Path(tabWidget.path).parent.as_posix()) def showMessage(self, text, level=Qgis.Info, timeout=-1, title=""): currWidget = self.currentWidget() From b81cdef45bd616cfce30d8c23f06cd1eb3475c87 Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Thu, 7 Mar 2024 08:48:14 +1000 Subject: [PATCH 19/39] [console] Minor cleanups - Add some typehints - Avoid creating an unnecessary local variable --- python/console/console_editor.py | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/python/console/console_editor.py b/python/console/console_editor.py index c02197cb1caf..3b4d77764b55 100644 --- a/python/console/console_editor.py +++ b/python/console/console_editor.py @@ -26,6 +26,7 @@ import re import sys import tempfile +from typing import Optional from functools import partial from operator import itemgetter from pathlib import Path @@ -60,7 +61,7 @@ class Editor(QgsCodeEditorPython): def __init__(self, parent=None): super().__init__(parent) self.parent = parent - self.path = None + self.path: Optional[str] = None # recent modification time self.lastModified = 0 @@ -403,24 +404,23 @@ def loadFile(self, filename, readOnly=False): self.setModified(False) self.recolor() - def save(self, filename=None): + def save(self, filename: Optional[str] = None): if self.isReadOnly(): return if QgsSettings().value("pythonConsole/formatOnSave", False, type=bool): self.reformatCode() - tabwidget = self.tabwidget - index = tabwidget.indexOf(self.parent) + index = self.tabwidget.indexOf(self.parent) if filename: self.path = filename - if self.path is None: + if not self.path: saveTr = QCoreApplication.translate('PythonConsole', 'Python Console: Save file') folder = QgsSettings().value("pythonConsole/lastDirPath", QDir.homePath()) self.path, filter = QFileDialog().getSaveFileName(self, saveTr, - os.path.join(folder, tabwidget.tabText(index).replace('*', '') + '.py'), + os.path.join(folder, self.tabwidget.tabText(index).replace('*', '') + '.py'), "Script file (*.py)") # If the user didn't select a file, abort the save operation if not self.path: @@ -435,15 +435,15 @@ def save(self, filename=None): # Need to use newline='' to avoid adding extra \r characters on Windows with open(self.path, 'w', encoding='utf-8', newline='') as f: f.write(self.text()) - tabwidget.setTabTitle(index, Path(self.path).name) - tabwidget.setTabToolTip(index, self.path) + self.tabwidget.setTabTitle(index, Path(self.path).name) + self.tabwidget.setTabToolTip(index, self.path) self.setModified(False) self.pythonconsole.saveFileButton.setEnabled(False) self.lastModified = QFileInfo(self.path).lastModified() self.pythonconsole.updateTabListScript(self.path, action='append') - tabwidget.listObject(self.parent) - lastDirPath = str(Path(self.path).parent) - QgsSettings().setValue("pythonConsole/lastDirPath", lastDirPath) + self.tabwidget.listObject(self.parent) + QgsSettings().setValue("pythonConsole/lastDirPath", + Path(self.path).parent.as_posix()) def event(self, e): """ Used to override the Application shortcuts when the editor has focus """ From 0affcf371824ec8ee6fb7db80fd458c575037465 Mon Sep 17 00:00:00 2001 From: Alessandro Pasotti Date: Thu, 14 Mar 2024 18:22:52 +0100 Subject: [PATCH 20/39] Browser: fix slow behavior with network drives on windows This fixes UNC paths not identified as network paths. By ensuring that a trailing slash for directories is passed down to the identify function. --- src/core/browser/qgsdirectoryitem.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/core/browser/qgsdirectoryitem.cpp b/src/core/browser/qgsdirectoryitem.cpp index 746c4d462aca..b5ed0b8deecd 100644 --- a/src/core/browser/qgsdirectoryitem.cpp +++ b/src/core/browser/qgsdirectoryitem.cpp @@ -452,7 +452,9 @@ bool QgsDirectoryItem::pathShouldByMonitoredByDefault( const QString &path ) // else if we know that the path is on a slow device, we don't monitor by default // as this can be very expensive and slow down QGIS - if ( QgsFileUtils::pathIsSlowDevice( path ) ) + // Add trailing slash or windows API functions like GetDriveTypeW won't identify + // UNC network drives correctly + if ( QgsFileUtils::pathIsSlowDevice( path.endsWith( '/' ) ? path : path + '/' ) ) return false; // paths are monitored by default if no explicit setting is in place, and the user hasn't From c56593881bbd2733337550fcaba1253ee84e8873 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Germ=C3=A1n=20Carrillo?= Date: Mon, 11 Mar 2024 12:11:12 +0100 Subject: [PATCH 21/39] [postgres] Avoid setting nextval() clause if no default sequence was found (fix #54058) --- src/providers/postgres/qgspostgresprovider.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/providers/postgres/qgspostgresprovider.cpp b/src/providers/postgres/qgspostgresprovider.cpp index 6b4a4ecf3d43..a4fef1a00565 100644 --- a/src/providers/postgres/qgspostgresprovider.cpp +++ b/src/providers/postgres/qgspostgresprovider.cpp @@ -1374,7 +1374,7 @@ bool QgsPostgresProvider::loadFields() .arg( quotedValue( mQuery ) ) .arg( quotedValue( fieldName ) ); QgsPostgresResult seqResult( connectionRO()->PQexec( seqSql ) ); - if ( seqResult.PQntuples() == 1 ) + if ( seqResult.PQntuples() == 1 && !QgsVariantUtils::isNull( seqResult.PQgetvalue( 0, 0 ) ) ) { defValMap[tableoid][attnum] = QStringLiteral( "nextval(%1)" ).arg( quotedValue( seqResult.PQgetvalue( 0, 0 ) ) ); } From 15d9da942302edac16e96c161525fc6571a444b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Germ=C3=A1n=20Carrillo?= Date: Mon, 11 Mar 2024 12:34:52 +0100 Subject: [PATCH 22/39] [tests] Add tests for fix #54058 --- tests/src/python/test_provider_postgres.py | 34 ++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/src/python/test_provider_postgres.py b/tests/src/python/test_provider_postgres.py index 6747ab578ce9..428f17c7c086 100644 --- a/tests/src/python/test_provider_postgres.py +++ b/tests/src/python/test_provider_postgres.py @@ -2708,6 +2708,40 @@ def testDefaultValuesAndClauses(self): self.assertEqual(feature.attribute(4), 123) self.assertEqual(feature.attribute(5), 'My default') + def testNoDefaultValueClauseForPKWithNoDefaultValue(self): + """Test issue GH #54058""" + + self.execSQLCommand( + 'ALTER TABLE IF EXISTS qgis_test."gh_54058" DROP CONSTRAINT IF EXISTS pk_gh_54058;') + self.execSQLCommand( + 'DROP TABLE IF EXISTS qgis_test."gh_54058" CASCADE;') + self.execSQLCommand( + 'CREATE TABLE qgis_test."gh_54058" ( "T_Id" integer NOT NULL, name text );') + self.execSQLCommand( + 'ALTER TABLE qgis_test."gh_54058" ADD CONSTRAINT pk_gh_54058 PRIMARY KEY ("T_Id");') + + vl = QgsVectorLayer(self.dbconn + ' sslmode=disable key=\'id\' table="qgis_test"."gh_54058" () sql=', 'gh_54058', 'postgres') + self.assertTrue(vl.isValid()) + + dp = vl.dataProvider() + self.assertEqual(dp.defaultValueClause(0), '') # Not nextVal(NULL) anymore! + + def testNoDefaultValueClauseForUniqueNotNullFieldWithNoDefaultValue(self): + """Test issue GH #54058b""" + + self.execSQLCommand( + 'DROP TABLE IF EXISTS qgis_test."gh_54058b" CASCADE;') + self.execSQLCommand( + 'CREATE TABLE qgis_test."gh_54058b" ( "T_Id" integer NOT NULL, name varchar(8) UNIQUE, codigo integer NOT NULL UNIQUE);') + + vl = QgsVectorLayer(self.dbconn + ' sslmode=disable key=\'id\' table="qgis_test"."gh_54058b" () sql=', 'gh_54058b', 'postgres') + self.assertTrue(vl.isValid()) + + dp = vl.dataProvider() + self.assertEqual(dp.defaultValueClause(0), '') # The issue didn't occur here + self.assertEqual(dp.defaultValueClause(1), '') # The issue didn't occur here + self.assertEqual(dp.defaultValueClause(2), '') # Not nextVal(NULL) anymore! + def testEncodeDecodeUri(self): """Test PG encode/decode URI""" From 6832983dc5a8c3a87fe044792ba181063cb504e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=BCrgen=20Fischer?= Date: Mon, 11 Mar 2024 21:11:58 +0000 Subject: [PATCH 23/39] suggest PQgetisnull --- src/providers/postgres/qgspostgresprovider.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/providers/postgres/qgspostgresprovider.cpp b/src/providers/postgres/qgspostgresprovider.cpp index a4fef1a00565..e90da5bb68fc 100644 --- a/src/providers/postgres/qgspostgresprovider.cpp +++ b/src/providers/postgres/qgspostgresprovider.cpp @@ -1374,7 +1374,7 @@ bool QgsPostgresProvider::loadFields() .arg( quotedValue( mQuery ) ) .arg( quotedValue( fieldName ) ); QgsPostgresResult seqResult( connectionRO()->PQexec( seqSql ) ); - if ( seqResult.PQntuples() == 1 && !QgsVariantUtils::isNull( seqResult.PQgetvalue( 0, 0 ) ) ) + if ( seqResult.PQntuples() == 1 && !seqResult.PQgetisnull( 0, 0 ) ) { defValMap[tableoid][attnum] = QStringLiteral( "nextval(%1)" ).arg( quotedValue( seqResult.PQgetvalue( 0, 0 ) ) ); } From d67a11e5a33095e1ce57ecd63aa722f31016c6c4 Mon Sep 17 00:00:00 2001 From: Mathieu Pellerin Date: Fri, 15 Mar 2024 14:19:43 +0700 Subject: [PATCH 24/39] [plugins manager] Normalize _all_ version references --- python/pyplugin_installer/installer_data.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/python/pyplugin_installer/installer_data.py b/python/pyplugin_installer/installer_data.py index a8af38edda86..e4be813f430e 100644 --- a/python/pyplugin_installer/installer_data.py +++ b/python/pyplugin_installer/installer_data.py @@ -386,8 +386,8 @@ def xmlDownloaded(self): "plugin_id": plugin_id, "name": pluginNodes.item(i).toElement().attribute("name"), "version_available": version, - "version_available_stable": version if not experimental else "", - "version_available_experimental": version if experimental else "", + "version_available_stable": normalizeVersion(version) if not experimental else "", + "version_available_experimental": normalizeVersion(version) if experimental else "", "description": pluginNodes.item(i).firstChildElement("description").text().strip(), "about": pluginNodes.item(i).firstChildElement("about").text().strip(), "author_name": pluginNodes.item(i).firstChildElement("author_name").text().strip(), From 136eabf086a1818697560c72860e24cff6cbd195 Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Wed, 6 Mar 2024 13:45:34 +1000 Subject: [PATCH 25/39] Add a bunch of debugging output to diagnose GPS connection errors --- src/app/gps/qgsappgpsconnection.cpp | 7 +++- src/core/gps/qgsgpsdetector.cpp | 52 ++++++++++++++++++++++++++--- src/core/gps/qgsgpsdetector.h | 4 +++ src/core/gps/qgsnmeaconnection.cpp | 6 ++++ 4 files changed, 64 insertions(+), 5 deletions(-) diff --git a/src/app/gps/qgsappgpsconnection.cpp b/src/app/gps/qgsappgpsconnection.cpp index db5b692f355d..f88b5b83fef0 100644 --- a/src/app/gps/qgsappgpsconnection.cpp +++ b/src/app/gps/qgsappgpsconnection.cpp @@ -24,7 +24,7 @@ #include "qgsmessagebaritem.h" #include "qgssettingsentryimpl.h" #include "qgssettingsentryenumflag.h" - +#include "qgsmessagelog.h" QgsAppGpsConnection::QgsAppGpsConnection( QObject *parent ) : QObject( parent ) @@ -154,6 +154,8 @@ void QgsAppGpsConnection::connectGps() QgisApp::instance()->statusBarIface()->clearMessage(); showStatusBarMessage( tr( "Connecting to GPS device %1…" ).arg( port ) ); + QgsMessageLog::logMessage( QObject::tr( "Firing up GPS detector" ), QObject::tr( "GPS" ), Qgis::Info ); + QgsGpsDetector *detector = new QgsGpsDetector( port ); connect( detector, static_cast < void ( QgsGpsDetector::* )( QgsGpsConnection * ) > ( &QgsGpsDetector::detected ), this, &QgsAppGpsConnection::onConnected ); connect( detector, &QgsGpsDetector::detectionFailed, this, &QgsAppGpsConnection::onTimeOut ); @@ -179,6 +181,7 @@ void QgsAppGpsConnection::disconnectGps() void QgsAppGpsConnection::onTimeOut() { + QgsMessageLog::logMessage( QObject::tr( "GPS detector reported timeout" ), QObject::tr( "GPS" ), Qgis::Critical ); disconnectGps(); emit connectionTimedOut(); @@ -188,6 +191,8 @@ void QgsAppGpsConnection::onTimeOut() void QgsAppGpsConnection::onConnected( QgsGpsConnection *conn ) { + QgsMessageLog::logMessage( QObject::tr( "GPS detector GOT a connection" ), QObject::tr( "GPS" ), Qgis::Info ); + mConnection = conn; connect( mConnection, &QgsGpsConnection::stateChanged, this, &QgsAppGpsConnection::stateChanged ); connect( mConnection, &QgsGpsConnection::nmeaSentenceReceived, this, &QgsAppGpsConnection::nmeaSentenceReceived ); diff --git a/src/core/gps/qgsgpsdetector.cpp b/src/core/gps/qgsgpsdetector.cpp index b78de32e686c..5f9b594675bc 100644 --- a/src/core/gps/qgsgpsdetector.cpp +++ b/src/core/gps/qgsgpsdetector.cpp @@ -21,7 +21,7 @@ #include "qgsgpsdconnection.h" #include "qgssettingstree.h" #include "qgssettingsentryenumflag.h" - +#include "qgsmessagelog.h" #if defined(QT_POSITIONING_LIB) #include "qgsqtlocationconnection.h" @@ -72,19 +72,30 @@ QgsGpsDetector::QgsGpsDetector( const QString &portName ) if ( portName.isEmpty() ) { + QgsMessageLog::logMessage( QObject::tr( "Attempting to autodetect GPS connection" ), QObject::tr( "GPS" ), Qgis::Info ); mPortList = availablePorts(); } else { + QgsMessageLog::logMessage( QObject::tr( "Attempting GPS connection for %1" ).arg( portName ), QObject::tr( "GPS" ), Qgis::Info ); mPortList << QPair( portName, portName ); } } -QgsGpsDetector::~QgsGpsDetector() = default; +QgsGpsDetector::~QgsGpsDetector() +{ + QgsMessageLog::logMessage( QObject::tr( "Destroying GPS detector" ), QObject::tr( "GPS" ), Qgis::Info ); +} void QgsGpsDetector::advance() { - mConn.reset(); + if ( mConn ) + { + disconnect( mConn.get(), &QObject::destroyed, this, &QgsGpsDetector::connDestroyed ); + mConn.reset(); + } + + QgsMessageLog::logMessage( QObject::tr( "Trying..." ), QObject::tr( "GPS" ), Qgis::Info ); while ( !mConn ) { @@ -97,11 +108,14 @@ void QgsGpsDetector::advance() if ( mPortIndex == mPortList.size() ) { + QgsMessageLog::logMessage( QObject::tr( "No more devices to try!" ), QObject::tr( "GPS" ), Qgis::Critical ); emit detectionFailed(); deleteLater(); return; } + QgsMessageLog::logMessage( QObject::tr( "Attempting connection to device %1 @ %2" ).arg( mPortIndex ).arg( mBaudIndex ), QObject::tr( "GPS" ), Qgis::Info ); + if ( mPortList.at( mPortIndex ).first.contains( ':' ) ) { mBaudIndex = mBaudList.size() - 1; @@ -109,14 +123,17 @@ void QgsGpsDetector::advance() QStringList gpsParams = mPortList.at( mPortIndex ).first.split( ':' ); Q_ASSERT( gpsParams.size() >= 3 ); + QgsMessageLog::logMessage( QObject::tr( "Connecting to GPSD device %1" ).arg( gpsParams.join( ',' ) ), QObject::tr( "GPS" ), Qgis::Info ); mConn = std::make_unique< QgsGpsdConnection >( gpsParams[0], gpsParams[1].toShort(), gpsParams[2] ); } else if ( mPortList.at( mPortIndex ).first.contains( QLatin1String( "internalGPS" ) ) ) { #if defined(QT_POSITIONING_LIB) + QgsMessageLog::logMessage( QObject::tr( "Connecting to QtLocation service device" ), QObject::tr( "GPS" ), Qgis::Info ); mConn = std::make_unique< QgsQtLocationConnection >(); #else + QgsMessageLog::logMessage( QObject::tr( "QT_POSITIONING_LIB not found and mPortList matches internalGPS, this should never happen" ), QObject::tr( "GPS" ), Qgis::Critical ); qWarning( "QT_POSITIONING_LIB not found and mPortList matches internalGPS, this should never happen" ); #endif } @@ -132,45 +149,72 @@ void QgsGpsDetector::advance() serial->setDataBits( QgsGpsDetector::settingsGpsDataBits->value() ); serial->setStopBits( QgsGpsDetector::settingsGpsStopBits->value() ); + QgsMessageLog::logMessage( QObject::tr( "Connecting to GPSD device %1 (@ %2)" ).arg( mPortList.at( mPortIndex ).first ).arg( mBaudList[ mBaudIndex ] ), QObject::tr( "GPS" ), Qgis::Info ); + if ( serial->open( QIODevice::ReadOnly ) ) { + QgsMessageLog::logMessage( QObject::tr( "Successfully opened, have a port connection ready" ), QObject::tr( "GPS" ), Qgis::Info ); mConn = std::make_unique< QgsNmeaConnection >( serial.release() ); } + else + { + QgsMessageLog::logMessage( QObject::tr( "Serial port could NOT be opened" ), QObject::tr( "GPS" ), Qgis::Critical ); + } #else + QgsMessageLog::logMessage( QObject::tr( "QT5SERIALPORT not found and mPortList matches serial port, this should never happen" ), QObject::tr( "GPS" ), Qgis::Critical ); qWarning( "QT5SERIALPORT not found and mPortList matches serial port, this should never happen" ); #endif } } + QgsMessageLog::logMessage( QObject::tr( "Have a connection, now listening for messages" ), QObject::tr( "GPS" ), Qgis::Info ); + connect( mConn.get(), &QgsGpsConnection::stateChanged, this, static_cast < void ( QgsGpsDetector::* )( const QgsGpsInformation & ) >( &QgsGpsDetector::detected ) ); connect( mConn.get(), &QObject::destroyed, this, &QgsGpsDetector::connDestroyed ); // leave 2s to pickup a valid string - QTimer::singleShot( 2000, this, &QgsGpsDetector::advance ); + QTimer::singleShot( 2000, this, &QgsGpsDetector::connectionTimeout ); } void QgsGpsDetector::detected( const QgsGpsInformation &info ) { Q_UNUSED( info ) + QgsMessageLog::logMessage( QObject::tr( "Detected information" ), QObject::tr( "GPS" ), Qgis::Info ); + if ( !mConn ) { // advance if connection was destroyed + + QgsMessageLog::logMessage( QObject::tr( "Got information, but CONNECTION WAS DESTROYED EXTERNALLY!" ), QObject::tr( "GPS" ), Qgis::Critical ); advance(); } else if ( mConn->status() == QgsGpsConnection::GPSDataReceived ) { // signal detected + QgsMessageLog::logMessage( QObject::tr( "Connection status IS GPSDataReceived" ), QObject::tr( "GPS" ), Qgis::Info ); // let's hope there's a single, unique connection to this signal... otherwise... boom emit detected( mConn.release() ); deleteLater(); } + else + { + QgsMessageLog::logMessage( QObject::tr( "Connection status is NOT GPSDataReceived. It is %1" ).arg( mConn->status() ), QObject::tr( "GPS" ), Qgis::Critical ); + } +} + +void QgsGpsDetector::connectionTimeout() +{ + QgsMessageLog::logMessage( QObject::tr( "No data received within max listening time" ), QObject::tr( "GPS" ), Qgis::Info ); + advance(); } void QgsGpsDetector::connDestroyed( QObject *obj ) { + QgsMessageLog::logMessage( QObject::tr( "CONNECTION WAS DESTROYED EXTERNALLY!" ), QObject::tr( "GPS" ), Qgis::Critical ); + // WTF? This whole class needs re-writing... if ( obj == mConn.get() ) { diff --git a/src/core/gps/qgsgpsdetector.h b/src/core/gps/qgsgpsdetector.h index 90712a301334..266fb0fdf54c 100644 --- a/src/core/gps/qgsgpsdetector.h +++ b/src/core/gps/qgsgpsdetector.h @@ -78,6 +78,10 @@ class CORE_EXPORT QgsGpsDetector : public QObject void detectionFailed(); + private slots: + + void connectionTimeout(); + private: int mPortIndex = 0; int mBaudIndex = -1; diff --git a/src/core/gps/qgsnmeaconnection.cpp b/src/core/gps/qgsnmeaconnection.cpp index 69e742977bdf..17ba243a811c 100644 --- a/src/core/gps/qgsnmeaconnection.cpp +++ b/src/core/gps/qgsnmeaconnection.cpp @@ -17,6 +17,7 @@ #include "qgsnmeaconnection.h" #include "qgslogger.h" +#include "qgsmessagelog.h" #include #include @@ -60,15 +61,20 @@ void QgsNmeaConnection::parseData() if ( numBytes >= 6 ) { + QgsMessageLog::logMessage( QObject::tr( "Got %1 NMEA bytes" ).arg( numBytes ), QObject::tr( "GPS" ), Qgis::Info ); + QgsMessageLog::logMessage( QObject::tr( "Current NMEA device status is %1" ).arg( mStatus ), QObject::tr( "GPS" ), Qgis::Info ); if ( mStatus != GPSDataReceived ) { + QgsMessageLog::logMessage( QObject::tr( "Setting device status to DataReceived" ), QObject::tr( "GPS" ), Qgis::Info ); mStatus = DataReceived; } //append new data to the remaining results from last parseData() call mStringBuffer.append( mSource->read( numBytes ) ); processStringBuffer(); + QgsMessageLog::logMessage( QObject::tr( "Processed buffer" ), QObject::tr( "GPS" ), Qgis::Info ); + QgsMessageLog::logMessage( QObject::tr( "New status is %1" ).arg( mStatus ), QObject::tr( "GPS" ), Qgis::Info ); if ( mStatus == GPSDataReceived ) { emit stateChanged( mLastGPSInformation ); From 0a5065ab63ecfb68a65c36c00fdec6630fda5c30 Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Mon, 11 Mar 2024 11:40:01 +1000 Subject: [PATCH 26/39] Rework QgsGpsDetector to make it memory safe This is messy, as there's no way we can possibly make the current, stable API of this class safe. We have to resort to an opt-in "safe" mode which exposes a non-dangerous API. This should hopefully fix issues where the qt event loop causes destruction of the detected connection before listener slot is called and is able to take ownership of the signal argument... --- .../auto_generated/gps/qgsgpsdetector.sip.in | 48 +++++++++++++++++- .../auto_generated/gps/qgsgpsdetector.sip.in | 48 +++++++++++++++++- src/app/gps/qgsappgpsconnection.cpp | 25 +++++++--- src/app/gps/qgsappgpsconnection.h | 5 +- src/core/gps/qgsgpsdetector.cpp | 38 ++++++++++---- src/core/gps/qgsgpsdetector.h | 49 +++++++++++++++++-- 6 files changed, 188 insertions(+), 25 deletions(-) diff --git a/python/PyQt6/core/auto_generated/gps/qgsgpsdetector.sip.in b/python/PyQt6/core/auto_generated/gps/qgsgpsdetector.sip.in index 50011ec27f82..3f4627084ba1 100644 --- a/python/PyQt6/core/auto_generated/gps/qgsgpsdetector.sip.in +++ b/python/PyQt6/core/auto_generated/gps/qgsgpsdetector.sip.in @@ -23,11 +23,41 @@ Class to detect the GPS port #include "qgsgpsdetector.h" %End public: - QgsGpsDetector( const QString &portName ); + + + QgsGpsDetector( const QString &portName = QString(), bool useUnsafeSignals = true ); +%Docstring +Constructor for QgsGpsDetector. + +If ``portName`` is specified, then only devices from the given port will be scanned. Otherwise +all connection types will be attempted (including internal GPS devices). + +Since QGIS 3.38, the ``useUnsafeSignals`` parameter can be set to ``False`` to avoid emitting the +dangerous and fragile :py:func:`~QgsGpsDetector.detected` signal. This is highly recommended, but is opt-in to avoid +breaking stable QGIS 3.x API. If ``useUnsafeSignals`` is set to ``False``, only the safe :py:func:`~QgsGpsDetector.connectionDetected` signal +will be emitted and clients must manually take ownership of the detected connection via a call +to :py:func:`~QgsGpsDetector.takeConnection`. +%End ~QgsGpsDetector(); + QgsGpsConnection *takeConnection() /TransferBack/; +%Docstring +Returns the detected GPS connection, and removes it from the detector. + +The caller takes ownership of the connection. Only the first call to this +method following a :py:func:`~QgsGpsDetector.connectionDetected` signal will be able to retrieve the +detected connection -- subsequent calls will return ``None``. + +.. warning:: + + Do not call this method if the useUnsafeSignals option in the + QgsGpsDetector constructor was set to ``True``. + +.. versionadded:: 3.38 +%End + static QList< QPair > availablePorts(); public slots: @@ -37,14 +67,28 @@ Class to detect the GPS port signals: + void connectionDetected(); +%Docstring +Emitted when a GPS connection is successfully detected. + +Call :py:func:`~QgsGpsDetector.takeConnection` to take ownership of the detected connection. - void detected( QgsGpsConnection *connection ); +.. versionadded:: 3.38 +%End + + void detected( QgsGpsConnection *connection ) /Deprecated/; %Docstring Emitted when the GPS connection has been detected. A single connection must listen for this signal and immediately take ownership of the ``connection`` object. + +.. deprecated:: + This signal is dangerous and extremely unsafe! It is recommended to instead set the ``useUnsafeSignals`` parameter to ``False`` in the QgsGpsDetector constructor and use the safe :py:func:`~QgsGpsDetector.connectionDetected` signal instead. %End void detectionFailed(); +%Docstring +Emitted when the detector could not find a valid GPS connection. +%End }; diff --git a/python/core/auto_generated/gps/qgsgpsdetector.sip.in b/python/core/auto_generated/gps/qgsgpsdetector.sip.in index 50011ec27f82..3f4627084ba1 100644 --- a/python/core/auto_generated/gps/qgsgpsdetector.sip.in +++ b/python/core/auto_generated/gps/qgsgpsdetector.sip.in @@ -23,11 +23,41 @@ Class to detect the GPS port #include "qgsgpsdetector.h" %End public: - QgsGpsDetector( const QString &portName ); + + + QgsGpsDetector( const QString &portName = QString(), bool useUnsafeSignals = true ); +%Docstring +Constructor for QgsGpsDetector. + +If ``portName`` is specified, then only devices from the given port will be scanned. Otherwise +all connection types will be attempted (including internal GPS devices). + +Since QGIS 3.38, the ``useUnsafeSignals`` parameter can be set to ``False`` to avoid emitting the +dangerous and fragile :py:func:`~QgsGpsDetector.detected` signal. This is highly recommended, but is opt-in to avoid +breaking stable QGIS 3.x API. If ``useUnsafeSignals`` is set to ``False``, only the safe :py:func:`~QgsGpsDetector.connectionDetected` signal +will be emitted and clients must manually take ownership of the detected connection via a call +to :py:func:`~QgsGpsDetector.takeConnection`. +%End ~QgsGpsDetector(); + QgsGpsConnection *takeConnection() /TransferBack/; +%Docstring +Returns the detected GPS connection, and removes it from the detector. + +The caller takes ownership of the connection. Only the first call to this +method following a :py:func:`~QgsGpsDetector.connectionDetected` signal will be able to retrieve the +detected connection -- subsequent calls will return ``None``. + +.. warning:: + + Do not call this method if the useUnsafeSignals option in the + QgsGpsDetector constructor was set to ``True``. + +.. versionadded:: 3.38 +%End + static QList< QPair > availablePorts(); public slots: @@ -37,14 +67,28 @@ Class to detect the GPS port signals: + void connectionDetected(); +%Docstring +Emitted when a GPS connection is successfully detected. + +Call :py:func:`~QgsGpsDetector.takeConnection` to take ownership of the detected connection. - void detected( QgsGpsConnection *connection ); +.. versionadded:: 3.38 +%End + + void detected( QgsGpsConnection *connection ) /Deprecated/; %Docstring Emitted when the GPS connection has been detected. A single connection must listen for this signal and immediately take ownership of the ``connection`` object. + +.. deprecated:: + This signal is dangerous and extremely unsafe! It is recommended to instead set the ``useUnsafeSignals`` parameter to ``False`` in the QgsGpsDetector constructor and use the safe :py:func:`~QgsGpsDetector.connectionDetected` signal instead. %End void detectionFailed(); +%Docstring +Emitted when the detector could not find a valid GPS connection. +%End }; diff --git a/src/app/gps/qgsappgpsconnection.cpp b/src/app/gps/qgsappgpsconnection.cpp index f88b5b83fef0..ae8986dcd7ab 100644 --- a/src/app/gps/qgsappgpsconnection.cpp +++ b/src/app/gps/qgsappgpsconnection.cpp @@ -57,7 +57,7 @@ void QgsAppGpsConnection::setConnection( QgsGpsConnection *connection ) disconnectGps(); } - onConnected( connection ); + setConnectionPrivate( connection ); } QgsPoint QgsAppGpsConnection::lastValidLocation() const @@ -156,10 +156,11 @@ void QgsAppGpsConnection::connectGps() QgsMessageLog::logMessage( QObject::tr( "Firing up GPS detector" ), QObject::tr( "GPS" ), Qgis::Info ); - QgsGpsDetector *detector = new QgsGpsDetector( port ); - connect( detector, static_cast < void ( QgsGpsDetector::* )( QgsGpsConnection * ) > ( &QgsGpsDetector::detected ), this, &QgsAppGpsConnection::onConnected ); - connect( detector, &QgsGpsDetector::detectionFailed, this, &QgsAppGpsConnection::onTimeOut ); - detector->advance(); // start the detection process + // note -- QgsGpsDetector internally uses deleteLater to clean itself up! + mDetector = new QgsGpsDetector( port, false ); + connect( mDetector, &QgsGpsDetector::connectionDetected, this, &QgsAppGpsConnection::onConnectionDetected ); + connect( mDetector, &QgsGpsDetector::detectionFailed, this, &QgsAppGpsConnection::onTimeOut ); + mDetector->advance(); // start the detection process } void QgsAppGpsConnection::disconnectGps() @@ -181,6 +182,9 @@ void QgsAppGpsConnection::disconnectGps() void QgsAppGpsConnection::onTimeOut() { + if ( sender() != mDetector ) + return; + QgsMessageLog::logMessage( QObject::tr( "GPS detector reported timeout" ), QObject::tr( "GPS" ), Qgis::Critical ); disconnectGps(); emit connectionTimedOut(); @@ -189,11 +193,18 @@ void QgsAppGpsConnection::onTimeOut() showGpsConnectFailureWarning( tr( "TIMEOUT - Failed to connect to GPS device." ) ); } -void QgsAppGpsConnection::onConnected( QgsGpsConnection *conn ) +void QgsAppGpsConnection::onConnectionDetected() { + if ( sender() != mDetector ) + return; + QgsMessageLog::logMessage( QObject::tr( "GPS detector GOT a connection" ), QObject::tr( "GPS" ), Qgis::Info ); + setConnectionPrivate( mDetector->takeConnection() ); +} - mConnection = conn; +void QgsAppGpsConnection::setConnectionPrivate( QgsGpsConnection *connection ) +{ + mConnection = connection; connect( mConnection, &QgsGpsConnection::stateChanged, this, &QgsAppGpsConnection::stateChanged ); connect( mConnection, &QgsGpsConnection::nmeaSentenceReceived, this, &QgsAppGpsConnection::nmeaSentenceReceived ); connect( mConnection, &QgsGpsConnection::fixStatusChanged, this, &QgsAppGpsConnection::fixStatusChanged ); diff --git a/src/app/gps/qgsappgpsconnection.h b/src/app/gps/qgsappgpsconnection.h index 9c82af42853f..7b992c8cfeb2 100644 --- a/src/app/gps/qgsappgpsconnection.h +++ b/src/app/gps/qgsappgpsconnection.h @@ -25,6 +25,7 @@ class QgsGpsConnection; class QgsGpsInformation; class QgsPoint; class QgsMessageBarItem; +class QgsGpsDetector; /** * Manages a single "canonical" GPS connection for use in the QGIS app, eg for displaying GPS @@ -140,8 +141,9 @@ class APP_EXPORT QgsAppGpsConnection : public QObject private slots: void onTimeOut(); + void onConnectionDetected(); - void onConnected( QgsGpsConnection *conn ); + void setConnectionPrivate( QgsGpsConnection *connection ); private: @@ -150,6 +152,7 @@ class APP_EXPORT QgsAppGpsConnection : public QObject void showGpsConnectFailureWarning( const QString &message ); void showMessage( Qgis::MessageLevel level, const QString &message ); + QPointer< QgsGpsDetector > mDetector; QgsGpsConnection *mConnection = nullptr; QPointer< QgsMessageBarItem > mConnectionMessageItem; }; diff --git a/src/core/gps/qgsgpsdetector.cpp b/src/core/gps/qgsgpsdetector.cpp index 5f9b594675bc..2691905d4ad9 100644 --- a/src/core/gps/qgsgpsdetector.cpp +++ b/src/core/gps/qgsgpsdetector.cpp @@ -64,7 +64,8 @@ QList< QPair > QgsGpsDetector::availablePorts() return devs; } -QgsGpsDetector::QgsGpsDetector( const QString &portName ) +QgsGpsDetector::QgsGpsDetector( const QString &portName, bool useUnsafeSignals ) + : mUseUnsafeSignals( useUnsafeSignals ) { #if defined( HAVE_QTSERIALPORT ) mBaudList << QSerialPort::Baud4800 << QSerialPort::Baud9600 << QSerialPort::Baud38400 << QSerialPort::Baud57600 << QSerialPort::Baud115200; //add 57600 for SXBlueII GPS unit @@ -87,11 +88,21 @@ QgsGpsDetector::~QgsGpsDetector() QgsMessageLog::logMessage( QObject::tr( "Destroying GPS detector" ), QObject::tr( "GPS" ), Qgis::Info ); } +QgsGpsConnection *QgsGpsDetector::takeConnection() +{ + if ( mUseUnsafeSignals ) + { + QgsDebugError( QStringLiteral( "QgsGpsDetector::takeConnection() incorrectly called when useUnsafeSignals option is in effect" ) ); + return nullptr; + } + + return mConn.release(); +} + void QgsGpsDetector::advance() { if ( mConn ) { - disconnect( mConn.get(), &QObject::destroyed, this, &QgsGpsDetector::connDestroyed ); mConn.reset(); } @@ -170,22 +181,22 @@ void QgsGpsDetector::advance() QgsMessageLog::logMessage( QObject::tr( "Have a connection, now listening for messages" ), QObject::tr( "GPS" ), Qgis::Info ); connect( mConn.get(), &QgsGpsConnection::stateChanged, this, static_cast < void ( QgsGpsDetector::* )( const QgsGpsInformation & ) >( &QgsGpsDetector::detected ) ); - connect( mConn.get(), &QObject::destroyed, this, &QgsGpsDetector::connDestroyed ); + if ( mUseUnsafeSignals ) + { + connect( mConn.get(), &QObject::destroyed, this, &QgsGpsDetector::connDestroyed ); + } // leave 2s to pickup a valid string QTimer::singleShot( 2000, this, &QgsGpsDetector::connectionTimeout ); } -void QgsGpsDetector::detected( const QgsGpsInformation &info ) +void QgsGpsDetector::detected( const QgsGpsInformation & ) { - Q_UNUSED( info ) - QgsMessageLog::logMessage( QObject::tr( "Detected information" ), QObject::tr( "GPS" ), Qgis::Info ); if ( !mConn ) { // advance if connection was destroyed - QgsMessageLog::logMessage( QObject::tr( "Got information, but CONNECTION WAS DESTROYED EXTERNALLY!" ), QObject::tr( "GPS" ), Qgis::Critical ); advance(); } @@ -194,8 +205,17 @@ void QgsGpsDetector::detected( const QgsGpsInformation &info ) // signal detected QgsMessageLog::logMessage( QObject::tr( "Connection status IS GPSDataReceived" ), QObject::tr( "GPS" ), Qgis::Info ); - // let's hope there's a single, unique connection to this signal... otherwise... boom - emit detected( mConn.release() ); + if ( mUseUnsafeSignals ) + { + // let's hope there's a single, unique connection to this signal... otherwise... boom! + Q_NOWARN_DEPRECATED_PUSH + emit detected( mConn.release() ); + Q_NOWARN_DEPRECATED_POP + } + else + { + emit connectionDetected(); + } deleteLater(); } diff --git a/src/core/gps/qgsgpsdetector.h b/src/core/gps/qgsgpsdetector.h index 266fb0fdf54c..e287ccf658ce 100644 --- a/src/core/gps/qgsgpsdetector.h +++ b/src/core/gps/qgsgpsdetector.h @@ -47,7 +47,22 @@ class CORE_EXPORT QgsGpsDetector : public QObject { Q_OBJECT public: - QgsGpsDetector( const QString &portName ); + + // TODO QGIS 4.0 -- remove useUnsafeSignals option + + /** + * Constructor for QgsGpsDetector. + * + * If \a portName is specified, then only devices from the given port will be scanned. Otherwise + * all connection types will be attempted (including internal GPS devices). + * + * Since QGIS 3.38, the \a useUnsafeSignals parameter can be set to FALSE to avoid emitting the + * dangerous and fragile detected() signal. This is highly recommended, but is opt-in to avoid + * breaking stable QGIS 3.x API. If \a useUnsafeSignals is set to FALSE, only the safe connectionDetected() signal + * will be emitted and clients must manually take ownership of the detected connection via a call + * to takeConnection(). + */ + QgsGpsDetector( const QString &portName = QString(), bool useUnsafeSignals = true ); #if defined( HAVE_QTSERIALPORT ) static const QgsSettingsEntryEnumFlag *settingsGpsStopBits SIP_SKIP; @@ -58,6 +73,20 @@ class CORE_EXPORT QgsGpsDetector : public QObject ~QgsGpsDetector() override; + /** + * Returns the detected GPS connection, and removes it from the detector. + * + * The caller takes ownership of the connection. Only the first call to this + * method following a connectionDetected() signal will be able to retrieve the + * detected connection -- subsequent calls will return NULLPTR. + * + * \warning Do not call this method if the useUnsafeSignals option in the + * QgsGpsDetector constructor was set to TRUE. + * + * \since QGIS 3.38 + */ + QgsGpsConnection *takeConnection() SIP_TRANSFERBACK; + static QList< QPair > availablePorts(); public slots: @@ -67,15 +96,26 @@ class CORE_EXPORT QgsGpsDetector : public QObject signals: - // TODO QGIS 4.0 - this is horrible, fragile, leaky and crash prone API. - // don't transfer ownership with this signal, and add an explicit takeConnection member! + /** + * Emitted when a GPS connection is successfully detected. + * + * Call takeConnection() to take ownership of the detected connection. + * + * \since QGIS 3.38 + */ + void connectionDetected(); /** * Emitted when the GPS connection has been detected. A single connection must listen for this signal and * immediately take ownership of the \a connection object. + * + * \deprecated This signal is dangerous and extremely unsafe! It is recommended to instead set the \a useUnsafeSignals parameter to FALSE in the QgsGpsDetector constructor and use the safe connectionDetected() signal instead. */ - void detected( QgsGpsConnection *connection ); + Q_DECL_DEPRECATED void detected( QgsGpsConnection *connection ) SIP_DEPRECATED; + /** + * Emitted when the detector could not find a valid GPS connection. + */ void detectionFailed(); private slots: @@ -83,6 +123,7 @@ class CORE_EXPORT QgsGpsDetector : public QObject void connectionTimeout(); private: + bool mUseUnsafeSignals = true; int mPortIndex = 0; int mBaudIndex = -1; QList< QPair< QString, QString > > mPortList; From 991d03d9e836b4e7bdefdf2d973ff9942aa6071c Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Tue, 12 Mar 2024 09:23:38 +1000 Subject: [PATCH 27/39] Cleanup GPS detector timeout timer handling Use a single timer instead of the QTimer static method, so that we can stop the timeout as soon as we receive valid data and don't risk a race where unwanted timeouts are added to the event loop while we're processing a valid detected connection --- src/core/gps/qgsgpsdetector.cpp | 13 +++++++++++-- src/core/gps/qgsgpsdetector.h | 2 ++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/src/core/gps/qgsgpsdetector.cpp b/src/core/gps/qgsgpsdetector.cpp index 2691905d4ad9..1e19b64ee1ac 100644 --- a/src/core/gps/qgsgpsdetector.cpp +++ b/src/core/gps/qgsgpsdetector.cpp @@ -81,6 +81,10 @@ QgsGpsDetector::QgsGpsDetector( const QString &portName, bool useUnsafeSignals ) QgsMessageLog::logMessage( QObject::tr( "Attempting GPS connection for %1" ).arg( portName ), QObject::tr( "GPS" ), Qgis::Info ); mPortList << QPair( portName, portName ); } + + mTimeoutTimer = new QTimer( this ); + mTimeoutTimer->setSingleShot( true ); + connect( mTimeoutTimer, &QTimer::timeout, this, &QgsGpsDetector::connectionTimeout ); } QgsGpsDetector::~QgsGpsDetector() @@ -180,14 +184,14 @@ void QgsGpsDetector::advance() QgsMessageLog::logMessage( QObject::tr( "Have a connection, now listening for messages" ), QObject::tr( "GPS" ), Qgis::Info ); - connect( mConn.get(), &QgsGpsConnection::stateChanged, this, static_cast < void ( QgsGpsDetector::* )( const QgsGpsInformation & ) >( &QgsGpsDetector::detected ) ); + connect( mConn.get(), &QgsGpsConnection::stateChanged, this, qOverload< const QgsGpsInformation & >( &QgsGpsDetector::detected ) ); if ( mUseUnsafeSignals ) { connect( mConn.get(), &QObject::destroyed, this, &QgsGpsDetector::connDestroyed ); } // leave 2s to pickup a valid string - QTimer::singleShot( 2000, this, &QgsGpsDetector::connectionTimeout ); + mTimeoutTimer->start( 2000 ); } void QgsGpsDetector::detected( const QgsGpsInformation & ) @@ -196,12 +200,16 @@ void QgsGpsDetector::detected( const QgsGpsInformation & ) if ( !mConn ) { + mTimeoutTimer->stop(); + // advance if connection was destroyed QgsMessageLog::logMessage( QObject::tr( "Got information, but CONNECTION WAS DESTROYED EXTERNALLY!" ), QObject::tr( "GPS" ), Qgis::Critical ); advance(); } else if ( mConn->status() == QgsGpsConnection::GPSDataReceived ) { + mTimeoutTimer->stop(); + // signal detected QgsMessageLog::logMessage( QObject::tr( "Connection status IS GPSDataReceived" ), QObject::tr( "GPS" ), Qgis::Info ); @@ -221,6 +229,7 @@ void QgsGpsDetector::detected( const QgsGpsInformation & ) } else { + // don't stop timeout, we keep waiting to see if later we get the desired connection status... QgsMessageLog::logMessage( QObject::tr( "Connection status is NOT GPSDataReceived. It is %1" ).arg( mConn->status() ), QObject::tr( "GPS" ), Qgis::Critical ); } } diff --git a/src/core/gps/qgsgpsdetector.h b/src/core/gps/qgsgpsdetector.h index e287ccf658ce..59c9971ba6e1 100644 --- a/src/core/gps/qgsgpsdetector.h +++ b/src/core/gps/qgsgpsdetector.h @@ -38,6 +38,7 @@ class QgsSettingsEntryEnumFlag; class QgsGpsConnection; class QgsGpsInformation; +class QTimer; /** * \ingroup core @@ -130,6 +131,7 @@ class CORE_EXPORT QgsGpsDetector : public QObject QList mBaudList; std::unique_ptr< QgsGpsConnection > mConn; + QTimer *mTimeoutTimer = nullptr; }; #endif // QGSGPSDETECTOR_H From 9abe8277ad2a60064da18465fab14e9db6ed7904 Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Tue, 12 Mar 2024 09:25:13 +1000 Subject: [PATCH 28/39] As soon as we detect a valid device, stop listening for device status Otherwise we end up calling QgsGpsDetector::detected multiple times for the same device, and if an external client has taken the detected connection in the meantime then the detector will incorrectly move to the next device and continue trying to make connections --- src/core/gps/qgsgpsdetector.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/core/gps/qgsgpsdetector.cpp b/src/core/gps/qgsgpsdetector.cpp index 1e19b64ee1ac..2e328c3fc718 100644 --- a/src/core/gps/qgsgpsdetector.cpp +++ b/src/core/gps/qgsgpsdetector.cpp @@ -209,6 +209,9 @@ void QgsGpsDetector::detected( const QgsGpsInformation & ) else if ( mConn->status() == QgsGpsConnection::GPSDataReceived ) { mTimeoutTimer->stop(); + // stop listening for state changed signals, we've already validated this connection and don't want subsequent calls + // to QgsGpsDetector::detected being made + disconnect( mConn.get(), &QgsGpsConnection::stateChanged, this, qOverload< const QgsGpsInformation & >( &QgsGpsDetector::detected ) ); // signal detected QgsMessageLog::logMessage( QObject::tr( "Connection status IS GPSDataReceived" ), QObject::tr( "GPS" ), Qgis::Info ); From bcb4691906aad54acaa57501881c7f39e8397a9e Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Tue, 12 Mar 2024 09:30:23 +1000 Subject: [PATCH 29/39] Before releasing the connection, disconnect everything from the detector This ensures that there's NO change of any remaining unwanted interaction between the GPS detector and the connection which it no longer owns after a call to takeConnection. Otherwise when the detector outlives the "find a connection" logic we risk status updates from the taken GPS device from triggering unwanted detect-a-valid-connection logic from the detector. --- src/core/gps/qgsgpsdetector.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/core/gps/qgsgpsdetector.cpp b/src/core/gps/qgsgpsdetector.cpp index 2e328c3fc718..fdb0367cd6c1 100644 --- a/src/core/gps/qgsgpsdetector.cpp +++ b/src/core/gps/qgsgpsdetector.cpp @@ -100,6 +100,13 @@ QgsGpsConnection *QgsGpsDetector::takeConnection() return nullptr; } + if ( mConn ) + { + // this is NOT the detectors connection anymore, so disconnect all signals from the connection + // to the detector so that there's no unwanted interaction with the detector + mConn->disconnect( this ); + } + return mConn.release(); } From 88ea592d4a6e226b066b2b79e05161ee23f71128 Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Tue, 12 Mar 2024 09:32:12 +1000 Subject: [PATCH 30/39] Add a few more logging outputs --- src/core/gps/qgsgpsdetector.cpp | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/core/gps/qgsgpsdetector.cpp b/src/core/gps/qgsgpsdetector.cpp index fdb0367cd6c1..382064188af5 100644 --- a/src/core/gps/qgsgpsdetector.cpp +++ b/src/core/gps/qgsgpsdetector.cpp @@ -107,6 +107,11 @@ QgsGpsConnection *QgsGpsDetector::takeConnection() mConn->disconnect( this ); } + if ( mConn ) + QgsMessageLog::logMessage( QObject::tr( "Detected GPS connection is being taken by caller" ), QObject::tr( "GPS" ), Qgis::Info ); + else + QgsMessageLog::logMessage( QObject::tr( "Something is trying to take the GPS connection, but it doesn't exist!" ), QObject::tr( "GPS" ), Qgis::Critical ); + return mConn.release(); } @@ -114,6 +119,7 @@ void QgsGpsDetector::advance() { if ( mConn ) { + QgsMessageLog::logMessage( QObject::tr( "Destroying existing connection to attempt next configuration combination" ), QObject::tr( "GPS" ), Qgis::Info ); mConn.reset(); } @@ -187,6 +193,11 @@ void QgsGpsDetector::advance() qWarning( "QT5SERIALPORT not found and mPortList matches serial port, this should never happen" ); #endif } + + if ( !mConn ) + { + QgsMessageLog::logMessage( QObject::tr( "Got to end of connection handling loop, but have no connection!" ), QObject::tr( "GPS" ), Qgis::Critical ); + } } QgsMessageLog::logMessage( QObject::tr( "Have a connection, now listening for messages" ), QObject::tr( "GPS" ), Qgis::Info ); From d3a4ce1c54da1d9700a5eba6326b8bfa0c41afdc Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Wed, 13 Mar 2024 09:28:31 +1000 Subject: [PATCH 31/39] Demote debug messages --- src/app/gps/qgsappgpsconnection.cpp | 7 ++-- src/core/gps/qgsgpsdetector.cpp | 56 ++++++++++++++++------------- src/core/gps/qgsnmeaconnection.cpp | 11 +++--- 3 files changed, 39 insertions(+), 35 deletions(-) diff --git a/src/app/gps/qgsappgpsconnection.cpp b/src/app/gps/qgsappgpsconnection.cpp index ae8986dcd7ab..cc94879c574f 100644 --- a/src/app/gps/qgsappgpsconnection.cpp +++ b/src/app/gps/qgsappgpsconnection.cpp @@ -24,7 +24,6 @@ #include "qgsmessagebaritem.h" #include "qgssettingsentryimpl.h" #include "qgssettingsentryenumflag.h" -#include "qgsmessagelog.h" QgsAppGpsConnection::QgsAppGpsConnection( QObject *parent ) : QObject( parent ) @@ -154,7 +153,7 @@ void QgsAppGpsConnection::connectGps() QgisApp::instance()->statusBarIface()->clearMessage(); showStatusBarMessage( tr( "Connecting to GPS device %1…" ).arg( port ) ); - QgsMessageLog::logMessage( QObject::tr( "Firing up GPS detector" ), QObject::tr( "GPS" ), Qgis::Info ); + QgsDebugMsgLevel( QStringLiteral( "Firing up GPS detector" ), 2 ); // note -- QgsGpsDetector internally uses deleteLater to clean itself up! mDetector = new QgsGpsDetector( port, false ); @@ -185,7 +184,7 @@ void QgsAppGpsConnection::onTimeOut() if ( sender() != mDetector ) return; - QgsMessageLog::logMessage( QObject::tr( "GPS detector reported timeout" ), QObject::tr( "GPS" ), Qgis::Critical ); + QgsDebugMsgLevel( QStringLiteral( "GPS detector reported timeout" ), 2 ); disconnectGps(); emit connectionTimedOut(); @@ -198,7 +197,7 @@ void QgsAppGpsConnection::onConnectionDetected() if ( sender() != mDetector ) return; - QgsMessageLog::logMessage( QObject::tr( "GPS detector GOT a connection" ), QObject::tr( "GPS" ), Qgis::Info ); + QgsDebugMsgLevel( QStringLiteral( "GPS detector GOT a connection" ), 2 ); setConnectionPrivate( mDetector->takeConnection() ); } diff --git a/src/core/gps/qgsgpsdetector.cpp b/src/core/gps/qgsgpsdetector.cpp index 382064188af5..3317ff88b8c1 100644 --- a/src/core/gps/qgsgpsdetector.cpp +++ b/src/core/gps/qgsgpsdetector.cpp @@ -21,7 +21,7 @@ #include "qgsgpsdconnection.h" #include "qgssettingstree.h" #include "qgssettingsentryenumflag.h" -#include "qgsmessagelog.h" +#include "qgslogger.h" #if defined(QT_POSITIONING_LIB) #include "qgsqtlocationconnection.h" @@ -73,12 +73,12 @@ QgsGpsDetector::QgsGpsDetector( const QString &portName, bool useUnsafeSignals ) if ( portName.isEmpty() ) { - QgsMessageLog::logMessage( QObject::tr( "Attempting to autodetect GPS connection" ), QObject::tr( "GPS" ), Qgis::Info ); + QgsDebugMsgLevel( QStringLiteral( "Attempting to autodetect GPS connection" ), 2 ); mPortList = availablePorts(); } else { - QgsMessageLog::logMessage( QObject::tr( "Attempting GPS connection for %1" ).arg( portName ), QObject::tr( "GPS" ), Qgis::Info ); + QgsDebugMsgLevel( QStringLiteral( "Attempting GPS connection for %1" ).arg( portName ), 2 ); mPortList << QPair( portName, portName ); } @@ -89,7 +89,7 @@ QgsGpsDetector::QgsGpsDetector( const QString &portName, bool useUnsafeSignals ) QgsGpsDetector::~QgsGpsDetector() { - QgsMessageLog::logMessage( QObject::tr( "Destroying GPS detector" ), QObject::tr( "GPS" ), Qgis::Info ); + QgsDebugMsgLevel( QStringLiteral( "Destroying GPS detector" ), 2 ); } QgsGpsConnection *QgsGpsDetector::takeConnection() @@ -107,10 +107,16 @@ QgsGpsConnection *QgsGpsDetector::takeConnection() mConn->disconnect( this ); } +#ifdef QGISDEBUG if ( mConn ) - QgsMessageLog::logMessage( QObject::tr( "Detected GPS connection is being taken by caller" ), QObject::tr( "GPS" ), Qgis::Info ); + { + QgsDebugMsgLevel( QStringLiteral( "Detected GPS connection is being taken by caller" ), 2 ); + } else - QgsMessageLog::logMessage( QObject::tr( "Something is trying to take the GPS connection, but it doesn't exist!" ), QObject::tr( "GPS" ), Qgis::Critical ); + { + QgsDebugError( QStringLiteral( "Something is trying to take the GPS connection, but it doesn't exist!" ) ); + } +#endif return mConn.release(); } @@ -119,11 +125,11 @@ void QgsGpsDetector::advance() { if ( mConn ) { - QgsMessageLog::logMessage( QObject::tr( "Destroying existing connection to attempt next configuration combination" ), QObject::tr( "GPS" ), Qgis::Info ); + QgsDebugMsgLevel( QStringLiteral( "Destroying existing connection to attempt next configuration combination" ), 2 ); mConn.reset(); } - QgsMessageLog::logMessage( QObject::tr( "Trying..." ), QObject::tr( "GPS" ), Qgis::Info ); + QgsDebugMsgLevel( QStringLiteral( "Trying to find a connection..." ), 2 ); while ( !mConn ) { @@ -136,13 +142,13 @@ void QgsGpsDetector::advance() if ( mPortIndex == mPortList.size() ) { - QgsMessageLog::logMessage( QObject::tr( "No more devices to try!" ), QObject::tr( "GPS" ), Qgis::Critical ); + QgsDebugError( QStringLiteral( "No more devices to try!" ) ); emit detectionFailed(); deleteLater(); return; } - QgsMessageLog::logMessage( QObject::tr( "Attempting connection to device %1 @ %2" ).arg( mPortIndex ).arg( mBaudIndex ), QObject::tr( "GPS" ), Qgis::Info ); + QgsDebugMsgLevel( QStringLiteral( "Attempting connection to device %1 @ %2" ).arg( mPortIndex ).arg( mBaudIndex ), 2 ); if ( mPortList.at( mPortIndex ).first.contains( ':' ) ) { @@ -151,17 +157,17 @@ void QgsGpsDetector::advance() QStringList gpsParams = mPortList.at( mPortIndex ).first.split( ':' ); Q_ASSERT( gpsParams.size() >= 3 ); - QgsMessageLog::logMessage( QObject::tr( "Connecting to GPSD device %1" ).arg( gpsParams.join( ',' ) ), QObject::tr( "GPS" ), Qgis::Info ); + QgsDebugMsgLevel( QStringLiteral( "Connecting to GPSD device %1" ).arg( gpsParams.join( ',' ) ), 2 ); mConn = std::make_unique< QgsGpsdConnection >( gpsParams[0], gpsParams[1].toShort(), gpsParams[2] ); } else if ( mPortList.at( mPortIndex ).first.contains( QLatin1String( "internalGPS" ) ) ) { #if defined(QT_POSITIONING_LIB) - QgsMessageLog::logMessage( QObject::tr( "Connecting to QtLocation service device" ), QObject::tr( "GPS" ), Qgis::Info ); + QgsDebugMsgLevel( QStringLiteral( "Connecting to QtLocation service device" ), 2 ); mConn = std::make_unique< QgsQtLocationConnection >(); #else - QgsMessageLog::logMessage( QObject::tr( "QT_POSITIONING_LIB not found and mPortList matches internalGPS, this should never happen" ), QObject::tr( "GPS" ), Qgis::Critical ); + QgsDebugError( QStringLiteral( "QT_POSITIONING_LIB not found and mPortList matches internalGPS, this should never happen" ) ); qWarning( "QT_POSITIONING_LIB not found and mPortList matches internalGPS, this should never happen" ); #endif } @@ -177,30 +183,30 @@ void QgsGpsDetector::advance() serial->setDataBits( QgsGpsDetector::settingsGpsDataBits->value() ); serial->setStopBits( QgsGpsDetector::settingsGpsStopBits->value() ); - QgsMessageLog::logMessage( QObject::tr( "Connecting to GPSD device %1 (@ %2)" ).arg( mPortList.at( mPortIndex ).first ).arg( mBaudList[ mBaudIndex ] ), QObject::tr( "GPS" ), Qgis::Info ); + QgsDebugMsgLevel( QStringLiteral( "Connecting to serial GPS device %1 (@ %2)" ).arg( mPortList.at( mPortIndex ).first ).arg( mBaudList[ mBaudIndex ] ), 2 ); if ( serial->open( QIODevice::ReadOnly ) ) { - QgsMessageLog::logMessage( QObject::tr( "Successfully opened, have a port connection ready" ), QObject::tr( "GPS" ), Qgis::Info ); + QgsDebugMsgLevel( QStringLiteral( "Successfully opened, have a port connection ready" ), 2 ); mConn = std::make_unique< QgsNmeaConnection >( serial.release() ); } else { - QgsMessageLog::logMessage( QObject::tr( "Serial port could NOT be opened" ), QObject::tr( "GPS" ), Qgis::Critical ); + QgsDebugError( QStringLiteral( "Serial port could NOT be opened" ) ); } #else - QgsMessageLog::logMessage( QObject::tr( "QT5SERIALPORT not found and mPortList matches serial port, this should never happen" ), QObject::tr( "GPS" ), Qgis::Critical ); + QgsDebugError( QStringLiteral( "QT5SERIALPORT not found and mPortList matches serial port, this should never happen" ) ); qWarning( "QT5SERIALPORT not found and mPortList matches serial port, this should never happen" ); #endif } if ( !mConn ) { - QgsMessageLog::logMessage( QObject::tr( "Got to end of connection handling loop, but have no connection!" ), QObject::tr( "GPS" ), Qgis::Critical ); + QgsDebugError( QStringLiteral( "Got to end of connection handling loop, but have no connection!" ) ); } } - QgsMessageLog::logMessage( QObject::tr( "Have a connection, now listening for messages" ), QObject::tr( "GPS" ), Qgis::Info ); + QgsDebugMsgLevel( QStringLiteral( "Have a connection, now listening for messages" ), 2 ); connect( mConn.get(), &QgsGpsConnection::stateChanged, this, qOverload< const QgsGpsInformation & >( &QgsGpsDetector::detected ) ); if ( mUseUnsafeSignals ) @@ -214,14 +220,14 @@ void QgsGpsDetector::advance() void QgsGpsDetector::detected( const QgsGpsInformation & ) { - QgsMessageLog::logMessage( QObject::tr( "Detected information" ), QObject::tr( "GPS" ), Qgis::Info ); + QgsDebugMsgLevel( QStringLiteral( "Detected information" ), 2 ); if ( !mConn ) { mTimeoutTimer->stop(); // advance if connection was destroyed - QgsMessageLog::logMessage( QObject::tr( "Got information, but CONNECTION WAS DESTROYED EXTERNALLY!" ), QObject::tr( "GPS" ), Qgis::Critical ); + QgsDebugError( QStringLiteral( "Got information, but CONNECTION WAS DESTROYED EXTERNALLY!" ) ); advance(); } else if ( mConn->status() == QgsGpsConnection::GPSDataReceived ) @@ -232,7 +238,7 @@ void QgsGpsDetector::detected( const QgsGpsInformation & ) disconnect( mConn.get(), &QgsGpsConnection::stateChanged, this, qOverload< const QgsGpsInformation & >( &QgsGpsDetector::detected ) ); // signal detected - QgsMessageLog::logMessage( QObject::tr( "Connection status IS GPSDataReceived" ), QObject::tr( "GPS" ), Qgis::Info ); + QgsDebugMsgLevel( QStringLiteral( "Connection status IS GPSDataReceived" ), 2 ); if ( mUseUnsafeSignals ) { @@ -251,19 +257,19 @@ void QgsGpsDetector::detected( const QgsGpsInformation & ) else { // don't stop timeout, we keep waiting to see if later we get the desired connection status... - QgsMessageLog::logMessage( QObject::tr( "Connection status is NOT GPSDataReceived. It is %1" ).arg( mConn->status() ), QObject::tr( "GPS" ), Qgis::Critical ); + QgsDebugMsgLevel( QStringLiteral( "Connection status is NOT GPSDataReceived. It is %1" ).arg( mConn->status() ), 2 ); } } void QgsGpsDetector::connectionTimeout() { - QgsMessageLog::logMessage( QObject::tr( "No data received within max listening time" ), QObject::tr( "GPS" ), Qgis::Info ); + QgsDebugMsgLevel( QStringLiteral( "No data received within max listening time" ), 2 ); advance(); } void QgsGpsDetector::connDestroyed( QObject *obj ) { - QgsMessageLog::logMessage( QObject::tr( "CONNECTION WAS DESTROYED EXTERNALLY!" ), QObject::tr( "GPS" ), Qgis::Critical ); + QgsDebugError( QStringLiteral( "CONNECTION WAS DESTROYED EXTERNALLY!" ) ); // WTF? This whole class needs re-writing... if ( obj == mConn.get() ) diff --git a/src/core/gps/qgsnmeaconnection.cpp b/src/core/gps/qgsnmeaconnection.cpp index 17ba243a811c..72dc8fc7cb39 100644 --- a/src/core/gps/qgsnmeaconnection.cpp +++ b/src/core/gps/qgsnmeaconnection.cpp @@ -17,7 +17,6 @@ #include "qgsnmeaconnection.h" #include "qgslogger.h" -#include "qgsmessagelog.h" #include #include @@ -61,20 +60,20 @@ void QgsNmeaConnection::parseData() if ( numBytes >= 6 ) { - QgsMessageLog::logMessage( QObject::tr( "Got %1 NMEA bytes" ).arg( numBytes ), QObject::tr( "GPS" ), Qgis::Info ); - QgsMessageLog::logMessage( QObject::tr( "Current NMEA device status is %1" ).arg( mStatus ), QObject::tr( "GPS" ), Qgis::Info ); + QgsDebugMsgLevel( QStringLiteral( "Got %1 NMEA bytes" ).arg( numBytes ), 3 ); + QgsDebugMsgLevel( QStringLiteral( "Current NMEA device status is %1" ).arg( mStatus ), 3 ); if ( mStatus != GPSDataReceived ) { - QgsMessageLog::logMessage( QObject::tr( "Setting device status to DataReceived" ), QObject::tr( "GPS" ), Qgis::Info ); + QgsDebugMsgLevel( QStringLiteral( "Setting device status to DataReceived" ), 3 ); mStatus = DataReceived; } //append new data to the remaining results from last parseData() call mStringBuffer.append( mSource->read( numBytes ) ); processStringBuffer(); - QgsMessageLog::logMessage( QObject::tr( "Processed buffer" ), QObject::tr( "GPS" ), Qgis::Info ); + QgsDebugMsgLevel( QStringLiteral( "Processed buffer" ), 3 ); - QgsMessageLog::logMessage( QObject::tr( "New status is %1" ).arg( mStatus ), QObject::tr( "GPS" ), Qgis::Info ); + QgsDebugMsgLevel( QStringLiteral( "New status is %1" ).arg( mStatus ), 3 ); if ( mStatus == GPSDataReceived ) { emit stateChanged( mLastGPSInformation ); From 99b2a2a7d82de81242b8af73b7e5710ff8adf382 Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Fri, 8 Mar 2024 10:39:16 +1000 Subject: [PATCH 32/39] [processing] Use correct ellipsoid for network analysis tools Use the processing context's ellipsoid instead of a hardcoded WGS84 ellipsoid for distance calculations during network analysis, so that the lengths used will exactly match other measurement tools used on the same features in the same project. --- .../tests/testdata/qgis_algorithm_tests2.yaml | 12 ++++++++++++ .../processing/qgsalgorithmnetworkanalysisbase.cpp | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/python/plugins/processing/tests/testdata/qgis_algorithm_tests2.yaml b/python/plugins/processing/tests/testdata/qgis_algorithm_tests2.yaml index f43816b277bc..d98d5b180628 100644 --- a/python/plugins/processing/tests/testdata/qgis_algorithm_tests2.yaml +++ b/python/plugins/processing/tests/testdata/qgis_algorithm_tests2.yaml @@ -1808,6 +1808,7 @@ tests: - algorithm: native:shortestpathpointtopoint name: Shortest path (point to point, shortest route) + ellipsoid: WGS84 params: DEFAULT_DIRECTION: 2 DEFAULT_SPEED: 5.0 @@ -1838,6 +1839,7 @@ tests: - algorithm: native:shortestpathpointtopoint name: Shortest path (point to point, fastest route) + ellipsoid: WGS84 params: DEFAULT_DIRECTION: 2 DEFAULT_SPEED: 5.0 @@ -1869,6 +1871,7 @@ tests: - algorithm: native:shortestpathlayertopoint name: Shortest path layer to point + ellipsoid: WGS84 params: DEFAULT_DIRECTION: 2 DEFAULT_SPEED: 5.0 @@ -1899,6 +1902,7 @@ tests: - algorithm: native:shortestpathpointtolayer name: Shortest path point to layer + ellipsoid: WGS84 params: DEFAULT_DIRECTION: 2 DEFAULT_SPEED: 5.0 @@ -1929,6 +1933,7 @@ tests: - algorithm: native:serviceareafrompoint name: Service area from point (shortest, nodes) + ellipsoid: WGS84 params: DEFAULT_DIRECTION: 2 DEFAULT_SPEED: 50.0 @@ -1960,6 +1965,7 @@ tests: - algorithm: native:serviceareafrompoint name: Service area from point (shortest, nodes, bounds) + ellipsoid: WGS84 params: DEFAULT_DIRECTION: 2 DEFAULT_SPEED: 50.0 @@ -1986,6 +1992,7 @@ tests: - algorithm: native:serviceareafrompoint name: Service area from point (shortest, lines) + ellipsoid: WGS84 params: DEFAULT_DIRECTION: 2 DEFAULT_SPEED: 50.0 @@ -2017,6 +2024,7 @@ tests: - algorithm: native:serviceareafrompoint name: Service area from point (fastest, old parameter) + ellipsoid: WGS84 params: DEFAULT_DIRECTION: 2 DEFAULT_SPEED: 50.0 @@ -2051,6 +2059,7 @@ tests: - algorithm: native:serviceareafrompoint name: Service area from point (fastest, new parameter) + ellipsoid: WGS84 params: DEFAULT_DIRECTION: 2 DEFAULT_SPEED: 50.0 @@ -2085,6 +2094,7 @@ tests: - algorithm: native:serviceareafromlayer name: Service area from layer (shortest, nodes) + ellipsoid: WGS84 params: DEFAULT_DIRECTION: 2 DEFAULT_SPEED: 50.0 @@ -2120,6 +2130,7 @@ tests: - algorithm: native:serviceareafromlayer name: Service area from layer (shortest, nodes, boundary) + ellipsoid: WGS84 params: DEFAULT_DIRECTION: 2 DEFAULT_SPEED: 50.0 @@ -2147,6 +2158,7 @@ tests: - algorithm: native:serviceareafromlayer name: Service area from layer (shortest, lines) + ellipsoid: WGS84 params: DEFAULT_DIRECTION: 2 DEFAULT_SPEED: 50.0 diff --git a/src/analysis/processing/qgsalgorithmnetworkanalysisbase.cpp b/src/analysis/processing/qgsalgorithmnetworkanalysisbase.cpp index 1aac17581aa0..d273671fc8c0 100644 --- a/src/analysis/processing/qgsalgorithmnetworkanalysisbase.cpp +++ b/src/analysis/processing/qgsalgorithmnetworkanalysisbase.cpp @@ -133,7 +133,7 @@ void QgsNetworkAnalysisAlgorithmBase::loadCommonParams( const QVariantMap ¶m mDirector->addStrategy( new QgsNetworkDistanceStrategy() ); } - mBuilder = std::make_unique< QgsGraphBuilder >( mNetwork->sourceCrs(), true, tolerance ); + mBuilder = std::make_unique< QgsGraphBuilder >( mNetwork->sourceCrs(), true, tolerance, context.ellipsoid() ); } void QgsNetworkAnalysisAlgorithmBase::loadPoints( QgsFeatureSource *source, QVector< QgsPointXY > &points, QHash< int, QgsAttributes > &attributes, QgsProcessingContext &context, QgsProcessingFeedback *feedback ) From e37f52f703192a896f515165da8fdcf6b1d7965f Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Thu, 14 Mar 2024 09:10:10 +1000 Subject: [PATCH 33/39] Fix test --- python/plugins/processing/tests/AlgorithmsTestBase.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/python/plugins/processing/tests/AlgorithmsTestBase.py b/python/plugins/processing/tests/AlgorithmsTestBase.py index d192abe6889b..4a97db068a68 100644 --- a/python/plugins/processing/tests/AlgorithmsTestBase.py +++ b/python/plugins/processing/tests/AlgorithmsTestBase.py @@ -168,6 +168,12 @@ def check_algorithm(self, name, defs): # ignore user setting for invalid geometry handling context = QgsProcessingContext() context.setProject(QgsProject.instance()) + if 'ellipsoid' in defs: + # depending on the project settings, we can't always rely + # on QgsProject.ellipsoid() returning the same ellipsoid as was + # specified in the test definition. So just force ensure that the + # context's ellipsoid is the desired one + context.setEllipsoid(defs['ellipsoid']) if 'skipInvalid' in defs and defs['skipInvalid']: context.setInvalidGeometryCheck(QgsFeatureRequest.GeometrySkipInvalid) From dd7cb7fceed34c4b699bf1f7b523925f9f28f6b0 Mon Sep 17 00:00:00 2001 From: Jean Felder Date: Thu, 2 Nov 2023 12:30:29 +0100 Subject: [PATCH 34/39] qgswfsgetfeature: Do not invert axis if no SRSNAME is passed A WFS request such as `SERVICE=WFS&REQUEST=GetFeature&VERSION=1.1.0&SRSNAME=EPSG:4326` does not invert the axis and return the coordinates in the LON/LAT order. For example: 2.358 48.865 2.37 48.876 However, the same request without a SRSNAME parameter inverts the axis and returns the the coordinates in the LAT/LON order: 48.865 2.358 48.876 2.37 With this change, the axis is not inverted if the SRSNAME parameter is not passed. This way, this is the same behavior as `SRSNAME=EPSG:4326`. This is a follow-up of https://github.com/qgis/QGIS/pull/45270. --- src/server/services/wfs/qgswfsgetfeature.cpp | 14 ++++++++++++-- ..._getFeature_1_1_0_featureid_0_1_1_0_srsname.txt | 10 +++++----- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/src/server/services/wfs/qgswfsgetfeature.cpp b/src/server/services/wfs/qgswfsgetfeature.cpp index e994491b9e86..4785a43065f1 100644 --- a/src/server/services/wfs/qgswfsgetfeature.cpp +++ b/src/server/services/wfs/qgswfsgetfeature.cpp @@ -461,7 +461,16 @@ namespace QgsWfs { // For WFS 1.1 we honor requested CRS and axis order - const QString srsName {request.serverParameters().value( QStringLiteral( "SRSNAME" ) )}; + // if the CRS is defined in the parameters, use it + // otherwise: + // - geojson uses 'EPSG:4326' by default + // - other formats use the default CRS (DefaultSRS, which is the layer's CRS) + const QString requestSrsName = request.serverParameters().value( QStringLiteral( "SRSNAME" ) ); + const QString srsName + { + !requestSrsName.isEmpty() ? requestSrsName : + ( aRequest.outputFormat == QgsWfsParameters::Format::GeoJSON ? QStringLiteral( "EPSG:4326" ) : outputCrs.authid() ) + }; const bool invertAxis { mWfsParameters.versionAsNumber() >= QgsProjectVersion( 1, 1, 0 ) && outputCrs.hasAxisInverted() && ! srsName.startsWith( QLatin1String( "EPSG:" ) ) }; @@ -1248,7 +1257,8 @@ namespace QgsWfs if ( format == QgsWfsParameters::Format::GML3 ) { // For WFS 1.1 we honor requested CRS and axis order - const QString srsName {request.serverParameters().value( QStringLiteral( "SRSNAME" ) )}; + const QString requestSrsName = request.serverParameters().value( QStringLiteral( "SRSNAME" ) ); + const QString srsName = !requestSrsName.isEmpty() ? requestSrsName : crs.authid(); const bool invertAxis { mWfsParameters.versionAsNumber() >= QgsProjectVersion( 1, 1, 0 ) && crs.hasAxisInverted() && ! srsName.startsWith( QLatin1String( "EPSG:" ) ) }; diff --git a/tests/testdata/qgis_server/wfs_getFeature_1_1_0_featureid_0_1_1_0_srsname.txt b/tests/testdata/qgis_server/wfs_getFeature_1_1_0_featureid_0_1_1_0_srsname.txt index ce39f88833d0..c5c1206856e6 100644 --- a/tests/testdata/qgis_server/wfs_getFeature_1_1_0_featureid_0_1_1_0_srsname.txt +++ b/tests/testdata/qgis_server/wfs_getFeature_1_1_0_featureid_0_1_1_0_srsname.txt @@ -3,21 +3,21 @@ Content-Type: text/xml; subtype=gml/3.1.1; charset=utf-8 - 44.90139484 8.20345931 - 44.90148253 8.20354699 + 8.20345931 44.90139484 + 8.20354699 44.90148253 - 44.90148253 8.20349634 - 44.90148253 8.20349634 + 8.20349634 44.90148253 + 8.20349634 44.90148253 - 44.90148253 8.20349634 + 8.20349634 44.90148253 1 From f98bcdc5b1375e0ee484c84b2e86d6b268b326f0 Mon Sep 17 00:00:00 2001 From: Jean Felder Date: Mon, 13 Nov 2023 15:23:43 +0100 Subject: [PATCH 35/39] qgswfsgetfeature: Add a comment to explain axis inversion logic --- src/server/services/wfs/qgswfsgetfeature.cpp | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/server/services/wfs/qgswfsgetfeature.cpp b/src/server/services/wfs/qgswfsgetfeature.cpp index 4785a43065f1..d1cec9340a5b 100644 --- a/src/server/services/wfs/qgswfsgetfeature.cpp +++ b/src/server/services/wfs/qgswfsgetfeature.cpp @@ -461,7 +461,11 @@ namespace QgsWfs { // For WFS 1.1 we honor requested CRS and axis order - // if the CRS is defined in the parameters, use it + // Axis is not inverted if srsName starts with EPSG + // It needs to be an EPSG urn, e.g. urn:ogc:def:crs:EPSG::4326 + // This follows geoserver convention + // See: https://docs.geoserver.org/stable/en/user/services/wfs/axis_order.html + // if the crs is defined in the parameters, use it // otherwise: // - geojson uses 'EPSG:4326' by default // - other formats use the default CRS (DefaultSRS, which is the layer's CRS) @@ -1257,6 +1261,10 @@ namespace QgsWfs if ( format == QgsWfsParameters::Format::GML3 ) { // For WFS 1.1 we honor requested CRS and axis order + // Axis is not inverted if srsName starts with EPSG + // It needs to be an EPSG urn, e.g. urn:ogc:def:crs:EPSG::4326 + // This follows geoserver convention + // See: https://docs.geoserver.org/stable/en/user/services/wfs/axis_order.html const QString requestSrsName = request.serverParameters().value( QStringLiteral( "SRSNAME" ) ); const QString srsName = !requestSrsName.isEmpty() ? requestSrsName : crs.authid(); const bool invertAxis { mWfsParameters.versionAsNumber() >= QgsProjectVersion( 1, 1, 0 ) && From 8481f49a57d43057508561e345f05d161567f32f Mon Sep 17 00:00:00 2001 From: Andrea Giudiceandrea Date: Wed, 20 Mar 2024 06:08:52 +0100 Subject: [PATCH 36/39] Fix QgsSymbol::drawPreviewIcon Transform the "Opacity" value calculated by Data Defined Override expression from [0-100] range to [0-1] range. --- src/core/symbology/qgssymbol.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/symbology/qgssymbol.cpp b/src/core/symbology/qgssymbol.cpp index c281f8f9db64..cd5239d537fe 100644 --- a/src/core/symbology/qgssymbol.cpp +++ b/src/core/symbology/qgssymbol.cpp @@ -942,7 +942,7 @@ void QgsSymbol::drawPreviewIcon( QPainter *painter, QSize size, QgsRenderContext const bool prevForceVector = context->forceVectorOutput(); context->setForceVectorOutput( true ); - const double opacity = expressionContext ? dataDefinedProperties().valueAsDouble( QgsSymbol::PropertyOpacity, *expressionContext, mOpacity ) : mOpacity; + const double opacity = expressionContext ? dataDefinedProperties().valueAsDouble( QgsSymbol::PropertyOpacity, *expressionContext, mOpacity * 100 ) * 0.01 : mOpacity; QgsSymbolRenderContext symbolContext( *context, Qgis::RenderUnit::Unknown, opacity, false, mRenderHints, nullptr ); symbolContext.setSelected( selected ); From 1dcf0c35c1edff441d063797064aa674a44a2657 Mon Sep 17 00:00:00 2001 From: Mathieu Pellerin Date: Mon, 18 Mar 2024 14:23:04 +0700 Subject: [PATCH 37/39] [layouts] Defer legend item feature count until layout is drawn --- src/core/layertree/qgslayertreemodel.cpp | 13 +++++++---- src/core/layout/qgslayoutitemlegend.cpp | 28 +++++++++++++++++++++--- src/core/layout/qgslayoutitemlegend.h | 8 ++++--- 3 files changed, 39 insertions(+), 10 deletions(-) diff --git a/src/core/layertree/qgslayertreemodel.cpp b/src/core/layertree/qgslayertreemodel.cpp index 6766cfd2fec7..c1824aea4b0a 100644 --- a/src/core/layertree/qgslayertreemodel.cpp +++ b/src/core/layertree/qgslayertreemodel.cpp @@ -41,7 +41,10 @@ QgsLayerTreeModel::QgsLayerTreeModel( QgsLayerTree *rootNode, QObject *parent ) , mRootNode( rootNode ) , mFlags( ShowLegend | AllowLegendChangeState | DeferredLegendInvalidation ) { - connectToRootNode(); + if ( rootNode ) + { + connectToRootNode(); + } mFontLayer.setBold( true ); @@ -1072,9 +1075,11 @@ void QgsLayerTreeModel::connectToRootNode() void QgsLayerTreeModel::disconnectFromRootNode() { - disconnect( mRootNode, nullptr, this, nullptr ); - - disconnectFromLayers( mRootNode ); + if ( mRootNode ) + { + disconnect( mRootNode, nullptr, this, nullptr ); + disconnectFromLayers( mRootNode ); + } } void QgsLayerTreeModel::recursivelyEmitDataChanged( const QModelIndex &idx ) diff --git a/src/core/layout/qgslayoutitemlegend.cpp b/src/core/layout/qgslayoutitemlegend.cpp index 3ed55d97dcb3..cd6a3dcb8cc3 100644 --- a/src/core/layout/qgslayoutitemlegend.cpp +++ b/src/core/layout/qgslayoutitemlegend.cpp @@ -46,7 +46,7 @@ QgsLayoutItemLegend::QgsLayoutItemLegend( QgsLayout *layout ) : QgsLayoutItem( layout ) - , mLegendModel( new QgsLegendModel( layout->project()->layerTreeRoot(), this ) ) + , mLegendModel( new QgsLegendModel( nullptr, this ) ) { #if 0 //no longer required? connect( &layout->atlasComposition(), &QgsAtlasComposition::renderEnded, this, &QgsLayoutItemLegend::onAtlasEnded ); @@ -105,6 +105,8 @@ void QgsLayoutItemLegend::paint( QPainter *painter, const QStyleOptionGraphicsIt if ( !painter ) return; + ensureModelIsInitialized(); + if ( mFilterAskedForUpdate ) { mFilterAskedForUpdate = false; @@ -309,11 +311,28 @@ bool QgsLayoutItemLegend::resizeToContents() const void QgsLayoutItemLegend::setCustomLayerTree( QgsLayerTree *rootGroup ) { - mLegendModel->setRootGroup( rootGroup ? rootGroup : ( mLayout ? mLayout->project()->layerTreeRoot() : nullptr ) ); + if ( !mDeferLegendModelInitialization ) + { + mLegendModel->setRootGroup( rootGroup ? rootGroup : ( mLayout ? mLayout->project()->layerTreeRoot() : nullptr ) ); + } mCustomLayerTree.reset( rootGroup ); } +void QgsLayoutItemLegend::ensureModelIsInitialized() +{ + if ( mDeferLegendModelInitialization ) + { + mDeferLegendModelInitialization = false; + setCustomLayerTree( mCustomLayerTree.release() ); + } +} + +QgsLegendModel *QgsLayoutItemLegend::model() +{ + ensureModelIsInitialized(); + return mLegendModel.get(); +} void QgsLayoutItemLegend::setAutoUpdateModel( bool autoUpdate ) { @@ -1020,7 +1039,10 @@ void QgsLayoutItemLegend::clearLegendCachedData() } }; - clearNodeCache( mLegendModel->rootGroup() ); + if ( QgsLayerTree *rootGroup = mLegendModel->rootGroup() ) + { + clearNodeCache( rootGroup ); + } } void QgsLayoutItemLegend::mapLayerStyleOverridesChanged() diff --git a/src/core/layout/qgslayoutitemlegend.h b/src/core/layout/qgslayoutitemlegend.h index 39d7410f595b..ee1b6c275a3e 100644 --- a/src/core/layout/qgslayoutitemlegend.h +++ b/src/core/layout/qgslayoutitemlegend.h @@ -23,7 +23,7 @@ #include "qgslayoutitem.h" #include "qgslayertreemodel.h" #include "qgslegendsettings.h" -#include "qgslayertreegroup.h" +#include "qgslayertree.h" #include "qgsexpressioncontext.h" class QgsLayerTreeModel; @@ -158,7 +158,7 @@ class CORE_EXPORT QgsLayoutItemLegend : public QgsLayoutItem /** * Returns the legend model. */ - QgsLegendModel *model() { return mLegendModel.get(); } + QgsLegendModel *model(); /** * Sets whether the legend content should auto update to reflect changes in the project's @@ -636,8 +636,10 @@ class CORE_EXPORT QgsLayoutItemLegend : public QgsLayoutItem void setModelStyleOverrides( const QMap &overrides ); + void ensureModelIsInitialized(); std::unique_ptr< QgsLegendModel > mLegendModel; - std::unique_ptr< QgsLayerTreeGroup > mCustomLayerTree; + std::unique_ptr< QgsLayerTree > mCustomLayerTree; + bool mDeferLegendModelInitialization = true; QgsLegendSettings mSettings; From 0238727acc61563ae61912088169cf3b20f4984b Mon Sep 17 00:00:00 2001 From: Alessandro Pasotti Date: Wed, 13 Mar 2024 10:53:22 +0100 Subject: [PATCH 38/39] Browser: avoid calling slow functions on disconnected drives This is an attempt to fix browser slowness when network drives becomes unavailable. Some QDir/QFileInfo call may become slow when called on disconneced drives on windows, by calling them in the init method (which is called by a separate thread and when the driver is more likely to be available) the risk to call them when they can become slow is drastically reduced. Whether this will help to reduce the browser slowness on network drives it is hard to tell but it won't certainly hurt. --- src/core/browser/qgsdirectoryitem.cpp | 7 +++++-- src/core/browser/qgsdirectoryitem.h | 3 +++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/core/browser/qgsdirectoryitem.cpp b/src/core/browser/qgsdirectoryitem.cpp index b5ed0b8deecd..629a38056947 100644 --- a/src/core/browser/qgsdirectoryitem.cpp +++ b/src/core/browser/qgsdirectoryitem.cpp @@ -59,6 +59,10 @@ void QgsDirectoryItem::init( const QString &dirName ) QgsSettings settings; + const QFileInfo fi { mDirPath }; + mIsDir = fi.isDir(); + mIsSymLink = fi.isSymLink(); + mMonitoring = monitoringForPath( mDirPath ); switch ( mMonitoring ) { @@ -165,8 +169,7 @@ QIcon QgsDirectoryItem::icon() return QgsDataItem::icon(); // symbolic link? use link icon - const QFileInfo fi( mDirPath ); - if ( fi.isDir() && fi.isSymLink() ) + if ( mIsDir && mIsSymLink ) { return mIconColor.isValid() ? QgsApplication::getThemeIcon( QStringLiteral( "/mIconFolderLinkParams.svg" ), mIconColor, mIconColor.darker() ) diff --git a/src/core/browser/qgsdirectoryitem.h b/src/core/browser/qgsdirectoryitem.h index 0334624e5031..3b5fe960d516 100644 --- a/src/core/browser/qgsdirectoryitem.h +++ b/src/core/browser/qgsdirectoryitem.h @@ -216,6 +216,9 @@ class CORE_EXPORT QgsDirectoryItem : public QgsDataCollectionItem QDateTime mLastScan; QColor mIconColor; + bool mIsDir = false; + bool mIsSymLink = false; + friend class TestQgsDataItem; }; From 6b8f06518d08a0833cb781f7e95cb68f5e603c3c Mon Sep 17 00:00:00 2001 From: Alessandro Pasotti Date: Sat, 9 Mar 2024 19:07:54 +0100 Subject: [PATCH 39/39] Raster - paletted: fix slow rendering with huge number of classes Fix #56652 --- src/core/raster/qgspalettedrasterrenderer.cpp | 28 +++++++++++++------ 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/src/core/raster/qgspalettedrasterrenderer.cpp b/src/core/raster/qgspalettedrasterrenderer.cpp index f1429d857ca3..e312ede9b441 100644 --- a/src/core/raster/qgspalettedrasterrenderer.cpp +++ b/src/core/raster/qgspalettedrasterrenderer.cpp @@ -42,21 +42,33 @@ QgsPalettedRasterRenderer::QgsPalettedRasterRenderer( QgsRasterInterface *input, : QgsRasterRenderer( input, QStringLiteral( "paletted" ) ) , mBand( bandNumber ) { + + QHash>> classData; + // Prepare for the worst case, where we have to store all the values for each class + classData.reserve( classes.size() ); + // This is to keep the ordering of the labels, because hash is fast but unordered + QVector labels; + labels.reserve( classes.size() ); + for ( const Class &klass : std::as_const( classes ) ) { - MultiValueClassData::iterator it = std::find_if( mMultiValueClassData.begin(), mMultiValueClassData.end(), [&klass]( const MultiValueClass & val ) -> bool - { - return val.label == klass.label && val.color == klass.color ; - } ); - if ( it != mMultiValueClassData.end() ) + if ( !classData.contains( klass.label ) ) { - it->values.push_back( klass.value ); + labels.push_back( klass.label ); } - else + classData[klass.label][klass.color].push_back( klass.value ); + } + + mMultiValueClassData.reserve( classData.size() ); + + for ( auto labelIt = labels.constBegin(); labelIt != labels.constEnd(); ++labelIt ) + { + for ( auto colorIt = classData[*labelIt].constBegin(); colorIt != classData[*labelIt].constEnd(); ++colorIt ) { - mMultiValueClassData.push_back( MultiValueClass{ { klass.value }, klass.color, klass.label } ); + mMultiValueClassData.push_back( MultiValueClass{ colorIt.value(), colorIt.key(), *labelIt } ); } } + updateArrays(); }