Skip to content

Commit

Permalink
Merge pull request #7140 from rouault/fix_19009
Browse files Browse the repository at this point in the history
Assorted set of fixes regarding field length for OGR provider
  • Loading branch information
rouault authored Jun 1, 2018
2 parents 3b29102 + b9003ff commit 9d3f8d4
Show file tree
Hide file tree
Showing 5 changed files with 134 additions and 59 deletions.
43 changes: 32 additions & 11 deletions src/app/qgsfieldcalculator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,15 @@

#include <QMessageBox>

// FTC = FieldTypeCombo
constexpr int FTC_TYPE_ROLE_IDX = 0;
constexpr int FTC_TYPE_NAME_IDX = 1;
constexpr int FTC_MINLEN_IDX = 2;
constexpr int FTC_MAXLEN_IDX = 3;
constexpr int FTC_MINPREC_IDX = 4;
constexpr int FTC_MAXPREC_IDX = 5;
constexpr int FTC_SUBTYPE_IDX = 6;

QgsFieldCalculator::QgsFieldCalculator( QgsVectorLayer *vl, QWidget *parent )
: QDialog( parent )
, mVectorLayer( vl )
Expand Down Expand Up @@ -338,13 +347,13 @@ void QgsFieldCalculator::populateOutputFieldTypes()
for ( int i = 0; i < typelist.size(); i++ )
{
mOutputFieldTypeComboBox->addItem( typelist[i].mTypeDesc );
mOutputFieldTypeComboBox->setItemData( i, static_cast<int>( typelist[i].mType ), Qt::UserRole );
mOutputFieldTypeComboBox->setItemData( i, typelist[i].mTypeName, Qt::UserRole + 1 );
mOutputFieldTypeComboBox->setItemData( i, typelist[i].mMinLen, Qt::UserRole + 2 );
mOutputFieldTypeComboBox->setItemData( i, typelist[i].mMaxLen, Qt::UserRole + 3 );
mOutputFieldTypeComboBox->setItemData( i, typelist[i].mMinPrec, Qt::UserRole + 4 );
mOutputFieldTypeComboBox->setItemData( i, typelist[i].mMaxPrec, Qt::UserRole + 5 );
mOutputFieldTypeComboBox->setItemData( i, static_cast<int>( typelist[i].mSubType ), Qt::UserRole + 6 );
mOutputFieldTypeComboBox->setItemData( i, static_cast<int>( typelist[i].mType ), Qt::UserRole + FTC_TYPE_ROLE_IDX );
mOutputFieldTypeComboBox->setItemData( i, typelist[i].mTypeName, Qt::UserRole + FTC_TYPE_NAME_IDX );
mOutputFieldTypeComboBox->setItemData( i, typelist[i].mMinLen, Qt::UserRole + FTC_MINLEN_IDX );
mOutputFieldTypeComboBox->setItemData( i, typelist[i].mMaxLen, Qt::UserRole + FTC_MAXLEN_IDX );
mOutputFieldTypeComboBox->setItemData( i, typelist[i].mMinPrec, Qt::UserRole + FTC_MINPREC_IDX );
mOutputFieldTypeComboBox->setItemData( i, typelist[i].mMaxPrec, Qt::UserRole + FTC_MAXPREC_IDX );
mOutputFieldTypeComboBox->setItemData( i, static_cast<int>( typelist[i].mSubType ), Qt::UserRole + FTC_SUBTYPE_IDX );
}
mOutputFieldTypeComboBox->blockSignals( false );
mOutputFieldTypeComboBox->setCurrentIndex( 0 );
Expand Down Expand Up @@ -416,8 +425,8 @@ void QgsFieldCalculator::mOutputFieldNameLineEdit_textChanged( const QString &te

void QgsFieldCalculator::mOutputFieldTypeComboBox_activated( int index )
{
mOutputFieldWidthSpinBox->setMinimum( mOutputFieldTypeComboBox->itemData( index, Qt::UserRole + 2 ).toInt() );
mOutputFieldWidthSpinBox->setMaximum( mOutputFieldTypeComboBox->itemData( index, Qt::UserRole + 3 ).toInt() );
mOutputFieldWidthSpinBox->setMinimum( mOutputFieldTypeComboBox->itemData( index, Qt::UserRole + FTC_MINLEN_IDX ).toInt() );
mOutputFieldWidthSpinBox->setMaximum( mOutputFieldTypeComboBox->itemData( index, Qt::UserRole + FTC_MAXLEN_IDX ).toInt() );
mOutputFieldWidthSpinBox->setEnabled( mOutputFieldWidthSpinBox->minimum() < mOutputFieldWidthSpinBox->maximum() );
if ( mOutputFieldWidthSpinBox->value() < mOutputFieldWidthSpinBox->minimum() )
mOutputFieldWidthSpinBox->setValue( mOutputFieldWidthSpinBox->minimum() );
Expand Down Expand Up @@ -482,8 +491,8 @@ void QgsFieldCalculator::setOkButtonState()
void QgsFieldCalculator::setPrecisionMinMax()
{
int idx = mOutputFieldTypeComboBox->currentIndex();
int minPrecType = mOutputFieldTypeComboBox->itemData( idx, Qt::UserRole + 4 ).toInt();
int maxPrecType = mOutputFieldTypeComboBox->itemData( idx, Qt::UserRole + 5 ).toInt();
int minPrecType = mOutputFieldTypeComboBox->itemData( idx, Qt::UserRole + FTC_MINPREC_IDX ).toInt();
int maxPrecType = mOutputFieldTypeComboBox->itemData( idx, Qt::UserRole + FTC_MAXPREC_IDX ).toInt();
mOutputFieldPrecisionSpinBox->setEnabled( minPrecType < maxPrecType );
mOutputFieldPrecisionSpinBox->setMinimum( minPrecType );
mOutputFieldPrecisionSpinBox->setMaximum( std::max( minPrecType, std::min( maxPrecType, mOutputFieldWidthSpinBox->value() ) ) );
Expand All @@ -493,3 +502,15 @@ void QgsFieldCalculator::showHelp()
{
QgsHelp::openHelp( QStringLiteral( "working_with_vector/attribute_table.html#editing-attribute-values" ) );
}

QgsField QgsFieldCalculator::fieldDefinition()
{
return QgsField( mOutputFieldNameLineEdit->text(),
static_cast< QVariant::Type >( mOutputFieldTypeComboBox->currentData( Qt::UserRole + FTC_TYPE_ROLE_IDX ).toInt() ),
mOutputFieldTypeComboBox->currentData( Qt::UserRole + FTC_TYPE_NAME_IDX ).toString(),
mOutputFieldWidthSpinBox->value(),
mOutputFieldPrecisionSpinBox->value(),
QString(),
static_cast< QVariant::Type >( mOutputFieldTypeComboBox->currentData( Qt::UserRole + FTC_SUBTYPE_IDX ).toInt() )
);
}
12 changes: 1 addition & 11 deletions src/app/qgsfieldcalculator.h
Original file line number Diff line number Diff line change
Expand Up @@ -61,17 +61,7 @@ class APP_EXPORT QgsFieldCalculator: public QDialog, private Ui::QgsFieldCalcula
QMap<QString, int> mFieldMap;

//! Create a field based on the definitions
inline QgsField fieldDefinition()
{
return QgsField( mOutputFieldNameLineEdit->text(),
static_cast< QVariant::Type >( mOutputFieldTypeComboBox->currentData( Qt::UserRole ).toInt() ),
mOutputFieldTypeComboBox->currentData( Qt::UserRole + 1 ).toString(),
mOutputFieldWidthSpinBox->value(),
mOutputFieldPrecisionSpinBox->value(),
QString(),
static_cast< QVariant::Type >( mOutputFieldTypeComboBox->currentData( Qt::UserRole + 6 ).toInt() )
);
}
QgsField fieldDefinition();

//! Idx of changed attribute
int mAttributeId;
Expand Down
32 changes: 7 additions & 25 deletions src/core/qgsvectordataprovider.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -343,40 +343,22 @@ bool QgsVectorDataProvider::supportedType( const QgsField &field ) const
if ( field.type() != nativeType.mType )
continue;

if ( field.length() == -1 )
{
// source length unlimited
if ( nativeType.mMinLen > -1 || nativeType.mMaxLen > -1 )
{
// destination limited
continue;
}
}
else
if ( field.length() > 0 )
{
// source length limited
if ( nativeType.mMinLen > -1 && nativeType.mMaxLen > -1 &&
( field.length() < nativeType.mMinLen || field.length() > nativeType.mMaxLen ) )
if ( ( nativeType.mMinLen > 0 && field.length() < nativeType.mMinLen ) ||
( nativeType.mMaxLen > 0 && field.length() > nativeType.mMaxLen ) )
{
// source length exceeds destination limits
continue;
}
}

if ( field.precision() == -1 )
{
// source precision unlimited / n/a
if ( nativeType.mMinPrec > -1 || nativeType.mMaxPrec > -1 )
{
// destination limited
continue;
}
}
else
if ( field.precision() > 0 )
{
// source precision unlimited / n/a
if ( nativeType.mMinPrec > -1 && nativeType.mMaxPrec > -1 &&
( field.precision() < nativeType.mMinPrec || field.precision() > nativeType.mMaxPrec ) )
// source precision limited
if ( ( nativeType.mMinPrec > 0 && field.precision() < nativeType.mMinPrec ) ||
( nativeType.mMaxPrec > 0 && field.precision() > nativeType.mMaxPrec ) )
{
// source precision exceeds destination limits
continue;
Expand Down
86 changes: 74 additions & 12 deletions src/providers/ogr/qgsogrprovider.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -449,20 +449,60 @@ QgsOgrProvider::QgsOgrProvider( QString const &uri, const ProviderOptions &optio
open( OpenModeInitial );
int nMaxIntLen = 11;
int nMaxInt64Len = 21;
int nMaxDoubleLen = 20;
int nMaxDoublePrec = 15;
int nDateLen = 8;
if ( mGDALDriverName == QLatin1String( "GPKG" ) )
{
// GPKG only supports field length for text (and binary)
nMaxIntLen = 0;
nMaxInt64Len = 0;
nMaxDoubleLen = 0;
nMaxDoublePrec = 0;
nDateLen = 0;
}
QList<NativeType> nativeTypes;
nativeTypes
<< QgsVectorDataProvider::NativeType( tr( "Whole number (integer)" ), QStringLiteral( "integer" ), QVariant::Int, 0, 11 )
<< QgsVectorDataProvider::NativeType( tr( "Whole number (integer 64 bit)" ), QStringLiteral( "integer64" ), QVariant::LongLong, 0, 21 )
<< QgsVectorDataProvider::NativeType( tr( "Decimal number (real)" ), QStringLiteral( "double" ), QVariant::Double, 0, 20, 0, 15 )
<< QgsVectorDataProvider::NativeType( tr( "Text (string)" ), QStringLiteral( "string" ), QVariant::String, 0, 65535 )
<< QgsVectorDataProvider::NativeType( tr( "Date" ), QStringLiteral( "date" ), QVariant::Date, 8, 8 );
<< QgsVectorDataProvider::NativeType( tr( "Whole number (integer)" ), QStringLiteral( "integer" ), QVariant::Int, 0, nMaxIntLen )
<< QgsVectorDataProvider::NativeType( tr( "Whole number (integer 64 bit)" ), QStringLiteral( "integer64" ), QVariant::LongLong, 0, nMaxInt64Len )
<< QgsVectorDataProvider::NativeType( tr( "Decimal number (real)" ), QStringLiteral( "double" ), QVariant::Double, 0, nMaxDoubleLen, 0, nMaxDoublePrec )
<< QgsVectorDataProvider::NativeType( tr( "Text (string)" ), QStringLiteral( "string" ), QVariant::String, 0, 65535 );

bool supportsDate = true;
bool supportsTime = mGDALDriverName != QLatin1String( "ESRI Shapefile" ) && mGDALDriverName != QLatin1String( "GPKG" );
bool supportsDateTime = mGDALDriverName != QLatin1String( "ESRI Shapefile" );
const char *pszDataTypes = nullptr;
if ( mOgrOrigLayer )
{
pszDataTypes = GDALGetMetadataItem( mOgrOrigLayer->driver(), GDAL_DMD_CREATIONFIELDDATATYPES, nullptr );
}
// For drivers that advertize their data type, use that instead of the
// above hardcoded defaults.
if ( pszDataTypes )
{
char **papszTokens = CSLTokenizeString2( pszDataTypes, " ", 0 );
supportsDate = CSLFindString( papszTokens, "Date" ) >= 0;
supportsTime = CSLFindString( papszTokens, "Time" ) >= 0;
supportsDateTime = CSLFindString( papszTokens, "DateTime" ) >= 0;
CSLDestroy( papszTokens );
}

// Some drivers do not support datetime type
// Please help to fill this list
if ( mGDALDriverName != QLatin1String( "ESRI Shapefile" ) )
if ( supportsDate )
{
nativeTypes
<< QgsVectorDataProvider::NativeType( tr( "Date" ), QStringLiteral( "date" ), QVariant::Date, nDateLen, nDateLen );
}
if ( supportsTime )
{
nativeTypes
<< QgsVectorDataProvider::NativeType( tr( "Time" ), QStringLiteral( "time" ), QVariant::Time );
}
if ( supportsDateTime )
{
nativeTypes
<< QgsVectorDataProvider::NativeType( tr( "Time" ), QStringLiteral( "time" ), QVariant::Time, -1, -1 )
<< QgsVectorDataProvider::NativeType( tr( "Date & Time" ), QStringLiteral( "datetime" ), QVariant::DateTime );
}

Expand All @@ -471,8 +511,8 @@ QgsOgrProvider::QgsOgrProvider( QString const &uri, const ProviderOptions &optio
#if GDAL_VERSION_NUM >= GDAL_COMPUTE_VERSION(2,3,0)
if ( mOgrOrigLayer )
{
const char *pszDataTypes = GDALGetMetadataItem( mOgrOrigLayer->driver(), GDAL_DMD_CREATIONFIELDDATASUBTYPES, nullptr );
if ( pszDataTypes && strstr( pszDataTypes, "Boolean" ) )
const char *pszDataSubTypes = GDALGetMetadataItem( mOgrOrigLayer->driver(), GDAL_DMD_CREATIONFIELDDATASUBTYPES, nullptr );
if ( pszDataSubTypes && strstr( pszDataSubTypes, "Boolean" ) )
supportsBoolean = true;
}
#else
Expand All @@ -492,7 +532,7 @@ QgsOgrProvider::QgsOgrProvider( QString const &uri, const ProviderOptions &optio
{
// boolean data type
nativeTypes
<< QgsVectorDataProvider::NativeType( tr( "Boolean" ), QStringLiteral( "bool" ), QVariant::Bool, -1, -1, -1, -1 );
<< QgsVectorDataProvider::NativeType( tr( "Boolean" ), QStringLiteral( "bool" ), QVariant::Bool );
}

setNativeTypes( nativeTypes );
Expand Down Expand Up @@ -1567,6 +1607,10 @@ bool QgsOgrProvider::addAttributes( const QList<QgsField> &attributes )
returnvalue = false;
}
}

// Backup existing fields. We need them to 'restore' field type, length, precision
QgsFields oldFields = mAttributeFields;

loadFields();

// The check in QgsVectorLayerEditBuffer::commitChanges() is questionable with
Expand All @@ -1588,6 +1632,24 @@ bool QgsOgrProvider::addAttributes( const QList<QgsField> &attributes )
}
}

// Restore field type, length, precision of existing fields as well
// We need that in scenarios where the user adds a int field with length != 0
// in a editing session, and repeat that again in another editing session
// Without the below hack, the length of the first added field would have
// been reset to zero, and QgsVectorLayerEditBuffer::commitChanges() would
// error out because of this.
// See https://issues.qgis.org/issues/19009
for ( auto field : oldFields )
{
int idx = mAttributeFields.lookupField( field.name() );
if ( idx >= 0 )
{
mAttributeFields[ idx ].setType( field.type() );
mAttributeFields[ idx ].setLength( field.length() );
mAttributeFields[ idx ].setPrecision( field.precision() );
}
}

return returnvalue;
}

Expand Down
20 changes: 20 additions & 0 deletions tests/src/python/test_provider_ogr_gpkg.py
Original file line number Diff line number Diff line change
Expand Up @@ -929,6 +929,26 @@ def testRequestWithoutGeometryOnLayerMixedGeometry(self):
features = [f for f in vl.getFeatures(request)]
self.assertEqual(len(features), 1)

def testAddingTwoIntFieldsWithWidth(self):
""" Test buggfix for https://issues.qgis.org/issues/19009 """

tmpfile = os.path.join(self.basetestpath, 'testRequestWithoutGeometryOnLayerMixedGeometry.gpkg')
ds = ogr.GetDriverByName('GPKG').CreateDataSource(tmpfile)
lyr = ds.CreateLayer('test', geom_type=ogr.wkbPoint, options=['SPATIAL_INDEX=NO'])
lyr.CreateField(ogr.FieldDefn('a', ogr.OFTInteger))
ds = None

vl = QgsVectorLayer(u'{}'.format(tmpfile) + "|layername=" + "test", 'test', u'ogr')
self.assertTrue(vl.isValid())

vl.startEditing()
self.assertTrue(vl.addAttribute(QgsField("b", QVariant.Int, "integer", 10)))
self.assertTrue(vl.commitChanges())

vl.startEditing()
self.assertTrue(vl.addAttribute(QgsField("c", QVariant.Int, "integer", 10)))
self.assertTrue(vl.commitChanges())


if __name__ == '__main__':
unittest.main()

0 comments on commit 9d3f8d4

Please sign in to comment.