Fix memory leaks and make gcp container class safer to use

qgis · Feb 14, 2022 · 16e9f91 · 16e9f91
1 parent 90febde
commit 16e9f91
Show file tree

Hide file tree

Showing 8 changed files with 154 additions and 39 deletions.
diff --git a/src/app/georeferencer/qgsgcplist.cpp b/src/app/georeferencer/qgsgcplist.cpp
@@ -21,18 +21,6 @@
 
 #include "qgsgcplist.h"
 
-QgsGCPList::QgsGCPList( const QgsGCPList &list )
-  :  QList<QgsGeorefDataPoint *>()
-{
-  clear();
-  QgsGCPList::const_iterator it = list.constBegin();
-  for ( ; it != list.constEnd(); ++it )
-  {
-    QgsGeorefDataPoint *pt = new QgsGeorefDataPoint( **it );
-    append( pt );
-  }
-}
-
 void QgsGCPList::createGCPVectors( QVector<QgsPointXY> &sourceCoordinates, QVector<QgsPointXY> &destinationCoordinates, const QgsCoordinateReferenceSystem &targetCrs )
 {
   const int targetSize = countEnabledPoints();
@@ -82,14 +70,13 @@ int QgsGCPList::countEnabledPoints() const
   return s;
 }
 
-QgsGCPList &QgsGCPList::operator =( const QgsGCPList &list )
+QList<QgsGcpPoint> QgsGCPList::asPoints() const
 {
-  clear();
-  QgsGCPList::const_iterator it = list.constBegin();
-  for ( ; it != list.constEnd(); ++it )
+  QList<QgsGcpPoint> res;
+  res.reserve( size() );
+  for ( QgsGeorefDataPoint *pt : *this )
   {
-    QgsGeorefDataPoint *pt = new QgsGeorefDataPoint( **it );
-    append( pt );
+    res.append( QgsGcpPoint( pt->point() ) );
   }
-  return *this;
+  return res;
 }
diff --git a/src/app/georeferencer/qgsgcplist.h b/src/app/georeferencer/qgsgcplist.h
@@ -18,21 +18,24 @@
 
 #include <QList>
 #include <QVector>
+#include "qgis_app.h"
 
 class QgsGeorefDataPoint;
+class QgsGcpPoint;
 class QgsPointXY;
 class QgsCoordinateReferenceSystem;
 
 /**
  * A container for GCP data points.
  *
- * The container does NOT own the points!
+ * The container does NOT own the points -- they have to be manually deleted elsewhere!!
  */
-class QgsGCPList : public QList<QgsGeorefDataPoint *>
+class APP_EXPORT QgsGCPList : public QList<QgsGeorefDataPoint *>
 {
   public:
     QgsGCPList() = default;
-    QgsGCPList( const QgsGCPList &list );
+    QgsGCPList( const QgsGCPList &list ) = delete;
+    QgsGCPList &operator =( const QgsGCPList &list ) = delete;
 
     void createGCPVectors( QVector<QgsPointXY> &sourceCoordinates, QVector<QgsPointXY> &destinationCoordinates, const QgsCoordinateReferenceSystem &targetCrs );
 
@@ -41,7 +44,11 @@ class QgsGCPList : public QList<QgsGeorefDataPoint *>
      */
     int countEnabledPoints() const;
 
-    QgsGCPList &operator =( const QgsGCPList &list );
+    /**
+     * Returns the container as a list of GCP points.
+     */
+    QList< QgsGcpPoint > asPoints() const;
+
 };
 
 #endif
diff --git a/src/app/georeferencer/qgsgeorefdatapoint.h b/src/app/georeferencer/qgsgeorefdatapoint.h
@@ -100,6 +100,20 @@ class APP_EXPORT QgsGcpPoint
      */
     void setEnabled( bool enabled ) { mEnabled = enabled; }
 
+    // TODO c++20 - replace with = default
+    bool operator==( const QgsGcpPoint &other ) const
+    {
+      return mEnabled == other.mEnabled
+             && mSourcePoint == other.mSourcePoint
+             && mDestinationPoint == other.mDestinationPoint
+             && mDestinationCrs == other.mDestinationCrs;
+    }
+
+    bool operator!=( const QgsGcpPoint &other ) const
+    {
+      return !( *this == other );
+    }
+
   private:
 
     QgsPointXY mSourcePoint;
@@ -113,7 +127,7 @@ class APP_EXPORT QgsGcpPoint
 /**
  * Container for a GCP point and the graphical objects which represent it on the map canvas.
  */
-class QgsGeorefDataPoint : public QObject
+class APP_EXPORT QgsGeorefDataPoint : public QObject
 {
     Q_OBJECT
 
@@ -201,6 +215,8 @@ class QgsGeorefDataPoint : public QObject
      */
     QgsCoordinateReferenceSystem destinationPointCrs() const { return mGcpPoint.destinationPointCrs(); }
 
+    QgsGcpPoint point() const { return mGcpPoint; }
+
   public slots:
     void moveTo( QPoint canvasPixels, bool isMapPlugin );
     void updateCoords();

diff --git a/src/app/georeferencer/qgsgeorefmainwindow.cpp b/src/app/georeferencer/qgsgeorefmainwindow.cpp
@@ -1287,7 +1287,7 @@ bool QgsGeoreferencerMainWindow::loadGCPs( /*bool verbose*/ )
     ++i;
   }
 
-  mInitialPoints = mPoints;
+  mSavedPoints = mPoints.asPoints();
   //    showMessageInLog(tr("GCP points loaded from"), mGCPpointsFileName);
   if ( mGCPsDirty )
   {
@@ -1324,7 +1324,7 @@ void QgsGeoreferencerMainWindow::saveGCPs()
              << endl;
     }
 
-    mInitialPoints = mPoints;
+    mSavedPoints = mPoints.asPoints();
   }
   else
   {
@@ -1340,7 +1340,7 @@ QgsGeoreferencerMainWindow::SaveGCPs QgsGeoreferencerMainWindow::checkNeedGCPSav
   if ( 0 == mPoints.count() )
     return QgsGeoreferencerMainWindow::GCPDISCARD;
 
-  if ( !equalGCPlists( mInitialPoints, mPoints ) )
+  if ( !equalGCPlists( mSavedPoints, mPoints ) )
   {
     QMessageBox::StandardButton a = QMessageBox::question( this, tr( "Save GCPs" ),
                                     tr( "Save GCP points?" ),
@@ -2133,7 +2133,7 @@ bool QgsGeoreferencerMainWindow::checkFileExisting( const QString &fileName, con
   return true;
 }
 
-bool QgsGeoreferencerMainWindow::equalGCPlists( const QgsGCPList &list1, const QgsGCPList &list2 )
+bool QgsGeoreferencerMainWindow::equalGCPlists( const QList< QgsGcpPoint > &list1, const QgsGCPList &list2 )
 {
   if ( list1.count() != list2.count() )
     return false;
@@ -2142,12 +2142,9 @@ bool QgsGeoreferencerMainWindow::equalGCPlists( const QgsGCPList &list1, const Q
   int j = 0;
   for ( int i = 0; i < count; ++i, ++j )
   {
-    QgsGeorefDataPoint *p1 = list1.at( i );
-    QgsGeorefDataPoint *p2 = list2.at( j );
-    if ( p1->sourcePoint() != p2->sourcePoint() )
-      return false;
-
-    if ( p1->destinationPoint() != p2->destinationPoint() )
+    const QgsGcpPoint p1 = list1.at( i );
+    const QgsGcpPoint p2 = list2.at( j )->point();
+    if ( p1 != p2 )
       return false;
   }
 

diff --git a/src/app/georeferencer/qgsgeorefmainwindow.h b/src/app/georeferencer/qgsgeorefmainwindow.h
@@ -45,6 +45,7 @@ class QgsGeorefToolDeletePoint;
 class QgsGeorefToolMovePoint;
 class QgsGeorefToolMovePoint;
 class QgsGCPCanvasItem;
+class QgsGcpPoint;
 
 class QgsGeorefDockWidget : public QgsDockWidget
 {
@@ -193,7 +194,7 @@ class QgsGeoreferencerMainWindow : public QMainWindow, private Ui::QgsGeorefPlug
     int polynomialOrder( QgsGeorefTransform::TransformMethod transform );
     QString guessWorldFileName( const QString &rasterFileName );
     bool checkFileExisting( const QString &fileName, const QString &title, const QString &question );
-    bool equalGCPlists( const QgsGCPList &list1, const QgsGCPList &list2 );
+    bool equalGCPlists( const QList<QgsGcpPoint> &list1, const QgsGCPList &list2 );
     void logTransformOptions();
     void logRequaredGCPs();
     void clearGCPData();
@@ -245,7 +246,8 @@ class QgsGeoreferencerMainWindow : public QMainWindow, private Ui::QgsGeorefPlug
     QString mCompressionMethod;
 
     QgsGCPList mPoints;
-    QgsGCPList mInitialPoints;
+    QList< QgsGcpPoint > mSavedPoints;
+
     QgsMapCanvas *mCanvas = nullptr;
     std::unique_ptr< QgsRasterLayer > mLayer;
 

diff --git a/src/app/georeferencer/qgsresidualplotitem.cpp b/src/app/georeferencer/qgsresidualplotitem.cpp
@@ -27,6 +27,12 @@ QgsResidualPlotItem::QgsResidualPlotItem( QgsLayout *layout )
   setBackgroundEnabled( false );
 }
 
+QgsResidualPlotItem::~QgsResidualPlotItem()
+{
+  qDeleteAll( mGCPList );
+  mGCPList.clear();
+}
+
 QgsLayoutItem::Flags QgsResidualPlotItem::itemFlags() const
 {
   return QgsLayoutItem::FlagOverridesPaint;
@@ -154,6 +160,17 @@ void QgsResidualPlotItem::paint( QPainter *painter, const QStyleOptionGraphicsIt
   }
 }
 
+void QgsResidualPlotItem::setGCPList( const QgsGCPList &list )
+{
+  qDeleteAll( mGCPList );
+  mGCPList.clear();
+
+  for ( const QgsGeorefDataPoint *pt : list )
+  {
+    mGCPList.append( new QgsGeorefDataPoint( *pt ) );
+  }
+}
+
 void QgsResidualPlotItem::draw( QgsLayoutItemRenderContext & )
 {
 

diff --git a/src/app/georeferencer/qgsresidualplotitem.h b/src/app/georeferencer/qgsresidualplotitem.h
@@ -30,14 +30,15 @@ class QgsResidualPlotItem: public QgsLayoutItem
 
   public:
     explicit QgsResidualPlotItem( QgsLayout *layout );
+    ~QgsResidualPlotItem() override;
 
     QgsLayoutItem::Flags itemFlags() const override;
 
     //! \brief Reimplementation of QCanvasItem::paint
     void paint( QPainter *painter, const QStyleOptionGraphicsItem *itemStyle, QWidget *pWidget ) override;
 
-    void setGCPList( const QgsGCPList &list ) { mGCPList = list; }
-    QgsGCPList GCPList() const { return mGCPList; }
+    void setGCPList( const QgsGCPList &list );
+    const QgsGCPList &GCPList() const { return mGCPList; }
 
     void setExtent( const QgsRectangle &rect ) { mExtent = rect;}
     QgsRectangle extent() const { return mExtent; }

diff --git a/tests/src/app/testqgsgeoreferencer.cpp b/tests/src/app/testqgsgeoreferencer.cpp
@@ -25,6 +25,7 @@
 #include "qgsmapcanvas.h"
 #include "georeferencer/qgsgeoreftransform.h"
 #include "georeferencer/qgsgeorefdatapoint.h"
+#include "georeferencer/qgsgcplist.h"
 
 /**
  * \ingroup UnitTests
@@ -42,6 +43,8 @@ class TestQgsGeoreferencer : public QObject
     void init() {} // will be called before each testfunction is executed.
     void cleanup() {} // will be called after every testfunction.
     void testGcpPoint();
+    void testGeorefDataPoint();
+    void testGcpList();
     void testTransformImageNoGeoference();
     void testTransformImageWithExistingGeoreference();
     void testRasterChangeCoords();
@@ -70,7 +73,7 @@ void TestQgsGeoreferencer::cleanupTestCase()
 
 void TestQgsGeoreferencer::testGcpPoint()
 {
-  QgsGcpPoint p( QgsPointXY( 1, 2 ), QgsPointXY( 3, 4 ), QgsCoordinateReferenceSystem( "EPSG:3111" ), false );
+  QgsGcpPoint p( QgsPointXY( 1, 2 ), QgsPointXY( 3, 4 ), QgsCoordinateReferenceSystem( QStringLiteral( "EPSG:3111" ) ), false );
 
   QCOMPARE( p.sourcePoint(), QgsPointXY( 1, 2 ) );
   p.setSourcePoint( QgsPointXY( 11, 22 ) );
@@ -87,6 +90,91 @@ void TestQgsGeoreferencer::testGcpPoint()
   QVERIFY( !p.isEnabled() );
   p.setEnabled( true );
   QVERIFY( p.isEnabled() );
+
+  // equality operator
+  QVERIFY( QgsGcpPoint( QgsPointXY( 1, 2 ), QgsPointXY( 3, 4 ), QgsCoordinateReferenceSystem( QStringLiteral( "EPSG:3111" ) ), false )
+           == QgsGcpPoint( QgsPointXY( 1, 2 ), QgsPointXY( 3, 4 ), QgsCoordinateReferenceSystem( QStringLiteral( "EPSG:3111" ) ), false ) );
+  QVERIFY( QgsGcpPoint( QgsPointXY( 1, 2 ), QgsPointXY( 3, 4 ), QgsCoordinateReferenceSystem( QStringLiteral( "EPSG:3111" ) ), false )
+           != QgsGcpPoint( QgsPointXY( 11, 22 ), QgsPointXY( 3, 4 ), QgsCoordinateReferenceSystem( QStringLiteral( "EPSG:3111" ) ), false ) );
+  QVERIFY( QgsGcpPoint( QgsPointXY( 1, 2 ), QgsPointXY( 3, 4 ), QgsCoordinateReferenceSystem( QStringLiteral( "EPSG:3111" ) ), false )
+           != QgsGcpPoint( QgsPointXY( 1, 2 ), QgsPointXY( 33, 44 ), QgsCoordinateReferenceSystem( QStringLiteral( "EPSG:3111" ) ), false ) );
+  QVERIFY( QgsGcpPoint( QgsPointXY( 1, 2 ), QgsPointXY( 3, 4 ), QgsCoordinateReferenceSystem( QStringLiteral( "EPSG:3111" ) ), false )
+           != QgsGcpPoint( QgsPointXY( 1, 2 ), QgsPointXY( 3, 4 ), QgsCoordinateReferenceSystem( QStringLiteral( "EPSG:28356" ) ), false ) );
+  QVERIFY( QgsGcpPoint( QgsPointXY( 1, 2 ), QgsPointXY( 3, 4 ), QgsCoordinateReferenceSystem( QStringLiteral( "EPSG:3111" ) ), false )
+           != QgsGcpPoint( QgsPointXY( 1, 2 ), QgsPointXY( 3, 4 ), QgsCoordinateReferenceSystem( QStringLiteral( "EPSG:3111" ) ), true ) );
+
+}
+
+void TestQgsGeoreferencer::testGeorefDataPoint()
+{
+  QgsMapCanvas c1;
+  QgsMapCanvas c2;
+  QgsGeorefDataPoint p( &c1, &c2,
+                        QgsPointXY( 1, 2 ), QgsPointXY( 3, 4 ), QgsCoordinateReferenceSystem( QStringLiteral( "EPSG:3111" ) ),
+                        false );
+
+  QCOMPARE( p.sourcePoint(), QgsPointXY( 1, 2 ) );
+  p.setSourcePoint( QgsPointXY( 11, 22 ) );
+  QCOMPARE( p.sourcePoint(), QgsPointXY( 11, 22 ) );
+
+  QCOMPARE( p.destinationPoint(), QgsPointXY( 3, 4 ) );
+  p.setDestinationPoint( QgsPointXY( 33, 44 ) );
+  QCOMPARE( p.destinationPoint(), QgsPointXY( 33, 44 ) );
+
+  QCOMPARE( p.destinationPointCrs().authid(), QStringLiteral( "EPSG:3111" ) );
+
+  QVERIFY( !p.isEnabled() );
+  p.setEnabled( true );
+  QVERIFY( p.isEnabled() );
+
+  QCOMPARE( p.point(), QgsGcpPoint( QgsPointXY( 11, 22 ),
+                                    QgsPointXY( 33, 44 ),
+                                    QgsCoordinateReferenceSystem( QStringLiteral( "EPSG:3111" ) ),
+                                    true ) );
+}
+
+void TestQgsGeoreferencer::testGcpList()
+{
+  QgsGCPList list;
+  QCOMPARE( list.countEnabledPoints(), 0 );
+  QVERIFY( list.asPoints().isEmpty() );
+
+  QgsMapCanvas c1;
+  QgsMapCanvas c2;
+  list.append( new QgsGeorefDataPoint( &c1, &c2,
+                                       QgsPointXY( 1, 2 ), QgsPointXY( 3, 4 ), QgsCoordinateReferenceSystem( QStringLiteral( "EPSG:3111" ) ),
+                                       false ) );
+  QCOMPARE( list.countEnabledPoints(), 0 );
+  QCOMPARE( list.asPoints(),
+  {
+    QgsGcpPoint( QgsPointXY( 1, 2 ), QgsPointXY( 3, 4 ), QgsCoordinateReferenceSystem( QStringLiteral( "EPSG:3111" ) ), false )
+  }
+          );
+
+  list.append( new QgsGeorefDataPoint( &c1, &c2,
+                                       QgsPointXY( 11, 22 ), QgsPointXY( 33, 44 ), QgsCoordinateReferenceSystem( QStringLiteral( "EPSG:28356" ) ),
+                                       true ) );
+  QCOMPARE( list.countEnabledPoints(), 1 );
+  QCOMPARE( list.asPoints(),
+            QList< QgsGcpPoint >(
+  {
+    QgsGcpPoint( QgsPointXY( 1, 2 ), QgsPointXY( 3, 4 ), QgsCoordinateReferenceSystem( QStringLiteral( "EPSG:3111" ) ), false ),
+    QgsGcpPoint( QgsPointXY( 11, 22 ), QgsPointXY( 33, 44 ), QgsCoordinateReferenceSystem( QStringLiteral( "EPSG:28356" ) ), true )
+  } ) );
+
+  list.append( new QgsGeorefDataPoint( &c1, &c2,
+                                       QgsPointXY( 111, 222 ), QgsPointXY( 333, 444 ), QgsCoordinateReferenceSystem( QStringLiteral( "EPSG:28356" ) ),
+                                       true ) );
+  QCOMPARE( list.countEnabledPoints(), 2 );
+  QCOMPARE( list.asPoints(),
+            QList< QgsGcpPoint >(
+  {
+    QgsGcpPoint( QgsPointXY( 1, 2 ), QgsPointXY( 3, 4 ), QgsCoordinateReferenceSystem( QStringLiteral( "EPSG:3111" ) ), false ),
+    QgsGcpPoint( QgsPointXY( 11, 22 ), QgsPointXY( 33, 44 ), QgsCoordinateReferenceSystem( QStringLiteral( "EPSG:28356" ) ), true ),
+    QgsGcpPoint( QgsPointXY( 111, 222 ), QgsPointXY( 333, 444 ), QgsCoordinateReferenceSystem( QStringLiteral( "EPSG:28356" ) ), true )
+  } ) );
+
+  qDeleteAll( list );
 }
 
 void TestQgsGeoreferencer::testTransformImageNoGeoference()