Skip to content

Commit

Permalink
Port checkInputCRS to c++, and allow algorithms to flag when they
Browse files Browse the repository at this point in the history
require all input layers to be in the same CRS

The default behaviour is to assume that algorithms are well behaved
and can handle multi-CRS inputs, but algs have the option to
flag that they do not allow this and require the input CRS check.

Those algs should document that they require all inputs to have
matching CRS - processing 3.0 behaviour is to assume that algs
can handle this.
  • Loading branch information
nyalldawson committed Jun 11, 2017
1 parent 386c424 commit 2d2c229
Show file tree
Hide file tree
Showing 8 changed files with 160 additions and 26 deletions.
10 changes: 10 additions & 0 deletions python/core/processing/qgsprocessingalgorithm.sip
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ class QgsProcessingAlgorithm
FlagHideFromModeler,
FlagSupportsBatch,
FlagCanCancel,
FlagRequiresMatchingCrs,
FlagDeprecated,
};
typedef QFlags<QgsProcessingAlgorithm::Flag> Flags;
Expand Down Expand Up @@ -244,6 +245,15 @@ class QgsProcessingAlgorithm
:rtype: QgsExpressionContext
%End

virtual bool validateInputCrs( const QVariantMap &parameters,
QgsProcessingContext &context ) const;
%Docstring
Checks whether the coordinate reference systems for the specified set of ``parameters``
are valid for the algorithm. For instance, the base implementation performs
checks to ensure that all input CRS are equal
Returns true if ``parameters`` have passed the CRS check.
:rtype: bool
%End

protected:

Expand Down
20 changes: 0 additions & 20 deletions python/plugins/processing/core/GeoAlgorithm.py
Original file line number Diff line number Diff line change
Expand Up @@ -264,26 +264,6 @@ def setOutputCRS(self):
except:
pass

def checkInputCRS(self, context=None):
"""It checks that all input layers use the same CRS. If so,
returns True. False otherwise.
"""
if context is None:
context = dataobjects.createContext()
crsList = []
for param in self.parameterDefinitions():
if isinstance(param, (ParameterRaster, ParameterVector, ParameterMultipleInput)):
if param.value:
if isinstance(param, ParameterMultipleInput):
layers = param.value.split(';')
else:
layers = [param.value]
for item in layers:
crs = QgsProcessingUtils.mapLayerFromString(item, context).crs()
if crs not in crsList:
crsList.append(crs)
return len(crsList) < 2

def addOutput(self, output):
# TODO: check that name does not exist
if isinstance(output, Output):
Expand Down
2 changes: 1 addition & 1 deletion python/plugins/processing/core/Processing.py
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ def runAlgorithm(algOrName, onFinish, *args, **kwargs):
Processing.tr("Processing"))
return

if not alg.checkInputCRS(context):
if not alg.validateInputCrs(parameters, context):
print('Warning: Not all input layers use the same CRS.\n' +
'This can cause unexpected results.')
QgsMessageLog.logMessage(
Expand Down
3 changes: 1 addition & 2 deletions python/plugins/processing/gui/AlgorithmDialog.py
Original file line number Diff line number Diff line change
Expand Up @@ -168,8 +168,7 @@ def accept(self):

QgsMessageLog.logMessage(str(parameters), 'Processing', QgsMessageLog.CRITICAL)

# TODO
if False and checkCRS and not self.alg.checkInputCRS():
if checkCRS and not self.alg.validateInputCrs(parameters, context):
reply = QMessageBox.question(self, self.tr("Unmatching CRS's"),
self.tr('Layers do not all use the same CRS. This can '
'cause unexpected results.\nDo you want to '
Expand Down
4 changes: 2 additions & 2 deletions python/plugins/processing/script/ScriptAlgorithm.py
Original file line number Diff line number Diff line change
Expand Up @@ -125,11 +125,11 @@ def defineCharacteristicsFromScript(self):
def canExecute(self):
return not self.error, self.error

def checkInputCRS(self):
def validateInputCrs(self, parameters, context):
if self.noCRSWarning:
return True
else:
return GeoAlgorithm.checkInputCRS(self)
return QgsProcessingAlgorithm.validateInputCrs(self, parameters, context)

def createDescriptiveName(self, s):
return s.replace('_', ' ')
Expand Down
68 changes: 68 additions & 0 deletions src/core/processing/qgsprocessingalgorithm.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "qgsprocessingoutputs.h"
#include "qgsrectangle.h"
#include "qgsprocessingcontext.h"
#include "qgsprocessingutils.h"

QgsProcessingAlgorithm::~QgsProcessingAlgorithm()
{
Expand Down Expand Up @@ -117,6 +118,73 @@ QgsExpressionContext QgsProcessingAlgorithm::createExpressionContext( const QVar
return c;
}

bool QgsProcessingAlgorithm::validateInputCrs( const QVariantMap &parameters, QgsProcessingContext &context ) const
{
if ( !( flags() & FlagRequiresMatchingCrs ) )
{
// I'm a well behaved algorithm - I take work AWAY from users!
return true;
}

bool foundCrs = false;
QgsCoordinateReferenceSystem crs;
Q_FOREACH ( const QgsProcessingParameterDefinition *def, mParameters )
{
if ( def->type() == QStringLiteral( "layer" ) || def->type() == QStringLiteral( "raster" ) )
{
QgsMapLayer *layer = QgsProcessingParameters::parameterAsLayer( def, parameters, context );
if ( layer )
{
if ( foundCrs && layer->crs().isValid() && crs != layer->crs() )
{
return false;
}
else if ( !foundCrs && layer->crs().isValid() )
{
foundCrs = true;
crs = layer->crs();
}
}
}
else if ( def->type() == QStringLiteral( "source" ) )
{
QgsFeatureSource *source = QgsProcessingParameters::parameterAsSource( def, parameters, context );
if ( source )
{
if ( foundCrs && source->sourceCrs().isValid() && crs != source->sourceCrs() )
{
return false;
}
else if ( !foundCrs && source->sourceCrs().isValid() )
{
foundCrs = true;
crs = source->sourceCrs();
}
}
}
else if ( def->type() == QStringLiteral( "multilayer" ) )
{
QList< QgsMapLayer *> layers = QgsProcessingParameters::parameterAsLayerList( def, parameters, context );
Q_FOREACH ( QgsMapLayer *layer, layers )
{
if ( !layer )
continue;

if ( foundCrs && layer->crs().isValid() && crs != layer->crs() )
{
return false;
}
else if ( !foundCrs && layer->crs().isValid() )
{
foundCrs = true;
crs = layer->crs();
}
}
}
}
return true;
}

bool QgsProcessingAlgorithm::addParameter( QgsProcessingParameterDefinition *definition )
{
if ( !definition )
Expand Down
9 changes: 9 additions & 0 deletions src/core/processing/qgsprocessingalgorithm.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ class CORE_EXPORT QgsProcessingAlgorithm
FlagHideFromModeler = 1 << 2, //!< Algorithm should be hidden from the modeler
FlagSupportsBatch = 1 << 3, //!< Algorithm supports batch mode
FlagCanCancel = 1 << 4, //!< Algorithm can be canceled
FlagRequiresMatchingCrs = 1 << 5, //!< Algorithm requires that all input layers have matching coordinate reference systems
FlagDeprecated = FlagHideFromToolbox | FlagHideFromModeler, //!< Algorithm is deprecated
};
Q_DECLARE_FLAGS( Flags, Flag )
Expand Down Expand Up @@ -243,6 +244,14 @@ class CORE_EXPORT QgsProcessingAlgorithm
QgsExpressionContext createExpressionContext( const QVariantMap &parameters,
QgsProcessingContext &context ) const;

/**
* Checks whether the coordinate reference systems for the specified set of \a parameters
* are valid for the algorithm. For instance, the base implementation performs
* checks to ensure that all input CRS are equal
* Returns true if \a parameters have passed the CRS check.
*/
virtual bool validateInputCrs( const QVariantMap &parameters,
QgsProcessingContext &context ) const;

protected:

Expand Down
70 changes: 69 additions & 1 deletion tests/src/core/testqgsprocessing.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,19 @@ class DummyAlgorithm : public QgsProcessingAlgorithm
{
public:

DummyAlgorithm( const QString &name ) : mName( name ) {}
DummyAlgorithm( const QString &name ) : mName( name ) { mFlags = QgsProcessingAlgorithm::flags(); }

QString name() const override { return mName; }
QString displayName() const override { return mName; }
virtual QVariantMap processAlgorithm( const QVariantMap &,
QgsProcessingContext &, QgsProcessingFeedback * ) const override { return QVariantMap(); }

virtual Flags flags() const override { return mFlags; }

QString mName;

Flags mFlags;

void checkParameterVals()
{
addParameter( new QgsProcessingParameterString( "p1" ) );
Expand Down Expand Up @@ -126,6 +131,62 @@ class DummyAlgorithm : public QgsProcessingAlgorithm
QVERIFY( hasHtmlOutputs() );
}

void runValidateInputCrsChecks()
{
addParameter( new QgsProcessingParameterMapLayer( "p1" ) );
addParameter( new QgsProcessingParameterMapLayer( "p2" ) );
QVariantMap parameters;

QgsVectorLayer *layer3111 = new QgsVectorLayer( "Point?crs=epsg:3111", "v1", "memory" );
QgsProject p;
p.addMapLayer( layer3111 );

QString testDataDir = QStringLiteral( TEST_DATA_DIR ) + '/'; //defined in CmakeLists.txt
QString raster1 = testDataDir + "tenbytenraster.asc";
QFileInfo fi1( raster1 );
QgsRasterLayer *r1 = new QgsRasterLayer( fi1.filePath(), "R1" );
QVERIFY( r1->isValid() );
p.addMapLayer( r1 );

QgsVectorLayer *layer4326 = new QgsVectorLayer( "Point?crs=epsg:4326", "v1", "memory" );
p.addMapLayer( layer4326 );

QgsProcessingContext context;
context.setProject( &p );

// flag not set
mFlags = 0;
parameters.insert( "p1", QVariant::fromValue( layer3111 ) );
QVERIFY( validateInputCrs( parameters, context ) );
mFlags = FlagRequiresMatchingCrs;
QVERIFY( validateInputCrs( parameters, context ) );

// two layers, different crs
parameters.insert( "p2", QVariant::fromValue( layer4326 ) );
// flag not set
mFlags = 0;
QVERIFY( validateInputCrs( parameters, context ) );
mFlags = FlagRequiresMatchingCrs;
QVERIFY( !validateInputCrs( parameters, context ) );

// raster layer
parameters.remove( "p2" );
addParameter( new QgsProcessingParameterRasterLayer( "p3" ) );
parameters.insert( "p3", QVariant::fromValue( r1 ) );
QVERIFY( !validateInputCrs( parameters, context ) );

// feature source
parameters.remove( "p3" );
addParameter( new QgsProcessingParameterFeatureSource( "p4" ) );
parameters.insert( "p4", layer4326->id() );
QVERIFY( !validateInputCrs( parameters, context ) );

parameters.remove( "p4" );
addParameter( new QgsProcessingParameterMultipleLayers( "p5" ) );
parameters.insert( "p5", QVariantList() << layer4326->id() << r1->id() );
QVERIFY( !validateInputCrs( parameters, context ) );
}

};

//dummy provider for testing
Expand Down Expand Up @@ -228,6 +289,7 @@ class TestQgsProcessing: public QObject
void processingFeatureSource();
void processingFeatureSink();
void algorithmScope();
void validateInputCrs();

private:

Expand Down Expand Up @@ -2640,5 +2702,11 @@ void TestQgsProcessing::algorithmScope()
QCOMPARE( exp2.evaluate( &context ).toInt(), 5 );
}

void TestQgsProcessing::validateInputCrs()
{
DummyAlgorithm alg( "test" );
alg.runValidateInputCrsChecks();
}

QGSTEST_MAIN( TestQgsProcessing )
#include "testqgsprocessing.moc"

0 comments on commit 2d2c229

Please sign in to comment.