From 8ff0efd2301cfe6d154fd9540786f7c4296af6ba Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Thu, 27 May 2021 16:54:53 +1000 Subject: [PATCH] [browser] By default do not monitor directories on drives we know are slow Effectively this means that the browser no longer defaults to watching network and remote drives (on Windows) for changes. This is expensive to do and can result in large hangs in the QGIS application. Users can still manually opt-in to monitoring of these locations through the context menu of the directory in the browser panel. --- cmake_templates/qgsconfig.h.in | 2 + .../browser/qgsdirectoryitem.sip.in | 5 +- src/core/browser/qgsdirectoryitem.cpp | 6 +++ src/core/browser/qgsdirectoryitem.h | 5 +- src/core/qgsfileutils.cpp | 6 +++ tests/src/core/testqgsdataitem.cpp | 52 +++++++++++++++++++ 6 files changed, 72 insertions(+), 4 deletions(-) diff --git a/cmake_templates/qgsconfig.h.in b/cmake_templates/qgsconfig.h.in index 98cdc28cfeb..5fe3176b55b 100644 --- a/cmake_templates/qgsconfig.h.in +++ b/cmake_templates/qgsconfig.h.in @@ -97,5 +97,7 @@ #define PDAL_VERSION_MINOR "${PDAL_VERSION_MINOR}" #define PDAL_VERSION_MICRO "${PDAL_VERSION_MICRO}" +#cmakedefine ENABLE_TESTS + #endif diff --git a/python/core/auto_generated/browser/qgsdirectoryitem.sip.in b/python/core/auto_generated/browser/qgsdirectoryitem.sip.in index e8bead4ea30..d6b7a4d2f87 100644 --- a/python/core/auto_generated/browser/qgsdirectoryitem.sip.in +++ b/python/core/auto_generated/browser/qgsdirectoryitem.sip.in @@ -142,8 +142,9 @@ All parent directories will be checked to determine whether they have monitoring manually enabled or disabled. As soon as a parent directory is found which has monitoring manually enabled or disabled then the corresponding value will be returned. -Paths are monitored by default, so if no explicit setting is in place for a parent directory then -the function will return ``True``. +If no explicit setting is in place for a parent directory, then a check will be made to determine +whether the path resides on a known slow drive. If so, monitoring is disabled by default and +``False`` will be returned. Otherwise paths are monitored by default and the function will return ``True``. .. seealso:: :py:func:`isMonitored` diff --git a/src/core/browser/qgsdirectoryitem.cpp b/src/core/browser/qgsdirectoryitem.cpp index a7b62f96c67..d25909696b2 100644 --- a/src/core/browser/qgsdirectoryitem.cpp +++ b/src/core/browser/qgsdirectoryitem.cpp @@ -23,6 +23,7 @@ #include "qgsdataprovider.h" #include "qgszipitem.h" #include "qgsprojectitem.h" +#include "qgsfileutils.h" #include #include #include @@ -449,6 +450,11 @@ 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 ) ) + return false; + // paths are monitored by default if no explicit setting is in place return true; } diff --git a/src/core/browser/qgsdirectoryitem.h b/src/core/browser/qgsdirectoryitem.h index c019fb0a85e..22d5710e59c 100644 --- a/src/core/browser/qgsdirectoryitem.h +++ b/src/core/browser/qgsdirectoryitem.h @@ -150,8 +150,9 @@ class CORE_EXPORT QgsDirectoryItem : public QgsDataCollectionItem * manually enabled or disabled. As soon as a parent directory is found which has monitoring * manually enabled or disabled then the corresponding value will be returned. * - * Paths are monitored by default, so if no explicit setting is in place for a parent directory then - * the function will return TRUE. + * If no explicit setting is in place for a parent directory, then a check will be made to determine + * whether the path resides on a known slow drive. If so, monitoring is disabled by default and + * FALSE will be returned. Otherwise paths are monitored by default and the function will return TRUE. * * \see isMonitored() * \see setMonitoring() diff --git a/src/core/qgsfileutils.cpp b/src/core/qgsfileutils.cpp index 857f4713457..edaf412aadd 100644 --- a/src/core/qgsfileutils.cpp +++ b/src/core/qgsfileutils.cpp @@ -15,6 +15,7 @@ #include "qgsfileutils.h" #include "qgis.h" #include "qgsexception.h" +#include "qgsconfig.h" #include #include #include @@ -337,6 +338,11 @@ Qgis::DriveType QgsFileUtils::driveType( const QString &path ) bool QgsFileUtils::pathIsSlowDevice( const QString &path ) { +#ifdef ENABLE_TESTS + if ( path.contains( QLatin1String( "fake_slow_path_for_unit_tests" ) ) ) + return true; +#endif + try { const Qgis::DriveType type = driveType( path ); diff --git a/tests/src/core/testqgsdataitem.cpp b/tests/src/core/testqgsdataitem.cpp index ecc5b361aef..b104f0fa927 100644 --- a/tests/src/core/testqgsdataitem.cpp +++ b/tests/src/core/testqgsdataitem.cpp @@ -53,6 +53,7 @@ class TestQgsDataItem : public QObject void testDirItem(); void testDirItemChildren(); void testDirItemMonitoring(); + void testDirItemMonitoringSlowDrive(); void testLayerItemType(); void testProjectItemCreation(); @@ -341,6 +342,57 @@ void TestQgsDataItem::testDirItemMonitoring() QVERIFY( !childItem3->mFileSystemWatcher ); } +void TestQgsDataItem::testDirItemMonitoringSlowDrive() +{ + QTemporaryDir dir; + + // fake a directory on a slow drive -- this is a special hardcoded path which always reports to be on a slow drive. See QgsFileUtils::pathIsSlowDevice + const QString parentDir = dir.path() + QStringLiteral( "/fake_slow_path_for_unit_tests" ); + const QString child1 = parentDir + QStringLiteral( "/child1" ); + QVERIFY( QDir().mkpath( child1 ) ); + const QString child2 = parentDir + QStringLiteral( "/child2" ); + QVERIFY( QDir().mkpath( child2 ) ); + const QString child2child = child2 + QStringLiteral( "/child" ); + QVERIFY( QDir().mkpath( child2child ) ); + + QVERIFY( !QgsDirectoryItem::pathShouldByMonitoredByDefault( parentDir ) ); + QVERIFY( !QgsDirectoryItem::pathShouldByMonitoredByDefault( child1 ) ); + QVERIFY( !QgsDirectoryItem::pathShouldByMonitoredByDefault( child2 ) ); + QVERIFY( !QgsDirectoryItem::pathShouldByMonitoredByDefault( child2child ) ); + + std::unique_ptr< QgsDirectoryItem > dirItem = std::make_unique< QgsDirectoryItem >( nullptr, QStringLiteral( "parent name" ), parentDir, QStringLiteral( "/" ) + parentDir ); + // user has not explicitly set the path to be monitored or not, so Default should be returned here: + QCOMPARE( dirItem->monitoring(), Qgis::BrowserDirectoryMonitoring::Default ); + // but directory should NOT be monitored + QVERIFY( !dirItem->isMonitored() ); + + dirItem->populate( true ); + QVERIFY( !dirItem->mFileSystemWatcher ); + + QCOMPARE( dirItem->rowCount(), 2 ); + QPointer< QgsDirectoryItem > childItem1( qobject_cast< QgsDirectoryItem * >( dirItem->children().at( 0 ) ) ); + QPointer< QgsDirectoryItem > childItem2( qobject_cast< QgsDirectoryItem * >( dirItem->children().at( 1 ) ) ); + QVERIFY( childItem1 ); + QVERIFY( childItem2 ); + // neither of these should be monitored either! + QVERIFY( !childItem1->isMonitored() ); + QVERIFY( !childItem2->isMonitored() ); + childItem1->populate( true ); + childItem2->populate( true ); + QVERIFY( !childItem1->mFileSystemWatcher ); + QVERIFY( !childItem2->mFileSystemWatcher ); + + // explicitly opt in to monitoring a directory on a slow drive + childItem2->setMonitoring( Qgis::BrowserDirectoryMonitoring::AlwaysMonitor ); + QVERIFY( childItem2->mFileSystemWatcher ); + + // this means that subdirectories of childItem2 should now return true to being monitored by default + QVERIFY( !QgsDirectoryItem::pathShouldByMonitoredByDefault( parentDir ) ); + QVERIFY( !QgsDirectoryItem::pathShouldByMonitoredByDefault( child1 ) ); + QVERIFY( !QgsDirectoryItem::pathShouldByMonitoredByDefault( child2 ) ); + QVERIFY( QgsDirectoryItem::pathShouldByMonitoredByDefault( child2child ) ); +} + void TestQgsDataItem::testLayerItemType() { std::unique_ptr< QgsMapLayer > layer = std::make_unique< QgsVectorLayer >( mTestDataDir + "polys.shp",