Skip to content

Commit

Permalink
Refine logic relating to syncing project ellipsoid choice to CRS sele…
Browse files Browse the repository at this point in the history 
…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
nyalldawson committed Dec 20, 2019
1 parent 1a88667 commit 90d7f5b
Show file tree
Hide file tree
Showing 3 changed files with 111 additions and 40 deletions.
63 changes: 31 additions & 32 deletions src/app/qgsprojectproperties.cpp
View file
Expand Up @@ -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 );

Expand All @@ -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();
Expand Down Expand Up @@ -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
Expand All @@ -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() );
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 ) )
Expand All @@ -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;
Expand Down
23 changes: 15 additions & 8 deletions src/app/qgsprojectproperties.h
View file
Expand Up @@ -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:

/**
Expand Down Expand Up @@ -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:
Expand All @@ -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
{
Expand Down Expand Up @@ -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 );
Expand All @@ -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 );

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

void showHelp();

friend class TestQgsProjectProperties;
};
65 changes: 65 additions & 0 deletions tests/src/app/testqgsprojectproperties.cpp
View file
Expand Up @@ -39,6 +39,7 @@ class TestQgsProjectProperties : public QObject

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

private:
QgisApp *mQgisApp = nullptr;
Expand Down Expand Up @@ -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.