Skip to content
Permalink
Browse files

Refine logic relating to syncing project ellipsoid choice to CRS sele…

…ctions

in project properties dialog

Keep the existing logic which dictates that we should never overwrite
a user-set "NO ellipsoid" setting when the project CRS is changed, BUT
add a condition that if the project goes from NO CRS to A CRS, we
DO sync the ellipsoid choice for that first change only and overwrite
the default NO ellipsoid which is forced for projects with no crs.

Add unit tests to protect this logic, and hopefully see the last of
this crappy fragile dialog handling...

Fixes #33358

(cherry picked from commit 4275e7d)
  • Loading branch information
nyalldawson committed Dec 16, 2019
1 parent 1a88667 commit 90d7f5b6a21f5ac8ce74ec7a9092d1849c7ec0f6
Showing with 111 additions and 40 deletions.
  1. +31 −32 src/app/qgsprojectproperties.cpp
  2. +15 −8 src/app/qgsprojectproperties.h
  3. +65 −0 tests/src/app/testqgsprojectproperties.cpp
@@ -154,8 +154,13 @@ QgsProjectProperties::QgsProjectProperties( QgsMapCanvas *mapCanvas, QWidget *pa

connect( buttonBox->button( QDialogButtonBox::Apply ), &QAbstractButton::clicked, this, &QgsProjectProperties::apply );
connect( this, &QDialog::accepted, this, &QgsProjectProperties::apply );
connect( projectionSelector, &QgsProjectionSelectionTreeWidget::crsSelected, this, &QgsProjectProperties::srIdUpdated );
connect( projectionSelector, &QgsProjectionSelectionTreeWidget::initialized, this, &QgsProjectProperties::projectionSelectorInitialized );
connect( projectionSelector, &QgsProjectionSelectionTreeWidget::crsSelected, this, [ = ]
{
if ( mBlockCrsUpdates || !projectionSelector->hasValidSelection() )
return;

crsChanged( projectionSelector->crs() );
} );

connect( cmbEllipsoid, static_cast<void ( QComboBox::* )( int )>( &QComboBox::currentIndexChanged ), this, &QgsProjectProperties::updateEllipsoidUI );

@@ -172,7 +177,7 @@ QgsProjectProperties::QgsProjectProperties( QgsMapCanvas *mapCanvas, QWidget *pa

mCrs = QgsProject::instance()->crs();
updateGuiForMapUnits();
projectionSelector->setCrs( QgsProject::instance()->crs() );
setSelectedCrs( QgsProject::instance()->crs() );

// Datum transforms
QgsCoordinateTransformContext context = QgsProject::instance()->transformContext();
@@ -946,7 +951,9 @@ QgsProjectProperties::QgsProjectProperties( QgsMapCanvas *mapCanvas, QWidget *pa

connect( generateTsFileButton, &QPushButton::clicked, this, &QgsProjectProperties::onGenerateTsFileButton );

projectionSelectorInitialized();
// Reading ellipsoid from settings
setCurrentEllipsoid( QgsProject::instance()->ellipsoid() );

restoreOptionsBaseUi();

#ifdef QGISDEBUG
@@ -963,9 +970,16 @@ void QgsProjectProperties::title( QString const &title )
{
titleEdit->setText( title );
QgsProject::instance()->setTitle( title );
} // QgsProjectProperties::title( QString const & title )
}

void QgsProjectProperties::setSelectedCrs( const QgsCoordinateReferenceSystem &crs )
{
mBlockCrsUpdates = true;
projectionSelector->setCrs( crs );
mBlockCrsUpdates = false;
crsChanged( projectionSelector->crs() );
}

//when user clicks apply button
void QgsProjectProperties::apply()
{
mMapCanvas->enableMapTileRendering( mMapTileRenderingCheckBox->isChecked() );
@@ -1664,31 +1678,21 @@ void QgsProjectProperties::updateGuiForMapUnits()
}
}

void QgsProjectProperties::srIdUpdated()
void QgsProjectProperties::crsChanged( const QgsCoordinateReferenceSystem &crs )
{
if ( !projectionSelector->hasValidSelection() )
return;
const bool prevWasValid = mCrs.isValid();
mCrs = crs;

mCrs = projectionSelector->crs();
updateGuiForMapUnits();

if ( mCrs.isValid() )
{
cmbEllipsoid->setEnabled( true );
if ( cmbEllipsoid->currentIndex() != 0 )
// don't automatically change a "NONE" ellipsoid when the crs changes, UNLESS
// the project has gone from no CRS to a valid CRS
if ( !prevWasValid || cmbEllipsoid->currentIndex() != 0 )
{
// attempt to reset the projection ellipsoid according to the srs
int index = 0;
for ( int i = 0; i < mEllipsoidList.length(); i++ )
{
// TODO - use parameters instead of acronym
if ( mEllipsoidList[ i ].acronym == mCrs.ellipsoidAcronym() )
{
index = i;
break;
}
}
updateEllipsoidUI( index );
setCurrentEllipsoid( mCrs.ellipsoidAcronym() );
}
}
else
@@ -2428,18 +2432,13 @@ void QgsProjectProperties::updateEllipsoidUI( int newIndex )
cmbEllipsoid->setCurrentIndex( mEllipsoidIndex ); // Not always necessary
}

void QgsProjectProperties::projectionSelectorInitialized()
void QgsProjectProperties::setCurrentEllipsoid( const QString &ellipsoidAcronym )
{
QgsDebugMsgLevel( QStringLiteral( "Setting up ellipsoid" ), 4 );

// Reading ellipsoid from settings
const QString currentEllipsoid = QgsProject::instance()->ellipsoid();

int index = 0;
if ( currentEllipsoid.startsWith( QLatin1String( "PARAMETER" ) ) )
if ( ellipsoidAcronym.startsWith( QLatin1String( "PARAMETER" ) ) )
{
// Update parameters if present.
const QStringList mySplitEllipsoid = QgsProject::instance()->ellipsoid().split( ':' );
const QStringList mySplitEllipsoid = ellipsoidAcronym.split( ':' );
for ( int i = 0; i < mEllipsoidList.length(); i++ )
{
if ( mEllipsoidList.at( i ).acronym.startsWith( QLatin1String( "PARAMETER" ), Qt::CaseInsensitive ) )
@@ -2454,7 +2453,7 @@ void QgsProjectProperties::projectionSelectorInitialized()
{
for ( int i = 0; i < mEllipsoidList.length(); i++ )
{
if ( mEllipsoidList.at( i ).acronym.compare( currentEllipsoid, Qt::CaseInsensitive ) == 0 )
if ( mEllipsoidList.at( i ).acronym.compare( ellipsoidAcronym, Qt::CaseInsensitive ) == 0 )
{
index = i;
break;
@@ -57,6 +57,11 @@ class APP_EXPORT QgsProjectProperties : public QgsOptionsDialogBase, private Ui:
QString title() const;
void title( QString const &title );

/**
* Sets the \a crs shown as selected within the dialog.
*/
void setSelectedCrs( const QgsCoordinateReferenceSystem &crs );

public slots:

/**
@@ -162,18 +167,10 @@ class APP_EXPORT QgsProjectProperties : public QgsOptionsDialogBase, private Ui:
*/
void cbxWCSPubliedStateChanged( int aIdx );

/**
* If user changes the CRS, set the corresponding map units
*/
void srIdUpdated();

/* Update ComboBox accorindg to the selected new index
* Also sets the new selected Ellipsoid. */
void updateEllipsoidUI( int newIndex );

//! sets the right ellipsoid for measuring (from settings)
void projectionSelectorInitialized();

void mButtonAddColor_clicked();

signals:
@@ -182,6 +179,11 @@ class APP_EXPORT QgsProjectProperties : public QgsOptionsDialogBase, private Ui:

private:

/**
* Called when the user sets a CRS for the project.
*/
void crsChanged( const QgsCoordinateReferenceSystem &crs );

//! Formats for displaying coordinates
enum CoordinateFormat
{
@@ -222,6 +224,7 @@ class APP_EXPORT QgsProjectProperties : public QgsOptionsDialogBase, private Ui:
};
QList<EllipsoidDefs> mEllipsoidList;
int mEllipsoidIndex;
bool mBlockCrsUpdates = false;

//! populate WMTS tree
void populateWmtsTree( const QgsLayerTreeGroup *treeGroup, QgsTreeWidgetItem *treeItem );
@@ -233,6 +236,8 @@ class APP_EXPORT QgsProjectProperties : public QgsOptionsDialogBase, private Ui:
//! Populates list with ellipsoids from Sqlite3 db
void populateEllipsoidList();

void setCurrentEllipsoid( const QString &ellipsoidAcronym );

//! Create a new scale item and add it to the list of scales
QListWidgetItem *addScaleToScaleList( const QString &newScale );

@@ -244,4 +249,6 @@ class APP_EXPORT QgsProjectProperties : public QgsOptionsDialogBase, private Ui:
void updateGuiForMapUnits();

void showHelp();

friend class TestQgsProjectProperties;
};
@@ -39,6 +39,7 @@ class TestQgsProjectProperties : public QObject

void testProjectPropertiesDirty();
void testEllipsoidChange();
void testEllipsoidCrsSync();

private:
QgisApp *mQgisApp = nullptr;
@@ -148,6 +149,70 @@ void TestQgsProjectProperties::testEllipsoidChange()

}

void TestQgsProjectProperties::testEllipsoidCrsSync()
{
// test logic around syncing ellipsoid choice to project CRS

QgsProject::instance()->clear();

// if project has a crs and ellipsoid is none, then ellipsoid should not be changed when project crs is changed
QgsProject::instance()->setCrs( QgsCoordinateReferenceSystem( QStringLiteral( "EPSG:4326" ) ) );
QCOMPARE( QgsProject::instance()->ellipsoid(), QStringLiteral( "NONE" ) );

std::unique_ptr< QgsProjectProperties > pp = qgis::make_unique< QgsProjectProperties >( mQgisApp->mapCanvas() );
pp->setSelectedCrs( QgsCoordinateReferenceSystem( QStringLiteral( "EPSG:3111" ) ) );
pp->apply();
pp.reset();
QCOMPARE( QgsProject::instance()->crs().authid(), QStringLiteral( "EPSG:3111" ) );
// ellipsoid must remain not set
QCOMPARE( QgsProject::instance()->ellipsoid(), QStringLiteral( "NONE" ) );

// if ellipsoid is not set to none, then it should always be synced with the project crs choice
QCOMPARE( QgsProject::instance()->ellipsoid(), QStringLiteral( "NONE" ) );
pp = qgis::make_unique< QgsProjectProperties >( mQgisApp->mapCanvas() );
pp->setSelectedCrs( QgsCoordinateReferenceSystem( QStringLiteral( "EPSG:3111" ) ) );
pp->apply();
pp.reset();
// ellipsoid must remain not set
QCOMPARE( QgsProject::instance()->ellipsoid(), QStringLiteral( "NONE" ) );

// but if ellipsoid is initially set, then changing the project CRS should update the ellipsoid to match
QgsProject::instance()->setEllipsoid( QStringLiteral( "EPSG:7021" ) );
pp = qgis::make_unique< QgsProjectProperties >( mQgisApp->mapCanvas() );
pp->setSelectedCrs( QgsCoordinateReferenceSystem( QStringLiteral( "EPSG:3111" ) ) );
pp->apply();
pp.reset();
// ellipsoid should be updated to match CRS ellipsoid
QCOMPARE( QgsProject::instance()->ellipsoid(), QStringLiteral( "EPSG:7019" ) );

pp = qgis::make_unique< QgsProjectProperties >( mQgisApp->mapCanvas() );
pp->setSelectedCrs( QgsCoordinateReferenceSystem( QStringLiteral( "EPSG:4240" ) ) );
pp->apply();
pp.reset();
QCOMPARE( QgsProject::instance()->ellipsoid(), QStringLiteral( "EPSG:7015" ) );

// try creating a crs from a non-standard WKT string (in this case, the invalid WKT definition of EPSG:31370 used by
// some ArcGIS versions: see https://github.com/OSGeo/PROJ/issues/1781
const QString wkt = QStringLiteral( R"""(PROJCS["Belge 1972 / Belgian Lambert 72",GEOGCS["Belge 1972",DATUM["Reseau_National_Belge_1972",SPHEROID["International 1924",6378388,297],AUTHORITY["EPSG","6313"]],PRIMEM["Greenwich",0],UNIT["Degree",0.0174532925199433]],PROJECTION["Lambert_Conformal_Conic_2SP"],PARAMETER["latitude_of_origin",90],PARAMETER["central_meridian",4.36748666666667],PARAMETER["standard_parallel_1",49.8333339],PARAMETER["standard_parallel_2",51.1666672333333],PARAMETER["false_easting",150000.01256],PARAMETER["false_northing",5400088.4378],UNIT["metre",1,AUTHORITY["EPSG","9001"]],AXIS["Easting",EAST],AXIS["Northing",NORTH]])""" );
QgsCoordinateReferenceSystem customCrs = QgsCoordinateReferenceSystem::fromWkt( wkt );
pp = qgis::make_unique< QgsProjectProperties >( mQgisApp->mapCanvas() );
pp->setSelectedCrs( customCrs );
pp->apply();
pp.reset();
QCOMPARE( QgsProject::instance()->ellipsoid().left( 30 ), QStringLiteral( "PARAMETER:6378388:6356911.9461" ) );

// ok. Next bit of logic -- if the project is initially set to NO projection and NO ellipsoid, then first setting the project CRS should set an ellipsoid to match
QgsProject::instance()->setCrs( QgsCoordinateReferenceSystem() );
QgsProject::instance()->setEllipsoid( QStringLiteral( "NONE" ) );

pp = qgis::make_unique< QgsProjectProperties >( mQgisApp->mapCanvas() );
pp->setSelectedCrs( QgsCoordinateReferenceSystem( QStringLiteral( "EPSG:3111" ) ) );
pp->apply();
pp.reset();
// ellipsoid should be updated to match CRS ellipsoid
QCOMPARE( QgsProject::instance()->ellipsoid(), QStringLiteral( "EPSG:7019" ) );
}

QGSTEST_MAIN( TestQgsProjectProperties )

#include "testqgsprojectproperties.moc"

0 comments on commit 90d7f5b

Please sign in to comment.
You can’t perform that action at this time.