From 2e2721ae2c69ecba480aa50284261922b0f6fa37 Mon Sep 17 00:00:00 2001 From: Richard Kello Date: Sat, 8 Aug 2026 10:10:49 +0200 Subject: [PATCH 1/4] Add setter to avoid mismatch in values --- app/attributes/attributecontroller.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/attributes/attributecontroller.cpp b/app/attributes/attributecontroller.cpp index 6c071485a..702287ae5 100644 --- a/app/attributes/attributecontroller.cpp +++ b/app/attributes/attributecontroller.cpp @@ -624,6 +624,9 @@ void AttributeController::updateOnFeatureChange() { mFeatureLayerPair.featureRef().setAttribute( fieldIndex, rememberedValue ); itemData->setRawValue( rememberedValue ); + // keep originalValue in sync so change-detection (e.g. renamePhotos()) doesn't + // mistake a reused value for a freshly captured one + itemData->setOriginalValue( rememberedValue ); } } } From 32020d441549755f62090f9e6f55da15f7272f2c Mon Sep 17 00:00:00 2001 From: Richard Kello Date: Tue, 25 Aug 2026 14:32:25 +0200 Subject: [PATCH 2/4] Fix broken previews --- .../src/uk/co/lutraconsulting/MMActivity.java | 1 + app/attributes/attributecontroller.cpp | 81 +++++++++++++++++-- app/attributes/attributecontroller.h | 1 + app/attributes/attributedata.cpp | 20 +++++ app/attributes/attributedata.h | 8 ++ app/inpututils.cpp | 4 + app/ios/iosviewdelegate.mm | 2 + 7 files changed, 109 insertions(+), 8 deletions(-) diff --git a/app/android/src/uk/co/lutraconsulting/MMActivity.java b/app/android/src/uk/co/lutraconsulting/MMActivity.java index cb26be66a..988530b2d 100644 --- a/app/android/src/uk/co/lutraconsulting/MMActivity.java +++ b/app/android/src/uk/co/lutraconsulting/MMActivity.java @@ -201,6 +201,7 @@ public String importImage(Uri imageUri, String targetPath) { String fileName = getFileName( imageUri ); File newCopyFile = new File( targetPath + "/" + fileName ); try { + newCopyFile.getParentFile().mkdirs(); newCopyFile.createNewFile(); InputStream fileStream = getContentResolver().openInputStream( imageUri ); copyFile( fileStream, newCopyFile ); diff --git a/app/attributes/attributecontroller.cpp b/app/attributes/attributecontroller.cpp index 702287ae5..2acebbe4c 100644 --- a/app/attributes/attributecontroller.cpp +++ b/app/attributes/attributecontroller.cpp @@ -612,7 +612,8 @@ void AttributeController::updateOnFeatureChange() const QVariant newVal = feature.attribute( fieldIndex ); mFormItems[itemData->id()]->setOriginalValue( newVal ); mFormItems[itemData->id()]->setRawValue( newVal ); // we need to set raw value as well, as we use it in form now - if ( mRememberAttributesController && isNewFeature() ) // this is a new feature + itemData->setReusedValue( false ); + if ( mRememberAttributesController && isNewFeature() && newVal.toString().isEmpty() ) { QVariant rememberedValue; bool shouldUseRememberedValue = mRememberAttributesController->rememberedValue( @@ -622,11 +623,35 @@ void AttributeController::updateOnFeatureChange() ); if ( shouldUseRememberedValue ) { - mFeatureLayerPair.featureRef().setAttribute( fieldIndex, rememberedValue ); - itemData->setRawValue( rememberedValue ); - // keep originalValue in sync so change-detection (e.g. renamePhotos()) doesn't - // mistake a reused value for a freshly captured one - itemData->setOriginalValue( rememberedValue ); + QVariant valueToUse = rememberedValue; + + if ( itemData->editorWidgetType() == QStringLiteral( "ExternalResource" ) && !rememberedValue.toString().isEmpty() ) + { + const QVariantMap config = itemData->editorWidgetConfig(); + const FeatureLayerPair parentPair = mParentController ? mParentController->featureLayerPair() : FeatureLayerPair(); + const QString targetDir = InputUtils::resolveTargetDir( QgsProject::instance()->homePath(), config, mFeatureLayerPair, parentPair, QgsProject::instance() ); + const QString prefix = InputUtils::resolvePrefixForRelativePath( config[ QStringLiteral( "RelativeStorage" ) ].toInt(), QgsProject::instance()->homePath(), targetDir ); + const QString src = InputUtils::getAbsolutePath( rememberedValue.toString(), prefix ); + const QFileInfo fi( src ); + + static const QRegularExpression trailingCounter( QStringLiteral( "\\s\\(\\d+\\)$" ) ); + QString baseName = fi.completeBaseName(); + baseName.remove( trailingCounter ); + const QString canonicalName = fi.suffix().isEmpty() ? baseName : QStringLiteral( "%1.%2" ).arg( baseName, fi.suffix() ); + + const QString dst = CoreUtils::findUniquePath( InputUtils::getAbsolutePath( canonicalName, targetDir ) ); + + if ( InputUtils::copyFile( src, dst ) ) + { + valueToUse = InputUtils::getRelativePath( dst, prefix ); + itemData->setReusedCopyPath( dst ); + } + } + + mFeatureLayerPair.featureRef().setAttribute( fieldIndex, valueToUse ); + itemData->setRawValue( valueToUse ); + itemData->setOriginalValue( valueToUse ); + itemData->setReusedValue( true ); } } } @@ -794,7 +819,8 @@ void AttributeController::recalculateDefaultValues( bool shouldApplyDefaultValue = !defaultDefinition.expression().isEmpty() && - ( isFirstUpdateOfNewFeature || ( isFormValueChange && defaultDefinition.applyOnUpdate() ) ); + ( isFirstUpdateOfNewFeature || ( isFormValueChange && defaultDefinition.applyOnUpdate() ) ) && + !item->isReusedValue(); if ( shouldApplyDefaultValue ) { @@ -1214,6 +1240,8 @@ bool AttributeController::deleteFeature() bool AttributeController::rollback() { + discardReusedPhotoCopies( true ); + if ( !mFeatureLayerPair.layer() ) return false; @@ -1296,6 +1324,11 @@ bool AttributeController::save() disconnect( mFeatureLayerPair.layer(), &QgsVectorLayer::featureAdded, this, &AttributeController::onFeatureAdded ); } + if ( rv ) + { + discardReusedPhotoCopies( false ); + } + // Store the feature attributes for future use if ( featureIsNew && mRememberAttributesController ) { @@ -1500,6 +1533,7 @@ bool AttributeController::setFormValue( const QUuid &id, QVariant value ) QgsField field = item->field(); QVariant val( value ); + item->setReusedValue( false ); item->setRawValue( val ); emit formDataChanged( item->id(), { AttributeFormModel::RawValue } ); @@ -1579,6 +1613,36 @@ void AttributeController::onFeatureAdded( QgsFeatureId newFeatureId ) emit featureIdChanged(); } +void AttributeController::discardReusedPhotoCopies( bool force ) +{ + QMap>::const_iterator formItemsIterator = mFormItems.constBegin(); + while ( formItemsIterator != mFormItems.constEnd() ) + { + std::shared_ptr item = formItemsIterator.value(); + const QString copyPath = item->reusedCopyPath(); + if ( !copyPath.isEmpty() ) + { + bool stillReferenced = false; + if ( !force ) + { + const QVariantMap config = item->editorWidgetConfig(); + const FeatureLayerPair parentPair = mParentController ? mParentController->featureLayerPair() : FeatureLayerPair(); + const QString targetDir = InputUtils::resolveTargetDir( QgsProject::instance()->homePath(), config, mFeatureLayerPair, parentPair, QgsProject::instance() ); + const QString prefix = InputUtils::resolvePrefixForRelativePath( config[ QStringLiteral( "RelativeStorage" ) ].toInt(), QgsProject::instance()->homePath(), targetDir ); + const QString currentPath = InputUtils::getAbsolutePath( mFeatureLayerPair.feature().attribute( item->fieldIndex() ).toString(), prefix ); + stillReferenced = ( currentPath == copyPath ); + } + + if ( force || !stillReferenced ) + { + InputUtils::removeFile( copyPath ); + } + item->setReusedCopyPath( QString() ); + } + ++formItemsIterator; + } +} + void AttributeController::renamePhotos() { const QStringList photoNameFormat = QgsProject::instance()->entryList( QStringLiteral( "Mergin" ), QStringLiteral( "PhotoNaming/%1" ).arg( mFeatureLayerPair.layer()->id() ) ); @@ -1611,7 +1675,7 @@ void AttributeController::renamePhotos() continue; } - if ( item->originalValue() != mFeatureLayerPair.feature().attribute( item->fieldIndex() ) ) + if ( item->isReusedValue() || item->originalValue() != mFeatureLayerPair.feature().attribute( item->fieldIndex() ) ) { const QString expString = QgsProject::instance()->readEntry( QStringLiteral( "Mergin" ), QStringLiteral( "PhotoNaming/%1/%2" ).arg( mFeatureLayerPair.layer()->id(), field.name() ) ); QgsExpression exp( expString ); @@ -1659,6 +1723,7 @@ void AttributeController::renamePhotos() { const QString newValue = InputUtils::getRelativePath( dst, prefix ); setFormValue( item->id(), newValue ); + item->setReusedCopyPath( QString() ); expressionContext.setFeature( featureLayerPair().featureRef() ); } else diff --git a/app/attributes/attributecontroller.h b/app/attributes/attributecontroller.h index 9915b3e46..269b9b2bd 100644 --- a/app/attributes/attributecontroller.h +++ b/app/attributes/attributecontroller.h @@ -222,6 +222,7 @@ class AttributeController : public QObject */ bool allowTabs( QgsAttributeEditorContainer *container ); + void discardReusedPhotoCopies( bool force ); //! renames photos if necessary void renamePhotos(); //! save temporary sketched image to original image diff --git a/app/attributes/attributedata.cpp b/app/attributes/attributedata.cpp index a9e761cda..c19124789 100644 --- a/app/attributes/attributedata.cpp +++ b/app/attributes/attributedata.cpp @@ -300,6 +300,26 @@ void FormItem::setOriginalValue( const QVariant &originalValue ) mOriginalValue = originalValue; } +bool FormItem::isReusedValue() const +{ + return mIsReusedValue; +} + +void FormItem::setReusedValue( bool reused ) +{ + mIsReusedValue = reused; +} + +QString FormItem::reusedCopyPath() const +{ + return mReusedCopyPath; +} + +void FormItem::setReusedCopyPath( const QString &path ) +{ + mReusedCopyPath = path; +} + QgsRelation FormItem::relation() const { return mRelation; diff --git a/app/attributes/attributedata.h b/app/attributes/attributedata.h index 9a5178be3..f9964543c 100644 --- a/app/attributes/attributedata.h +++ b/app/attributes/attributedata.h @@ -152,6 +152,12 @@ class FormItem QVariant rawValue() const; void setRawValue( const QVariant &rawValue ); + bool isReusedValue() const; + void setReusedValue( bool reused ); + + QString reusedCopyPath() const; + void setReusedCopyPath( const QString &path ); + QgsRelation relation() const; QString fieldError() const; @@ -178,6 +184,8 @@ class FormItem bool mVisible = false; QVariant mOriginalValue; // original unmodified value QVariant mRawValue; + bool mIsReusedValue = false; + QString mReusedCopyPath; const QgsRelation mRelation; // Only used for FormItemType::Relation }; diff --git a/app/inpututils.cpp b/app/inpututils.cpp index 49fbb6716..5fd6b5203 100644 --- a/app/inpututils.cpp +++ b/app/inpututils.cpp @@ -1018,6 +1018,8 @@ QString InputUtils::resolveTargetDir( const QString &homePath, const QVariantMap { QString result = evaluateExpression( pair, parentPair, activeProject, expression ); sanitizePath( result ); + if ( !result.isEmpty() && !QDir::isAbsolutePath( result ) ) + result = QDir( homePath ).absoluteFilePath( result ); return result; } else @@ -1029,6 +1031,8 @@ QString InputUtils::resolveTargetDir( const QString &homePath, const QVariantMap } else { + if ( !QDir::isAbsolutePath( defaultRoot ) ) + defaultRoot = QDir( homePath ).absoluteFilePath( defaultRoot ); return defaultRoot; } } diff --git a/app/ios/iosviewdelegate.mm b/app/ios/iosviewdelegate.mm index 86d5e243f..3b932df10 100644 --- a/app/ios/iosviewdelegate.mm +++ b/app/ios/iosviewdelegate.mm @@ -94,6 +94,8 @@ - ( void )picker:( PHPickerViewController * )picker didFinishPicking:( NSArray

Date: Tue, 25 Aug 2026 15:09:02 +0200 Subject: [PATCH 3/4] Update and add tests --- app/test/testutilsfunctions.cpp | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/app/test/testutilsfunctions.cpp b/app/test/testutilsfunctions.cpp index 376852341..97f67d707 100644 --- a/app/test/testutilsfunctions.cpp +++ b/app/test/testutilsfunctions.cpp @@ -331,6 +331,7 @@ void TestUtilsFunctions::resolveTargetDir() { QString homePath = TestUtils::testDataDir(); QString DEFAULT_ROOT( "DEFAULT/ROOT/PATH" ); // can be not existing path + QString ABSOLUTE_DEFAULT_ROOT( "/absolute/default/root/path" ); // can be not existing path QgsProject *activeProject = nullptr; QVariantMap config; @@ -344,10 +345,16 @@ void TestUtilsFunctions::resolveTargetDir() QString resultDir = mUtils->resolveTargetDir( homePath, config, pair, FeatureLayerPair(), activeProject ); QCOMPARE( resultDir, homePath ); - // case 2: defined default root config, no expression + // case 2: defined default root config as a relative path, no expression - resolved against homePath config.insert( QStringLiteral( "DefaultRoot" ), DEFAULT_ROOT ); QString resultDir2 = mUtils->resolveTargetDir( homePath, config, pair, FeatureLayerPair(), activeProject ); - QCOMPARE( resultDir2, DEFAULT_ROOT ); + QCOMPARE( resultDir2, QStringLiteral( "%1/%2" ).arg( homePath, DEFAULT_ROOT ) ); + config.clear(); + + // case 2b: defined default root config as an already-absolute path, no expression - stays unchanged + config.insert( QStringLiteral( "DefaultRoot" ), ABSOLUTE_DEFAULT_ROOT ); + QString resultDir2b = mUtils->resolveTargetDir( homePath, config, pair, FeatureLayerPair(), activeProject ); + QCOMPARE( resultDir2b, ABSOLUTE_DEFAULT_ROOT ); config.clear(); // case 3: defined expression in config->"PropertyCollection" -> "properties" -> "propertyRootPath" -> "expression" From d3ac929a31c3391972847c11617c64ae262d437d Mon Sep 17 00:00:00 2001 From: Richard Kello Date: Tue, 8 Sep 2026 13:48:15 +0200 Subject: [PATCH 4/4] Add tests, fix wrong photo renaming --- app/attributes/attributecontroller.cpp | 34 +- app/test/testattributecontroller.cpp | 452 +++++++++++++++++++++++++ app/test/testattributecontroller.h | 25 ++ core/coreutils.cpp | 6 +- core/coreutils.h | 5 +- 5 files changed, 501 insertions(+), 21 deletions(-) diff --git a/app/attributes/attributecontroller.cpp b/app/attributes/attributecontroller.cpp index 2acebbe4c..fb6a8eda3 100644 --- a/app/attributes/attributecontroller.cpp +++ b/app/attributes/attributecontroller.cpp @@ -612,7 +612,7 @@ void AttributeController::updateOnFeatureChange() const QVariant newVal = feature.attribute( fieldIndex ); mFormItems[itemData->id()]->setOriginalValue( newVal ); mFormItems[itemData->id()]->setRawValue( newVal ); // we need to set raw value as well, as we use it in form now - itemData->setReusedValue( false ); + mFormItems[itemData->id()]->setReusedValue( false ); if ( mRememberAttributesController && isNewFeature() && newVal.toString().isEmpty() ) { QVariant rememberedValue; @@ -639,7 +639,7 @@ void AttributeController::updateOnFeatureChange() baseName.remove( trailingCounter ); const QString canonicalName = fi.suffix().isEmpty() ? baseName : QStringLiteral( "%1.%2" ).arg( baseName, fi.suffix() ); - const QString dst = CoreUtils::findUniquePath( InputUtils::getAbsolutePath( canonicalName, targetDir ) ); + const QString dst = CoreUtils::findUniquePath( InputUtils::getAbsolutePath( canonicalName, targetDir ), true ); if ( InputUtils::copyFile( src, dst ) ) { @@ -651,7 +651,8 @@ void AttributeController::updateOnFeatureChange() mFeatureLayerPair.featureRef().setAttribute( fieldIndex, valueToUse ); itemData->setRawValue( valueToUse ); itemData->setOriginalValue( valueToUse ); - itemData->setReusedValue( true ); + // an empty value means there's nothing to reuse, so don't mark it as such + itemData->setReusedValue( !valueToUse.toString().isEmpty() ); } } } @@ -817,6 +818,7 @@ void AttributeController::recalculateDefaultValues( const QgsField field = item->field(); const QgsDefaultValue defaultDefinition = field.defaultValueDefinition(); + // don't let a Default Value expression overwrite a value we just reused bool shouldApplyDefaultValue = !defaultDefinition.expression().isEmpty() && ( isFirstUpdateOfNewFeature || ( isFormValueChange && defaultDefinition.applyOnUpdate() ) ) && @@ -1326,6 +1328,7 @@ bool AttributeController::save() if ( rv ) { + // catches a reused copy that got deleted/replaced before save discardReusedPhotoCopies( false ); } @@ -1622,18 +1625,9 @@ void AttributeController::discardReusedPhotoCopies( bool force ) const QString copyPath = item->reusedCopyPath(); if ( !copyPath.isEmpty() ) { - bool stillReferenced = false; - if ( !force ) - { - const QVariantMap config = item->editorWidgetConfig(); - const FeatureLayerPair parentPair = mParentController ? mParentController->featureLayerPair() : FeatureLayerPair(); - const QString targetDir = InputUtils::resolveTargetDir( QgsProject::instance()->homePath(), config, mFeatureLayerPair, parentPair, QgsProject::instance() ); - const QString prefix = InputUtils::resolvePrefixForRelativePath( config[ QStringLiteral( "RelativeStorage" ) ].toInt(), QgsProject::instance()->homePath(), targetDir ); - const QString currentPath = InputUtils::getAbsolutePath( mFeatureLayerPair.feature().attribute( item->fieldIndex() ).toString(), prefix ); - stillReferenced = ( currentPath == copyPath ); - } - - if ( force || !stillReferenced ) + // isReusedValue() is cleared as soon as the field changes, so it already tells + // whether the copy is still the field's current value + if ( force || !item->isReusedValue() ) { InputUtils::removeFile( copyPath ); } @@ -1716,14 +1710,18 @@ void AttributeController::renamePhotos() InputUtils::sanitizePath( newName ); const QFileInfo fi( src ); - newName = QStringLiteral( "%1.%2" ).arg( newName, fi.completeSuffix() ); + newName = QStringLiteral( "%1.%2" ).arg( newName, fi.suffix() ); - const QString dst = CoreUtils::findUniquePath( InputUtils::getAbsolutePath( newName, targetDir ) ); + const QString dst = CoreUtils::findUniquePath( InputUtils::getAbsolutePath( newName, targetDir ), true ); if ( InputUtils::renameFile( src, dst ) ) { const QString newValue = InputUtils::getRelativePath( dst, prefix ); setFormValue( item->id(), newValue ); - item->setReusedCopyPath( QString() ); + // keep originalValue() in sync so this doesn't get renamed again on the next save + item->setOriginalValue( newValue ); + // only clear if we actually renamed the tracked clone, not a replacement photo + if ( src == item->reusedCopyPath() ) + item->setReusedCopyPath( QString() ); expressionContext.setFeature( featureLayerPair().featureRef() ); } else diff --git a/app/test/testattributecontroller.cpp b/app/test/testattributecontroller.cpp index 2145bb058..2c1ed87a2 100644 --- a/app/test/testattributecontroller.cpp +++ b/app/test/testattributecontroller.cpp @@ -19,6 +19,7 @@ #include "qgsapplication.h" #include "qgsvectorlayer.h" #include "qgsproject.h" +#include "qgsdefaultvalue.h" #include "attributecontroller.h" #include "attributetabproxymodel.h" @@ -894,6 +895,457 @@ void TestAttributeController::testPhotoRenaming() } } +void TestAttributeController::testPhotoRenamingCollisionWithDotInName() +{ + QString projectName = QStringLiteral( "testPhotoRenamingCollisionWithDotInName" ); + QString projectDir = QDir::tempPath() + "/MM_test_projects/" + projectName; + + QDir tempDir( projectDir ); + QVERIFY( tempDir.removeRecursively() ); + + QVERIFY( InputUtils::cpDir( TestUtils::testDataDir() + "/test_photo_rename", projectDir ) ); + QVERIFY( QFile::exists( projectDir + QStringLiteral( "/image1.jpg" ) ) ); + + // "photo" naming expression is 'image_' + "notes", so a dot in notes lands in the name too + const QString collidingPath = projectDir + QStringLiteral( "/image_my.notes.jpg" ); + QVERIFY( QFile::copy( projectDir + QStringLiteral( "/image1.jpg" ), collidingPath ) ); + QVERIFY( QFile::exists( collidingPath ) ); + + QVERIFY( QgsProject::instance()->read( projectDir + QStringLiteral( "/test_photo_rename.qgz" ) ) ); + + QgsMapLayer *layer = QgsProject::instance()->mapLayersByName( QStringLiteral( "Survey" ) ).at( 0 ); + QgsVectorLayer *surveyLayer = static_cast( layer ); + QVERIFY( surveyLayer && surveyLayer->isValid() ); + + QgsFeature feat( surveyLayer->fields() ); + FeatureLayerPair pair( feat, surveyLayer ); + + AttributeController controller; + controller.setFeatureLayerPair( pair ); + + const TabItem *tab = controller.tabItem( 0 ); + const QVector items = tab->formItems(); + + controller.setFormValue( items.at( 2 ), QStringLiteral( "my.notes" ) ); + controller.setFormValue( items.at( 3 ), QStringLiteral( "image1.jpg" ) ); + + controller.save(); + + const QgsFeature f = controller.featureLayerPair().feature(); + + QVERIFY( QFile::exists( collidingPath ) ); // untouched + QVERIFY( !QFile::exists( projectDir + QStringLiteral( "/image_my (1).notes.jpg" ) ) ); + QVERIFY( QFile::exists( projectDir + QStringLiteral( "/image_my.notes (1).jpg" ) ) ); + QCOMPARE( f.attribute( 3 ), QStringLiteral( "image_my.notes (1).jpg" ) ); +} + +void TestAttributeController::testPhotoReuseRenamesWithFreshExpressionValue() +{ + QString projectName = QStringLiteral( "testPhotoReuseRenamesWithFreshExpressionValue" ); + QString projectDir = QDir::tempPath() + "/MM_test_projects/" + projectName; + + QDir tempDir( projectDir ); + QVERIFY( tempDir.removeRecursively() ); + + QVERIFY( InputUtils::cpDir( TestUtils::testDataDir() + "/test_photo_rename", projectDir ) ); + QVERIFY( QFile::exists( projectDir + QStringLiteral( "/image1.jpg" ) ) ); + + QVERIFY( QgsProject::instance()->read( projectDir + QStringLiteral( "/test_photo_rename.qgz" ) ) ); + + QgsMapLayer *layer = QgsProject::instance()->mapLayersByName( QStringLiteral( "Survey" ) ).at( 0 ); + QgsVectorLayer *surveyLayer = static_cast( layer ); + QVERIFY( surveyLayer && surveyLayer->isValid() ); + + RememberAttributesController remController; + remController.reset(); + remController.setRememberValuesAllowed( true ); + + // feature 1: notes = "first" -> naming expression 'image_' + "notes" saves image_first.jpg + QgsFeature feat1( surveyLayer->fields() ); + FeatureLayerPair pair1( feat1, surveyLayer ); + + AttributeController controller1; + controller1.setRememberAttributesController( &remController ); + controller1.setFeatureLayerPair( pair1 ); + + const TabItem *tab1 = controller1.tabItem( 0 ); + const QVector items1 = tab1->formItems(); + + // mark the photo field to be reused on the next new feature + remController.setShouldRememberValue( surveyLayer, 3, true ); + + controller1.setFormValue( items1.at( 2 ), QStringLiteral( "first" ) ); + controller1.setFormValue( items1.at( 3 ), QStringLiteral( "image1.jpg" ) ); + controller1.save(); + + QVERIFY( QFile::exists( projectDir + QStringLiteral( "/image_first.jpg" ) ) ); + + // feature 2 reuses the photo but has notes = "second" - expression must re-evaluate fresh + QgsFeature feat2( surveyLayer->fields() ); + FeatureLayerPair pair2( feat2, surveyLayer ); + + AttributeController controller2; + controller2.setRememberAttributesController( &remController ); + controller2.setFeatureLayerPair( pair2 ); + + const TabItem *tab2 = controller2.tabItem( 0 ); + const QVector items2 = tab2->formItems(); + + controller2.setFormValue( items2.at( 2 ), QStringLiteral( "second" ) ); + controller2.save(); + + const QgsFeature f2 = controller2.featureLayerPair().feature(); + + QVERIFY( QFile::exists( projectDir + QStringLiteral( "/image_first.jpg" ) ) ); // untouched + QVERIFY( QFile::exists( projectDir + QStringLiteral( "/image_second.jpg" ) ) ); // fresh name + QCOMPARE( f2.attribute( 3 ), QStringLiteral( "image_second.jpg" ) ); +} + +void TestAttributeController::testDefaultValueDoesNotOverwriteReusedPhoto() +{ + QString projectName = QStringLiteral( "testDefaultValueDoesNotOverwriteReusedPhoto" ); + QString projectDir = QDir::tempPath() + "/MM_test_projects/" + projectName; + + QDir tempDir( projectDir ); + QVERIFY( tempDir.removeRecursively() ); + + QVERIFY( InputUtils::cpDir( TestUtils::testDataDir() + "/test_photo_rename", projectDir ) ); + QVERIFY( QgsProject::instance()->read( projectDir + QStringLiteral( "/test_photo_rename.qgz" ) ) ); + + QgsMapLayer *layer = QgsProject::instance()->mapLayersByName( QStringLiteral( "Survey" ) ).at( 0 ); + QgsVectorLayer *surveyLayer = static_cast( layer ); + QVERIFY( surveyLayer && surveyLayer->isValid() ); + + // field-level Default Value, only applied on a brand new feature - same moment as reuse + surveyLayer->setDefaultValueDefinition( 3, QgsDefaultValue( QStringLiteral( "'should_not_apply.jpg'" ), false ) ); + + RememberAttributesController remController; + remController.reset(); + remController.setRememberValuesAllowed( true ); + + QgsFeature feat1( surveyLayer->fields() ); + FeatureLayerPair pair1( feat1, surveyLayer ); + + AttributeController controller1; + controller1.setRememberAttributesController( &remController ); + controller1.setFeatureLayerPair( pair1 ); + + const TabItem *tab1 = controller1.tabItem( 0 ); + const QVector items1 = tab1->formItems(); + + remController.setShouldRememberValue( surveyLayer, 3, true ); + + controller1.setFormValue( items1.at( 2 ), QStringLiteral( "first" ) ); + controller1.setFormValue( items1.at( 3 ), QStringLiteral( "image1.jpg" ) ); + controller1.save(); + + QVERIFY( QFile::exists( projectDir + QStringLiteral( "/image_first.jpg" ) ) ); + + // feature 2: photo is reused - the Default Value expression must not clobber it + QgsFeature feat2( surveyLayer->fields() ); + FeatureLayerPair pair2( feat2, surveyLayer ); + + AttributeController controller2; + controller2.setRememberAttributesController( &remController ); + controller2.setFeatureLayerPair( pair2 ); + + const TabItem *tab2 = controller2.tabItem( 0 ); + const FormItem *photoItem2 = controller2.formItem( tab2->formItems().at( 3 ) ); + + QVERIFY( photoItem2->isReusedValue() ); + QVERIFY( !photoItem2->rawValue().toString().isEmpty() ); + QVERIFY( photoItem2->rawValue().toString() != QStringLiteral( "should_not_apply.jpg" ) ); +} + +void TestAttributeController::testDefaultValueAppliesWhenNothingReused() +{ + QString projectName = QStringLiteral( "testDefaultValueAppliesWhenNothingReused" ); + QString projectDir = QDir::tempPath() + "/MM_test_projects/" + projectName; + + QDir tempDir( projectDir ); + QVERIFY( tempDir.removeRecursively() ); + + QVERIFY( InputUtils::cpDir( TestUtils::testDataDir() + "/test_photo_rename", projectDir ) ); + QVERIFY( QgsProject::instance()->read( projectDir + QStringLiteral( "/test_photo_rename.qgz" ) ) ); + + QgsMapLayer *layer = QgsProject::instance()->mapLayersByName( QStringLiteral( "Survey" ) ).at( 0 ); + QgsVectorLayer *surveyLayer = static_cast( layer ); + QVERIFY( surveyLayer && surveyLayer->isValid() ); + + surveyLayer->setDefaultValueDefinition( 3, QgsDefaultValue( QStringLiteral( "'default_photo.jpg'" ), false ) ); + + RememberAttributesController remController; + remController.reset(); + remController.setRememberValuesAllowed( true ); + + QgsFeature feat1( surveyLayer->fields() ); + FeatureLayerPair pair1( feat1, surveyLayer ); + + AttributeController controller1; + controller1.setRememberAttributesController( &remController ); + controller1.setFeatureLayerPair( pair1 ); + + const TabItem *tab1 = controller1.tabItem( 0 ); + const QVector items1 = tab1->formItems(); + + remController.setShouldRememberValue( surveyLayer, 3, true ); + + controller1.setFormValue( items1.at( 2 ), QStringLiteral( "first" ) ); + // photo left empty - nothing to reuse for the next feature + controller1.save(); + + // feature 2: nothing was reused, so the Default Value expression must still apply + QgsFeature feat2( surveyLayer->fields() ); + FeatureLayerPair pair2( feat2, surveyLayer ); + + AttributeController controller2; + controller2.setRememberAttributesController( &remController ); + controller2.setFeatureLayerPair( pair2 ); + + const TabItem *tab2 = controller2.tabItem( 0 ); + const FormItem *photoItem2 = controller2.formItem( tab2->formItems().at( 3 ) ); + + QVERIFY( !photoItem2->isReusedValue() ); + QCOMPARE( photoItem2->rawValue().toString(), QStringLiteral( "default_photo.jpg" ) ); +} + +void TestAttributeController::testPhotoRenamingNotRepeatedOnResave() +{ + QString projectName = QStringLiteral( "testPhotoRenamingNotRepeatedOnResave" ); + QString projectDir = QDir::tempPath() + "/MM_test_projects/" + projectName; + + QDir tempDir( projectDir ); + QVERIFY( tempDir.removeRecursively() ); + + QVERIFY( InputUtils::cpDir( TestUtils::testDataDir() + "/test_photo_rename", projectDir ) ); + QVERIFY( QgsProject::instance()->read( projectDir + QStringLiteral( "/test_photo_rename.qgz" ) ) ); + + QgsMapLayer *layer = QgsProject::instance()->mapLayersByName( QStringLiteral( "Survey" ) ).at( 0 ); + QgsVectorLayer *surveyLayer = static_cast( layer ); + QVERIFY( surveyLayer && surveyLayer->isValid() ); + + RememberAttributesController remController; + remController.reset(); + remController.setRememberValuesAllowed( true ); + + QgsFeature feat1( surveyLayer->fields() ); + FeatureLayerPair pair1( feat1, surveyLayer ); + + AttributeController controller1; + controller1.setRememberAttributesController( &remController ); + controller1.setFeatureLayerPair( pair1 ); + + const TabItem *tab1 = controller1.tabItem( 0 ); + const QVector items1 = tab1->formItems(); + + remController.setShouldRememberValue( surveyLayer, 3, true ); + + controller1.setFormValue( items1.at( 2 ), QStringLiteral( "first" ) ); + controller1.setFormValue( items1.at( 3 ), QStringLiteral( "image1.jpg" ) ); + controller1.save(); + + QgsFeature feat2( surveyLayer->fields() ); + FeatureLayerPair pair2( feat2, surveyLayer ); + + AttributeController controller2; + controller2.setRememberAttributesController( &remController ); + controller2.setFeatureLayerPair( pair2 ); + + const TabItem *tab2 = controller2.tabItem( 0 ); + const QVector items2 = tab2->formItems(); + + controller2.setFormValue( items2.at( 2 ), QStringLiteral( "second" ) ); + controller2.save(); + + QVERIFY( QFile::exists( projectDir + QStringLiteral( "/image_second.jpg" ) ) ); + + // save the very same feature again without changing anything - must not re-trigger the rename + controller2.save(); + + QVERIFY( QFile::exists( projectDir + QStringLiteral( "/image_second.jpg" ) ) ); + QVERIFY( !QFile::exists( projectDir + QStringLiteral( "/image_second (1).jpg" ) ) ); +} + +void TestAttributeController::testDiscardReusedPhotoCopyOnRollback() +{ + QString projectName = QStringLiteral( "testDiscardReusedPhotoCopyOnRollback" ); + QString projectDir = QDir::tempPath() + "/MM_test_projects/" + projectName; + + QDir tempDir( projectDir ); + QVERIFY( tempDir.removeRecursively() ); + + QVERIFY( InputUtils::cpDir( TestUtils::testDataDir() + "/test_photo_rename", projectDir ) ); + QVERIFY( QgsProject::instance()->read( projectDir + QStringLiteral( "/test_photo_rename.qgz" ) ) ); + + QgsMapLayer *layer = QgsProject::instance()->mapLayersByName( QStringLiteral( "Survey" ) ).at( 0 ); + QgsVectorLayer *surveyLayer = static_cast( layer ); + QVERIFY( surveyLayer && surveyLayer->isValid() ); + + RememberAttributesController remController; + remController.reset(); + remController.setRememberValuesAllowed( true ); + + QgsFeature feat1( surveyLayer->fields() ); + FeatureLayerPair pair1( feat1, surveyLayer ); + + AttributeController controller1; + controller1.setRememberAttributesController( &remController ); + controller1.setFeatureLayerPair( pair1 ); + + const TabItem *tab1 = controller1.tabItem( 0 ); + const QVector items1 = tab1->formItems(); + + remController.setShouldRememberValue( surveyLayer, 3, true ); + + controller1.setFormValue( items1.at( 2 ), QStringLiteral( "first" ) ); + controller1.setFormValue( items1.at( 3 ), QStringLiteral( "image1.jpg" ) ); + controller1.save(); + + QVERIFY( QFile::exists( projectDir + QStringLiteral( "/image_first.jpg" ) ) ); + + // feature 2: photo gets reused (cloned), but the draft is discarded before it is ever saved + QgsFeature feat2( surveyLayer->fields() ); + FeatureLayerPair pair2( feat2, surveyLayer ); + + AttributeController controller2; + controller2.setRememberAttributesController( &remController ); + controller2.setFeatureLayerPair( pair2 ); + + const QString clonedRelativePath = controller2.featureLayerPair().feature().attribute( 3 ).toString(); + QVERIFY( !clonedRelativePath.isEmpty() ); + const QString clonedAbsolutePath = projectDir + "/" + clonedRelativePath; + QVERIFY( QFile::exists( clonedAbsolutePath ) ); + + controller2.rollback(); + + QVERIFY( !QFile::exists( clonedAbsolutePath ) ); + QVERIFY( QFile::exists( projectDir + QStringLiteral( "/image_first.jpg" ) ) ); +} + +void TestAttributeController::testDiscardReusedPhotoCopyOnReplace() +{ + QString projectName = QStringLiteral( "testDiscardReusedPhotoCopyOnReplace" ); + QString projectDir = QDir::tempPath() + "/MM_test_projects/" + projectName; + + QDir tempDir( projectDir ); + QVERIFY( tempDir.removeRecursively() ); + + QVERIFY( InputUtils::cpDir( TestUtils::testDataDir() + "/test_photo_rename", projectDir ) ); + QVERIFY( QFile::exists( projectDir + QStringLiteral( "/image1.jpg" ) ) ); + QVERIFY( QFile::exists( projectDir + QStringLiteral( "/image2.jpg" ) ) ); + + QVERIFY( QgsProject::instance()->read( projectDir + QStringLiteral( "/test_photo_rename.qgz" ) ) ); + + QgsMapLayer *layer = QgsProject::instance()->mapLayersByName( QStringLiteral( "Survey" ) ).at( 0 ); + QgsVectorLayer *surveyLayer = static_cast( layer ); + QVERIFY( surveyLayer && surveyLayer->isValid() ); + + RememberAttributesController remController; + remController.reset(); + remController.setRememberValuesAllowed( true ); + + QgsFeature feat1( surveyLayer->fields() ); + FeatureLayerPair pair1( feat1, surveyLayer ); + + AttributeController controller1; + controller1.setRememberAttributesController( &remController ); + controller1.setFeatureLayerPair( pair1 ); + + const TabItem *tab1 = controller1.tabItem( 0 ); + const QVector items1 = tab1->formItems(); + + remController.setShouldRememberValue( surveyLayer, 3, true ); + + controller1.setFormValue( items1.at( 2 ), QStringLiteral( "first" ) ); + controller1.setFormValue( items1.at( 3 ), QStringLiteral( "image1.jpg" ) ); + controller1.save(); + + QVERIFY( QFile::exists( projectDir + QStringLiteral( "/image_first.jpg" ) ) ); + + // feature 2: photo gets reused (cloned), then replaced with a different photo before saving + QgsFeature feat2( surveyLayer->fields() ); + FeatureLayerPair pair2( feat2, surveyLayer ); + + AttributeController controller2; + controller2.setRememberAttributesController( &remController ); + controller2.setFeatureLayerPair( pair2 ); + + const TabItem *tab2 = controller2.tabItem( 0 ); + const QVector items2 = tab2->formItems(); + + const QString clonedRelativePath = controller2.featureLayerPair().feature().attribute( 3 ).toString(); + QVERIFY( !clonedRelativePath.isEmpty() ); + const QString clonedAbsolutePath = projectDir + "/" + clonedRelativePath; + QVERIFY( QFile::exists( clonedAbsolutePath ) ); + + controller2.setFormValue( items2.at( 2 ), QStringLiteral( "replaced" ) ); + controller2.setFormValue( items2.at( 3 ), QStringLiteral( "image2.jpg" ) ); + controller2.save(); + + // the orphaned clone from the earlier reuse must be gone, feature 1's photo untouched + QVERIFY( !QFile::exists( clonedAbsolutePath ) ); + QVERIFY( QFile::exists( projectDir + QStringLiteral( "/image_first.jpg" ) ) ); + QVERIFY( QFile::exists( projectDir + QStringLiteral( "/image_replaced.jpg" ) ) ); +} + +void TestAttributeController::testReusedPhotoIsIndependentFile() +{ + QString projectName = QStringLiteral( "testReusedPhotoIsIndependentFile" ); + QString projectDir = QDir::tempPath() + "/MM_test_projects/" + projectName; + + QDir tempDir( projectDir ); + QVERIFY( tempDir.removeRecursively() ); + + QVERIFY( InputUtils::cpDir( TestUtils::testDataDir() + "/test_photo_rename", projectDir ) ); + QVERIFY( QgsProject::instance()->read( projectDir + QStringLiteral( "/test_photo_rename.qgz" ) ) ); + + QgsMapLayer *layer = QgsProject::instance()->mapLayersByName( QStringLiteral( "Survey" ) ).at( 0 ); + QgsVectorLayer *surveyLayer = static_cast( layer ); + QVERIFY( surveyLayer && surveyLayer->isValid() ); + + RememberAttributesController remController; + remController.reset(); + remController.setRememberValuesAllowed( true ); + + QgsFeature feat1( surveyLayer->fields() ); + FeatureLayerPair pair1( feat1, surveyLayer ); + + AttributeController controller1; + controller1.setRememberAttributesController( &remController ); + controller1.setFeatureLayerPair( pair1 ); + + const TabItem *tab1 = controller1.tabItem( 0 ); + const QVector items1 = tab1->formItems(); + + remController.setShouldRememberValue( surveyLayer, 3, true ); + + controller1.setFormValue( items1.at( 2 ), QStringLiteral( "first" ) ); + controller1.setFormValue( items1.at( 3 ), QStringLiteral( "image1.jpg" ) ); + controller1.save(); + + QVERIFY( QFile::exists( projectDir + QStringLiteral( "/image_first.jpg" ) ) ); + + QgsFeature feat2( surveyLayer->fields() ); + FeatureLayerPair pair2( feat2, surveyLayer ); + + AttributeController controller2; + controller2.setRememberAttributesController( &remController ); + controller2.setFeatureLayerPair( pair2 ); + + const QString reusedRelativePath = controller2.featureLayerPair().feature().attribute( 3 ).toString(); + QVERIFY( !reusedRelativePath.isEmpty() ); + // must be its own file, not literally feature 1's path + QVERIFY( reusedRelativePath != QStringLiteral( "image_first.jpg" ) ); + + const QString reusedAbsolutePath = projectDir + "/" + reusedRelativePath; + QVERIFY( QFile::exists( reusedAbsolutePath ) ); + QVERIFY( QFile::exists( projectDir + QStringLiteral( "/image_first.jpg" ) ) ); + + // removing feature 2's file directly must not affect feature 1's, since they're independent + QVERIFY( QFile::remove( reusedAbsolutePath ) ); + QVERIFY( QFile::exists( projectDir + QStringLiteral( "/image_first.jpg" ) ) ); +} + void TestAttributeController::testHtmlAndTextWidgets() { QString projectDir = TestUtils::testDataDir() + "/expressions"; diff --git a/app/test/testattributecontroller.h b/app/test/testattributecontroller.h index 5fa25b9dc..36f776a71 100644 --- a/app/test/testattributecontroller.h +++ b/app/test/testattributecontroller.h @@ -28,6 +28,31 @@ class TestAttributeController: public QObject void testRawValue(); void testFieldsOutsideForm(); void testPhotoRenaming(); + + //! A collision suffix must be inserted before the real extension, even if the name has a dot + void testPhotoRenamingCollisionWithDotInName(); + + //! A reused photo must be renamed with the new feature's own expression value, not the old one + void testPhotoReuseRenamesWithFreshExpressionValue(); + + //! A reused photo must not let a field-level Default Value expression overwrite it + void testDefaultValueDoesNotOverwriteReusedPhoto(); + + //! When there's nothing to reuse (empty value), the Default Value expression must still apply + void testDefaultValueAppliesWhenNothingReused(); + + //! Saving the same feature twice must not rename an already-renamed photo again + void testPhotoRenamingNotRepeatedOnResave(); + + //! Discarding a draft feature must delete the clone made for a reused photo + void testDiscardReusedPhotoCopyOnRollback(); + + //! Replacing a reused photo before saving must delete the now-orphaned clone + void testDiscardReusedPhotoCopyOnReplace(); + + //! Reusing a photo must create an independent file, not just copy the path string + void testReusedPhotoIsIndependentFile(); + void testHtmlAndTextWidgets(); void testVirtualFields(); diff --git a/core/coreutils.cpp b/core/coreutils.cpp index 143e1f81b..828423624 100644 --- a/core/coreutils.cpp +++ b/core/coreutils.cpp @@ -165,7 +165,7 @@ void CoreUtils::appendLog( const QByteArray &data, const QString &path ) } } -QString CoreUtils::findUniquePath( const QString &path ) +QString CoreUtils::findUniquePath( const QString &path, bool splitOnLastDot ) { QFileInfo originalPath( path ); QString uniquePath = path; @@ -186,7 +186,9 @@ QString CoreUtils::findUniquePath( const QString &path ) } else // file { - uniquePath = originalPath.path() + '/' + originalPath.baseName() + " (" + QString::number( i ) + ")." + originalPath.completeSuffix(); + const QString baseName = splitOnLastDot ? originalPath.completeBaseName() : originalPath.baseName(); + const QString suffix = splitOnLastDot ? originalPath.suffix() : originalPath.completeSuffix(); + uniquePath = originalPath.path() + '/' + baseName + " (" + QString::number( i ) + ")." + suffix; } f.setFile( uniquePath ); } diff --git a/core/coreutils.h b/core/coreutils.h index 990fbca45..26d2168a5 100644 --- a/core/coreutils.h +++ b/core/coreutils.h @@ -50,8 +50,11 @@ class CoreUtils * Returns given path if it does not exist yet, otherwise adds a number to the path in format: * - if path is a directory: "folder" -> "folder (1)" * - if path is a file: "filename.txt" -> "filename (1).txt" + * + * By default splits the name on the first dot, keeping compound extensions like ".tar.gz" + * intact. Pass splitOnLastDot TRUE for names with a dot but only a simple extension. */ - static QString findUniquePath( const QString &path ); + static QString findUniquePath( const QString &path, bool splitOnLastDot = false ); //! Creates a unique project directory for given project name (used for initial download of a project) static QString createUniqueProjectDirectory( const QString &baseDataDir, const QString &projectName );