Apply suggestions from code review

qgis · Jan 10, 2022 · 4b8485f · 4b8485f
1 parent 21b65e0
commit 4b8485f
Show file tree

Hide file tree

Showing 2 changed files with 19 additions and 12 deletions.
diff --git a/src/app/labeling/qgsmaptoolmovelabel.cpp b/src/app/labeling/qgsmaptoolmovelabel.cpp
@@ -187,7 +187,11 @@ void QgsMapToolMoveLabel::cadCanvasPressEvent( QgsMapMouseEvent *e )
       if ( !mCurrentLabel.pos.isDiagram && !labelMoveable( vlayer, mCurrentLabel.settings, xCol, yCol, pointCol ) )
       {
         if ( mCurrentLabel.settings.dataDefinedProperties().isActive( QgsPalLayerSettings::PositionPoint ) )
-          mCurrentLabel.settings.dataDefinedProperties().property( QgsPalLayerSettings::PositionPoint ).setActive( false );
+        {
+          // Point position is defined as a read only expression (not pointing to a writable geometry column)
+          QgisApp::instance()->messageBar()->pushWarning( tr( "Move Label" ), tr( "The point position expression is not pointing to a writable geometry column" ) );
+          return;
+        }
 
         QgsPalIndexes indexes;
         if ( createAuxiliaryFields( indexes ) )
@@ -223,18 +227,21 @@ void QgsMapToolMoveLabel::cadCanvasPressEvent( QgsMapMouseEvent *e )
         yCol = indexes[ QgsDiagramLayerSettings::PositionY ];
       }
 
-      const bool usesAuxFields = vlayer->fields().fieldOrigin( xCol ) == QgsFields::OriginJoin
-                                 && vlayer->fields().fieldOrigin( yCol ) == QgsFields::OriginJoin;
-      if ( !usesAuxFields && !vlayer->isEditable() )
+      if ( xCol >= 0 && yCol >= 0 )
       {
-        if ( vlayer->startEditing() )
-        {
-          QgisApp::instance()->messageBar()->pushInfo( tr( "Move Label" ), tr( "Layer “%1” was made editable" ).arg( vlayer->name() ) );
-        }
-        else
+        const bool usesAuxFields = vlayer->fields().fieldOrigin( xCol ) == QgsFields::OriginJoin
+                                   && vlayer->fields().fieldOrigin( yCol ) == QgsFields::OriginJoin;
+        if ( !usesAuxFields && !vlayer->isEditable() )
         {
-          QgisApp::instance()->messageBar()->pushWarning( tr( "Move Label" ), tr( "Cannot move “%1” — the layer “%2” could not be made editable" ).arg( mCurrentLabel.pos.labelText, vlayer->name() ) );
-          return;
+          if ( vlayer->startEditing() )
+          {
+            QgisApp::instance()->messageBar()->pushInfo( tr( "Move Label" ), tr( "Layer “%1” was made editable" ).arg( vlayer->name() ) );
+          }
+          else
+          {
+            QgisApp::instance()->messageBar()->pushWarning( tr( "Move Label" ), tr( "Cannot move “%1” — the layer “%2” could not be made editable" ).arg( mCurrentLabel.pos.labelText, vlayer->name() ) );
+            return;
+          }
         }
       }
 

diff --git a/src/core/labeling/qgspallabeling.cpp b/src/core/labeling/qgspallabeling.cpp
@@ -245,7 +245,7 @@ void QgsPalLayerSettings::initPropertyDefinitions()
     { QgsPalLayerSettings::PolygonLabelOutside, QgsPropertyDefinition( "PolygonLabelOutside", QgsPropertyDefinition::DataTypeString, QObject::tr( "Label outside polygons" ),  QObject::tr( "string " ) + "[<b>yes</b> (allow placing outside)|<b>no</b> (never place outside)|<b>force</b> (always place outside)]", origin ) },
     { QgsPalLayerSettings::PositionX, QgsPropertyDefinition( "PositionX", QObject::tr( "Position (X)" ), QgsPropertyDefinition::Double, origin ) },
     { QgsPalLayerSettings::PositionY, QgsPropertyDefinition( "PositionY", QObject::tr( "Position (Y)" ), QgsPropertyDefinition::Double, origin ) },
-    { QgsPalLayerSettings::PositionPoint, QgsPropertyDefinition( "PositionPoint", QObject::tr( "Position (point)" ), QgsPropertyDefinition::Point, origin ) },
+    { QgsPalLayerSettings::PositionPoint, QgsPropertyDefinition( "PositionPoint", QgsPropertyDefinition::DataTypeString, QObject::tr( "Position (point)" ), QObject::tr( "A point geometry" ), origin ) },
     { QgsPalLayerSettings::Hali, QgsPropertyDefinition( "Hali", QgsPropertyDefinition::DataTypeString, QObject::tr( "Horizontal alignment" ), QObject::tr( "string " ) + "[<b>Left</b>|<b>Center</b>|<b>Right</b>]", origin ) },
     {
       QgsPalLayerSettings::Vali, QgsPropertyDefinition( "Vali", QgsPropertyDefinition::DataTypeString, QObject::tr( "Vertical alignment" ), QObject::tr( "string " ) + QStringLiteral( "[<b>Bottom</b>|<b>Base</b>|<br>"