Apply suggestions from code review

qgis · Jan 13, 2023 · bf396b5 · bf396b5
1 parent 093f878
commit bf396b5
Show file tree

Hide file tree

Showing 4 changed files with 49 additions and 59 deletions.
diff --git a/src/app/qgsmaptoolmovefeature.cpp b/src/app/qgsmaptoolmovefeature.cpp
@@ -42,7 +42,7 @@ QgsMapToolMoveFeature::QgsMapToolMoveFeature( QgsMapCanvas *canvas, MoveMode mod
 
 QgsMapToolMoveFeature::~QgsMapToolMoveFeature()
 {
-  delete mRubberBand;
+  deleteRubberband();
 }
 
 void QgsMapToolMoveFeature::cadCanvasMoveEvent( QgsMapMouseEvent *e )
@@ -69,26 +69,17 @@ void QgsMapToolMoveFeature::cadCanvasMoveEvent( QgsMapMouseEvent *e )
       const double dx = stopPointLayerCoords.x() - startPointLayerCoords.x();
       const double dy = stopPointLayerCoords.y() - startPointLayerCoords.y();
 
-      QgsFeatureRequest request;
-      request.setFilterFids( mMovedFeatures ).setNoAttributes();
-      QgsFeatureIterator fi = vlayer->getFeatures( request );
-      QgsFeature f;
-      mRubberBand->reset( vlayer->geometryType() );
-      while ( fi.nextFeature( f ) )
-      {
-        if ( !f.hasGeometry() )
-          continue;
-
-        QgsGeometry geom = f.geometry();
-        if ( geom.translate( dx, dy ) != Qgis::GeometryOperationResult::Success )
-          continue;
+      QgsGeometry geom = mGeom;
 
-        mRubberBand->addGeometry( geom, vlayer, false );
+      if ( geom.translate( dx, dy ) == Qgis::GeometryOperationResult::Success )
+      {
+        mRubberBand->setToGeometry( geom, vlayer );
+      }
+      else
+      {
+        mRubberBand->reset( vlayer->geometryType() );
       }
     }
-
-    mRubberBand->updatePosition();
-    mRubberBand->update();
   }
 
   mSnapIndicator->setMatch( e->mapPointMatch() );
@@ -99,8 +90,7 @@ void QgsMapToolMoveFeature::cadCanvasReleaseEvent( QgsMapMouseEvent *e )
   QgsVectorLayer *vlayer = currentVectorLayer();
   if ( !vlayer || !vlayer->isEditable() )
   {
-    delete mRubberBand;
-    mRubberBand = nullptr;
+    deleteRubberband();
     mSnapIndicator->setMatch( QgsPointLocator::Match() );
     cadDockWidget()->clear();
     notifyNotEditableLayer();
@@ -154,7 +144,8 @@ void QgsMapToolMoveFeature::cadCanvasReleaseEvent( QgsMapMouseEvent *e )
       mMovedFeatures << cf.id(); //todo: take the closest feature, not the first one...
 
       mRubberBand = createRubberBand( vlayer->geometryType() );
-      mRubberBand->setToGeometry( cf.geometry(), vlayer );
+      mGeom = cf.geometry();
+      mRubberBand->setToGeometry( mGeom, vlayer );
     }
     else
     {
@@ -167,15 +158,16 @@ void QgsMapToolMoveFeature::cadCanvasReleaseEvent( QgsMapMouseEvent *e )
       bool allFeaturesInView = true;
       const QgsRectangle viewRect = mCanvas->mapSettings().mapToLayerCoordinates( vlayer, mCanvas->extent() );
 
+      QVector <QgsGeometry> selectedGeometries;
       while ( it.nextFeature( feat ) )
       {
-        mRubberBand->addGeometry( feat.geometry(), vlayer, false );
+        selectedGeometries << feat.geometry();
 
         if ( allFeaturesInView && !viewRect.intersects( feat.geometry().boundingBox() ) )
           allFeaturesInView = false;
       }
-      mRubberBand->updatePosition();
-      mRubberBand->update();
+      mGeom = QgsGeometry::collectGeometry( selectedGeometries );
+      mRubberBand->setToGeometry( mGeom, vlayer );
 
       if ( !allFeaturesInView )
       {
@@ -187,8 +179,7 @@ void QgsMapToolMoveFeature::cadCanvasReleaseEvent( QgsMapMouseEvent *e )
         if ( res != QMessageBox::Yes )
         {
           mMovedFeatures.clear();
-          delete mRubberBand;
-          mRubberBand = nullptr;
+          deleteRubberband();
           mSnapIndicator->setMatch( QgsPointLocator::Match() );
           return;
         }
@@ -204,8 +195,7 @@ void QgsMapToolMoveFeature::cadCanvasReleaseEvent( QgsMapMouseEvent *e )
     if ( e->button() != Qt::LeftButton )
     {
       cadDockWidget()->clear();
-      delete mRubberBand;
-      mRubberBand = nullptr;
+      deleteRubberband();
       mSnapIndicator->setMatch( QgsPointLocator::Match() );
       return;
     }
@@ -247,8 +237,7 @@ void QgsMapToolMoveFeature::cadCanvasReleaseEvent( QgsMapMouseEvent *e )
             vlayer->addTopologicalPoints( vlayer->getGeometry( id ) );
           }
         }
-        delete mRubberBand;
-        mRubberBand = nullptr;
+        deleteRubberband();
         mSnapIndicator->setMatch( QgsPointLocator::Match() );
         cadDockWidget()->clear();
         break;
@@ -260,8 +249,7 @@ void QgsMapToolMoveFeature::cadCanvasReleaseEvent( QgsMapMouseEvent *e )
         if ( !QgisApp::instance()->vectorLayerTools()->copyMoveFeatures( vlayer, request, dx, dy, errorMsg, QgsProject::instance()->topologicalEditing(), mSnapIndicator->match().layer() ) )
         {
           emit messageEmitted( *errorMsg, Qgis::MessageLevel::Critical );
-          delete mRubberBand;
-          mRubberBand = nullptr;
+          deleteRubberband();
           mSnapIndicator->setMatch( QgsPointLocator::Match() );
         }
         break;
@@ -274,9 +262,7 @@ void QgsMapToolMoveFeature::cadCanvasReleaseEvent( QgsMapMouseEvent *e )
 
 void QgsMapToolMoveFeature::deactivate()
 {
-  //delete rubber band
-  delete mRubberBand;
-  mRubberBand = nullptr;
+  deleteRubberband();
   mSnapIndicator->setMatch( QgsPointLocator::Match() );
 
   QgsMapToolAdvancedDigitizing::deactivate();
@@ -287,8 +273,14 @@ void QgsMapToolMoveFeature::keyReleaseEvent( QKeyEvent *e )
   if ( mRubberBand && e->key() == Qt::Key_Escape )
   {
     cadDockWidget()->clear();
-    delete mRubberBand;
-    mRubberBand = nullptr;
+    deleteRubberband();
     mSnapIndicator->setMatch( QgsPointLocator::Match() );
   }
 }
+
+void QgsMapToolMoveFeature::deleteRubberband()
+{
+  delete mRubberBand;
+  mRubberBand = nullptr;
+  mGeom = QgsGeometry();
+}
diff --git a/src/app/qgsmaptoolmovefeature.h b/src/app/qgsmaptoolmovefeature.h
@@ -48,6 +48,9 @@ class APP_EXPORT QgsMapToolMoveFeature: public QgsMapToolAdvancedDigitizing
     void keyReleaseEvent( QKeyEvent *e ) override;
 
   private:
+
+    void deleteRubberband();
+
     //! Start point of the move in map coordinates
     QgsPointXY mStartPointMapCoords;
 
@@ -57,13 +60,16 @@ class APP_EXPORT QgsMapToolMoveFeature: public QgsMapToolAdvancedDigitizing
     //! Snapping indicators
     std::unique_ptr<QgsSnapIndicator> mSnapIndicator;
 
-    //! Id of moved feature
+    //! Id of moved features
     QgsFeatureIds mMovedFeatures;
 
     QPoint mPressPos;
 
     MoveMode mMode;
 
+    // MultiGeometry of the moved features
+    QgsGeometry mGeom;
+
 };
 
 #endif
diff --git a/src/app/qgsmaptoolrotatefeature.cpp b/src/app/qgsmaptoolrotatefeature.cpp
@@ -303,7 +303,8 @@ void QgsMapToolRotateFeature::cadCanvasReleaseEvent( QgsMapMouseEvent *e )
       mRotatedFeatures << cf.id(); //todo: take the closest feature, not the first one...
 
       mRubberBand = createRubberBand( vlayer->geometryType() );
-      mRubberBand->setToGeometry( cf.geometry(), vlayer );
+      mGeom = cf.geometry();
+      mRubberBand->setToGeometry( mGeom, vlayer );
     }
     else
     {
@@ -313,12 +314,13 @@ void QgsMapToolRotateFeature::cadCanvasReleaseEvent( QgsMapMouseEvent *e )
 
       QgsFeature feat;
       QgsFeatureIterator it = vlayer->getSelectedFeatures();
+      QVector <QgsGeometry> selectedGeometries;
       while ( it.nextFeature( feat ) )
       {
-        mRubberBand->addGeometry( feat.geometry(), vlayer, false );
+        selectedGeometries << feat.geometry();
       }
-      mRubberBand->updatePosition();
-      mRubberBand->update();
+      mGeom = QgsGeometry::collectGeometry( selectedGeometries );
+      mRubberBand->setToGeometry( mGeom, vlayer );
     }
 
     mRubberBand->show();
@@ -363,7 +365,6 @@ void QgsMapToolRotateFeature::updateRubberband( double rotation )
   if ( mRotationActive )
   {
     mRotation = rotation;
-
     mStPoint = toCanvasCoordinates( mStartPointMapCoords );
 
     if ( mRubberBand )
@@ -378,28 +379,15 @@ void QgsMapToolRotateFeature::updateRubberband( double rotation )
         mRubberBand->setTransform( QTransform().translate( offsetX, offsetY ).rotate( mRotation ).translate( -1 * offsetX, -1 * offsetY ) );
         mRubberBand->update();
       }
-
       // Else, recreate the rubber band from the rotated geometries
       else
       {
-        mRubberBand->reset( vlayer->geometryType() );
         const QgsPointXY anchorPoint = toLayerCoordinates( vlayer, mStartPointMapCoords );
-        QgsFeatureRequest request;
-        request.setFilterFids( mRotatedFeatures ).setNoAttributes();
-        QgsFeatureIterator fi = vlayer->getFeatures( request );
-        QgsFeature f;
-        while ( fi.nextFeature( f ) )
-        {
-          QgsGeometry geom = f.geometry();
-          geom.rotate( mRotation, anchorPoint );
-          mRubberBand->addGeometry( geom, vlayer, false );
-        }
-        mRubberBand->updatePosition();
+        QgsGeometry geom = mGeom;
+        geom.rotate( mRotation, anchorPoint );
+        mRubberBand->setToGeometry( geom, vlayer );
       }
-
-      mRubberBand->update();
     }
-
   }
 }
 
@@ -487,6 +475,7 @@ void QgsMapToolRotateFeature::deleteRubberband()
 {
   delete mRubberBand;
   mRubberBand = nullptr;
+  mGeom  = QgsGeometry();
 }
 
 void QgsMapToolRotateFeature::deactivate()

diff --git a/src/app/qgsmaptoolrotatefeature.h b/src/app/qgsmaptoolrotatefeature.h
@@ -122,6 +122,9 @@ class APP_EXPORT QgsMapToolRotateFeature: public QgsMapToolAdvancedDigitizing
 
     //! Shows current angle value and allows numerical editing
     QgsAngleMagnetWidget *mRotationWidget = nullptr;
+
+    // MultiGeometry of the features being rotated
+    QgsGeometry mGeom;
 };
 
 #endif