diff --git a/python/plugins/processing/gui/AlgorithmDialog.py b/python/plugins/processing/gui/AlgorithmDialog.py index 39b85afd679..82f8d54ce39 100644 --- a/python/plugins/processing/gui/AlgorithmDialog.py +++ b/python/plugins/processing/gui/AlgorithmDialog.py @@ -138,7 +138,7 @@ class AlgorithmDialog(QgsProcessingAlgorithmDialogBase): if reply == QMessageBox.No: return ok, msg = self.algorithm().checkParameterValues(parameters, context) - if msg: + if not ok: QMessageBox.warning( self, self.tr('Unable to execute algorithm'), msg) return diff --git a/python/plugins/processing/tests/AlgorithmsTestBase.py b/python/plugins/processing/tests/AlgorithmsTestBase.py index 4632613f3cc..a3a38eb9545 100644 --- a/python/plugins/processing/tests/AlgorithmsTestBase.py +++ b/python/plugins/processing/tests/AlgorithmsTestBase.py @@ -117,6 +117,10 @@ class AlgorithmsTest(object): feedback = QgsProcessingFeedback() + # first check that algorithm accepts the parameters we pass... + ok, msg = alg.checkParameterValues(parameters, context) + self.assertTrue(ok, 'Algorithm failed checkParameterValues with result {}'.format(msg)) + if expectFailure: try: results, ok = alg.run(parameters, context, feedback) diff --git a/src/analysis/processing/qgsalgorithmbuffer.cpp b/src/analysis/processing/qgsalgorithmbuffer.cpp index cc7f508ba59..f73d4eb6bb9 100644 --- a/src/analysis/processing/qgsalgorithmbuffer.cpp +++ b/src/analysis/processing/qgsalgorithmbuffer.cpp @@ -55,8 +55,8 @@ void QgsBufferAlgorithm::initAlgorithm( const QVariantMap & ) addParameter( bufferParam.release() ); addParameter( new QgsProcessingParameterNumber( QStringLiteral( "SEGMENTS" ), QObject::tr( "Segments" ), QgsProcessingParameterNumber::Integer, 5, false, 1 ) ); - addParameter( new QgsProcessingParameterEnum( QStringLiteral( "END_CAP_STYLE" ), QObject::tr( "End cap style" ), QStringList() << QObject::tr( "Round" ) << QObject::tr( "Flat" ) << QObject::tr( "Square" ), false ) ); - addParameter( new QgsProcessingParameterEnum( QStringLiteral( "JOIN_STYLE" ), QObject::tr( "Join style" ), QStringList() << QObject::tr( "Round" ) << QObject::tr( "Miter" ) << QObject::tr( "Bevel" ), false ) ); + addParameter( new QgsProcessingParameterEnum( QStringLiteral( "END_CAP_STYLE" ), QObject::tr( "End cap style" ), QStringList() << QObject::tr( "Round" ) << QObject::tr( "Flat" ) << QObject::tr( "Square" ), false, 0 ) ); + addParameter( new QgsProcessingParameterEnum( QStringLiteral( "JOIN_STYLE" ), QObject::tr( "Join style" ), QStringList() << QObject::tr( "Round" ) << QObject::tr( "Miter" ) << QObject::tr( "Bevel" ), false, 0 ) ); addParameter( new QgsProcessingParameterNumber( QStringLiteral( "MITER_LIMIT" ), QObject::tr( "Miter limit" ), QgsProcessingParameterNumber::Double, 2, false, 1 ) ); addParameter( new QgsProcessingParameterBoolean( QStringLiteral( "DISSOLVE" ), QObject::tr( "Dissolve result" ), false ) ); diff --git a/src/core/processing/qgsprocessingparameters.cpp b/src/core/processing/qgsprocessingparameters.cpp index f8e44e61c39..52e65662d7b 100644 --- a/src/core/processing/qgsprocessingparameters.cpp +++ b/src/core/processing/qgsprocessingparameters.cpp @@ -1230,10 +1230,11 @@ QgsProcessingParameterDefinition::QgsProcessingParameterDefinition( const QStrin bool QgsProcessingParameterDefinition::checkValueIsAcceptable( const QVariant &input, QgsProcessingContext * ) const { - if ( !input.isValid() ) + if ( !input.isValid() && !mDefault.isValid() ) return mFlags & FlagOptional; - if ( input.type() == QVariant::String && input.toString().isEmpty() ) + if ( ( input.type() == QVariant::String && input.toString().isEmpty() ) + || ( !input.isValid() && mDefault.type() == QVariant::String && mDefault.toString().isEmpty() ) ) return mFlags & FlagOptional; return true; @@ -2045,10 +2046,16 @@ QgsProcessingParameterDefinition *QgsProcessingParameterNumber::clone() const return new QgsProcessingParameterNumber( *this ); } -bool QgsProcessingParameterNumber::checkValueIsAcceptable( const QVariant &input, QgsProcessingContext * ) const +bool QgsProcessingParameterNumber::checkValueIsAcceptable( const QVariant &value, QgsProcessingContext * ) const { + QVariant input = value; if ( !input.isValid() ) - return mFlags & FlagOptional; + { + if ( !defaultValue().isValid() ) + return mFlags & FlagOptional; + + input = defaultValue(); + } if ( input.canConvert() ) { @@ -2317,10 +2324,16 @@ QgsProcessingParameterDefinition *QgsProcessingParameterEnum::clone() const return new QgsProcessingParameterEnum( *this ); } -bool QgsProcessingParameterEnum::checkValueIsAcceptable( const QVariant &input, QgsProcessingContext * ) const +bool QgsProcessingParameterEnum::checkValueIsAcceptable( const QVariant &value, QgsProcessingContext * ) const { + QVariant input = value; if ( !input.isValid() ) - return mFlags & FlagOptional; + { + if ( !defaultValue().isValid() ) + return mFlags & FlagOptional; + + input = defaultValue(); + } if ( input.canConvert() ) { diff --git a/tests/src/analysis/testqgsprocessing.cpp b/tests/src/analysis/testqgsprocessing.cpp old mode 100755 new mode 100644 index 1b34b1fa713..379e94ebfe9 --- a/tests/src/analysis/testqgsprocessing.cpp +++ b/tests/src/analysis/testqgsprocessing.cpp @@ -1983,7 +1983,7 @@ void TestQgsProcessing::parameterBoolean() QVERIFY( def->checkValueIsAcceptable( true ) ); QVERIFY( def->checkValueIsAcceptable( "false" ) ); QVERIFY( def->checkValueIsAcceptable( "true" ) ); - QVERIFY( !def->checkValueIsAcceptable( QVariant() ) ); + QVERIFY( def->checkValueIsAcceptable( QVariant() ) ); // should be acceptable, because it falls back to default value params.insert( "non_optional_default_true", false ); QCOMPARE( QgsProcessingParameters::parameterAsBool( def.get(), params, context ), false ); @@ -2006,6 +2006,14 @@ void TestQgsProcessing::parameterBoolean() QCOMPARE( fromCode->description(), QStringLiteral( "non optional default true" ) ); QCOMPARE( fromCode->flags(), def->flags() ); QCOMPARE( fromCode->defaultValue().toBool(), true ); + + def.reset( new QgsProcessingParameterBoolean( "non_optional_no_default", QString(), QVariant(), false ) ); + + QVERIFY( def->checkValueIsAcceptable( false ) ); + QVERIFY( def->checkValueIsAcceptable( true ) ); + QVERIFY( def->checkValueIsAcceptable( "false" ) ); + QVERIFY( def->checkValueIsAcceptable( "true" ) ); + QVERIFY( !def->checkValueIsAcceptable( QVariant() ) ); // should NOT be acceptable, because it falls back to invalid default value } void TestQgsProcessing::parameterCrs() @@ -3034,7 +3042,7 @@ void TestQgsProcessing::parameterDistance() QVERIFY( !def->checkValueIsAcceptable( "1.1,2" ) ); QVERIFY( !def->checkValueIsAcceptable( "layer12312312" ) ); QVERIFY( !def->checkValueIsAcceptable( "" ) ); - QVERIFY( !def->checkValueIsAcceptable( QVariant() ) ); + QVERIFY( def->checkValueIsAcceptable( QVariant() ) ); // should be acceptable, falls back to default value // string representing a number QVariantMap params; @@ -3102,6 +3110,18 @@ void TestQgsProcessing::parameterDistance() params.insert( "optional", QVariant( "aaaa" ) ); number = QgsProcessingParameters::parameterAsDouble( def.get(), params, context ); QGSCOMPARENEAR( number, 5.4, 0.001 ); + + // non-optional, invalid default + def.reset( new QgsProcessingParameterDistance( "non_optional", QString(), QVariant(), QStringLiteral( "parent" ), false ) ); + QCOMPARE( def->parentParameterName(), QStringLiteral( "parent" ) ); + def->setParentParameterName( QStringLiteral( "parent2" ) ); + QCOMPARE( def->parentParameterName(), QStringLiteral( "parent2" ) ); + QVERIFY( def->checkValueIsAcceptable( 5 ) ); + QVERIFY( def->checkValueIsAcceptable( "1.1" ) ); + QVERIFY( !def->checkValueIsAcceptable( "1.1,2" ) ); + QVERIFY( !def->checkValueIsAcceptable( "layer12312312" ) ); + QVERIFY( !def->checkValueIsAcceptable( "" ) ); + QVERIFY( !def->checkValueIsAcceptable( QVariant() ) ); // should NOT be acceptable, falls back to invalid default value } void TestQgsProcessing::parameterNumber() @@ -3115,7 +3135,7 @@ void TestQgsProcessing::parameterNumber() QVERIFY( !def->checkValueIsAcceptable( "1.1,2" ) ); QVERIFY( !def->checkValueIsAcceptable( "layer12312312" ) ); QVERIFY( !def->checkValueIsAcceptable( "" ) ); - QVERIFY( !def->checkValueIsAcceptable( QVariant() ) ); + QVERIFY( def->checkValueIsAcceptable( QVariant() ) ); // should be acceptable, falls back to default value // string representing a number QVariantMap params; @@ -3221,6 +3241,15 @@ void TestQgsProcessing::parameterNumber() QCOMPARE( fromCode->description(), QStringLiteral( "optional" ) ); QCOMPARE( fromCode->flags(), def->flags() ); QVERIFY( !fromCode->defaultValue().isValid() ); + + // non-optional, invalid default + def.reset( new QgsProcessingParameterNumber( "non_optional", QString(), QgsProcessingParameterNumber::Double, QVariant(), false ) ); + QVERIFY( def->checkValueIsAcceptable( 5 ) ); + QVERIFY( def->checkValueIsAcceptable( "1.1" ) ); + QVERIFY( !def->checkValueIsAcceptable( "1.1,2" ) ); + QVERIFY( !def->checkValueIsAcceptable( "layer12312312" ) ); + QVERIFY( !def->checkValueIsAcceptable( "" ) ); + QVERIFY( !def->checkValueIsAcceptable( QVariant() ) ); // should NOT be acceptable, falls back to invalid default value } void TestQgsProcessing::parameterRange() @@ -3458,7 +3487,7 @@ void TestQgsProcessing::parameterEnum() QVERIFY( !def->checkValueIsAcceptable( -1 ) ); QVERIFY( !def->checkValueIsAcceptable( "layer12312312" ) ); QVERIFY( !def->checkValueIsAcceptable( "" ) ); - QVERIFY( !def->checkValueIsAcceptable( QVariant() ) ); + QVERIFY( def->checkValueIsAcceptable( QVariant() ) ); // should be acceptable, because falls back to default value // string representing a number QVariantMap params; @@ -3571,7 +3600,7 @@ void TestQgsProcessing::parameterEnum() QVERIFY( !def->checkValueIsAcceptable( -1 ) ); QVERIFY( !def->checkValueIsAcceptable( "layer12312312" ) ); QVERIFY( !def->checkValueIsAcceptable( "" ) ); - QVERIFY( def->checkValueIsAcceptable( QVariant() ) ); + QVERIFY( !def->checkValueIsAcceptable( QVariant() ) ); code = def->asScriptCode(); QCOMPARE( code, QStringLiteral( "##optional=optional enum a;b 5" ) ); @@ -3628,6 +3657,23 @@ void TestQgsProcessing::parameterEnum() QCOMPARE( fromCode->defaultValue(), def->defaultValue() ); QCOMPARE( fromCode->options(), def->options() ); QCOMPARE( fromCode->allowMultiple(), def->allowMultiple() ); + + // non optional, no default + def.reset( new QgsProcessingParameterEnum( "non_optional", QString(), QStringList() << "A" << "B" << "C", false, QVariant(), false ) ); + QVERIFY( !def->checkValueIsAcceptable( false ) ); + QVERIFY( !def->checkValueIsAcceptable( true ) ); + QVERIFY( def->checkValueIsAcceptable( 1 ) ); + QVERIFY( def->checkValueIsAcceptable( "1" ) ); + QVERIFY( !def->checkValueIsAcceptable( "1,2" ) ); + QVERIFY( def->checkValueIsAcceptable( 0 ) ); + QVERIFY( !def->checkValueIsAcceptable( QVariantList() ) ); + QVERIFY( !def->checkValueIsAcceptable( QVariantList() << 1 ) ); + QVERIFY( !def->checkValueIsAcceptable( QVariantList() << "a" ) ); + QVERIFY( !def->checkValueIsAcceptable( 15 ) ); + QVERIFY( !def->checkValueIsAcceptable( -1 ) ); + QVERIFY( !def->checkValueIsAcceptable( "layer12312312" ) ); + QVERIFY( !def->checkValueIsAcceptable( "" ) ); + QVERIFY( !def->checkValueIsAcceptable( QVariant() ) ); // should NOT be acceptable, because falls back to invalid default value } void TestQgsProcessing::parameterString() @@ -3760,6 +3806,13 @@ void TestQgsProcessing::parameterString() QCOMPARE( fromCode->flags(), def->flags() ); QCOMPARE( fromCode->defaultValue(), def->defaultValue() ); QCOMPARE( fromCode->multiLine(), def->multiLine() ); + + // not optional, valid default! + def.reset( new QgsProcessingParameterString( "non_optional", QString(), QString( "def" ), false, false ) ); + QVERIFY( def->checkValueIsAcceptable( 1 ) ); + QVERIFY( def->checkValueIsAcceptable( "test" ) ); + QVERIFY( !def->checkValueIsAcceptable( "" ) ); + QVERIFY( def->checkValueIsAcceptable( QVariant() ) ); // should be valid, falls back to valid default } void TestQgsProcessing::parameterExpression() @@ -3767,11 +3820,11 @@ void TestQgsProcessing::parameterExpression() QgsProcessingContext context; // not optional! - std::unique_ptr< QgsProcessingParameterExpression > def( new QgsProcessingParameterExpression( "non_optional", QString(), QString(), QString(), false ) ); + std::unique_ptr< QgsProcessingParameterExpression > def( new QgsProcessingParameterExpression( "non_optional", QString(), QString( "1+1" ), QString(), false ) ); QVERIFY( def->checkValueIsAcceptable( 1 ) ); QVERIFY( def->checkValueIsAcceptable( "test" ) ); QVERIFY( !def->checkValueIsAcceptable( "" ) ); - QVERIFY( !def->checkValueIsAcceptable( QVariant() ) ); + QVERIFY( def->checkValueIsAcceptable( QVariant() ) ); // should be acceptable, because it will fallback to default value // string QVariantMap params; @@ -3785,7 +3838,7 @@ void TestQgsProcessing::parameterExpression() QCOMPARE( def->valueAsPythonString( QVariant::fromValue( QgsProperty::fromExpression( "\"a\"=1" ) ), context ), QStringLiteral( "QgsProperty.fromExpression('\"a\"=1')" ) ); QString code = def->asScriptCode(); - QCOMPARE( code, QStringLiteral( "##non_optional=expression" ) ); + QCOMPARE( code, QStringLiteral( "##non_optional=expression 1+1" ) ); std::unique_ptr< QgsProcessingParameterExpression > fromCode( dynamic_cast< QgsProcessingParameterExpression * >( QgsProcessingParameters::parameterFromScriptCode( code ) ) ); QVERIFY( fromCode.get() ); QCOMPARE( fromCode->name(), def->name() ); @@ -3832,6 +3885,14 @@ void TestQgsProcessing::parameterExpression() QCOMPARE( fromCode->description(), QStringLiteral( "optional" ) ); QCOMPARE( fromCode->flags(), def->flags() ); QCOMPARE( fromCode->defaultValue(), def->defaultValue() ); + + // non optional, no default + def.reset( new QgsProcessingParameterExpression( "non_optional", QString(), QString(), QString(), false ) ); + QVERIFY( def->checkValueIsAcceptable( 1 ) ); + QVERIFY( def->checkValueIsAcceptable( "test" ) ); + QVERIFY( !def->checkValueIsAcceptable( "" ) ); + QVERIFY( !def->checkValueIsAcceptable( QVariant() ) ); // should NOT be acceptable, because it will fallback to invalid default value + } void TestQgsProcessing::parameterField()