From 2973735a6bdbb158336c8968fb4de794fb00f81c Mon Sep 17 00:00:00 2001 From: Damiano Lombardi Date: Mon, 27 Dec 2021 19:44:30 +0100 Subject: [PATCH] Apply suggestions from code review --- .../auto_generated/qgstextformatwidget.sip.in | 5 +- src/app/labeling/qgsmaptoollabel.cpp | 81 +------ src/app/labeling/qgsmaptoollabel.h | 6 - src/app/labeling/qgsmaptoolmovelabel.cpp | 67 +++--- src/core/labeling/qgspallabeling.cpp | 82 +++---- src/gui/labeling/qgslabelinggui.cpp | 2 - src/gui/qgspropertyoverridebutton.cpp | 5 +- src/gui/qgstextformatwidget.cpp | 16 +- src/gui/qgstextformatwidget.h | 5 +- src/ui/qgstextformatwidgetbase.ui | 214 +++++++++--------- 10 files changed, 209 insertions(+), 274 deletions(-) diff --git a/python/gui/auto_generated/qgstextformatwidget.sip.in b/python/gui/auto_generated/qgstextformatwidget.sip.in index f83aaa365c1..fded8bb2cc9 100644 --- a/python/gui/auto_generated/qgstextformatwidget.sip.in +++ b/python/gui/auto_generated/qgstextformatwidget.sip.in @@ -145,9 +145,12 @@ Sets the background color for the text preview widget. :param color: background color %End - void updateDataDefinedAlignment(); + void enableDataDefinedAlignment( bool enable ) /Deprecated/; %Docstring Update the enabled state of the data defined alignment buttons. + +.. deprecated:: + QGIS 3.24 %End virtual QgsExpressionContext createExpressionContext() const; diff --git a/src/app/labeling/qgsmaptoollabel.cpp b/src/app/labeling/qgsmaptoollabel.cpp index a6f4deeb9ff..179593bcd25 100644 --- a/src/app/labeling/qgsmaptoollabel.cpp +++ b/src/app/labeling/qgsmaptoollabel.cpp @@ -36,6 +36,7 @@ #include "qgsstatusbar.h" #include "qgslabelingresults.h" #include "qgsexpressionnodeimpl.h" +#include "qgsreferencedgeometry.h" #include @@ -632,50 +633,8 @@ QVariant QgsMapToolLabel::evaluateDataDefinedProperty( QgsPalLayerSettings::Prop bool QgsMapToolLabel::currentLabelDataDefinedPosition( double &x, bool &xSuccess, double &y, bool &ySuccess, int &xCol, int &yCol ) const { - QgsVectorLayer *vlayer = mCurrentLabel.layer; - QgsFeatureId featureId = mCurrentLabel.pos.featureId; - - xSuccess = false; - ySuccess = false; - - if ( !vlayer ) - { - return false; - } - - if ( mCurrentLabel.pos.isDiagram ) - { - if ( !diagramMoveable( vlayer, xCol, yCol ) ) - { - return false; - } - } - else if ( !labelMoveable( vlayer, mCurrentLabel.settings, xCol, yCol ) ) - { - return false; - } - - QgsFeature f; - if ( !vlayer->getFeatures( QgsFeatureRequest().setFilterFid( featureId ).setFlags( QgsFeatureRequest::NoGeometry ) ).nextFeature( f ) ) - { - return false; - } - - if ( mCurrentLabel.pos.isUnplaced ) - { - xSuccess = false; - ySuccess = false; - } - else - { - QgsAttributes attributes = f.attributes(); - if ( !attributes.at( xCol ).isNull() ) - x = attributes.at( xCol ).toDouble( &xSuccess ); - if ( !attributes.at( yCol ).isNull() ) - y = attributes.at( yCol ).toDouble( &ySuccess ); - } - - return true; + int pointCol = -1; + return currentLabelDataDefinedPosition( x, xSuccess, y, ySuccess, xCol, yCol, pointCol ); } bool QgsMapToolLabel::currentLabelDataDefinedPosition( double &x, bool &xSuccess, double &y, bool &ySuccess, int &xCol, int &yCol, int &pointCol ) const @@ -715,30 +674,18 @@ bool QgsMapToolLabel::currentLabelDataDefinedPosition( double &x, bool &xSuccess if ( mCurrentLabel.settings.dataDefinedProperties().isActive( QgsPalLayerSettings::PositionPoint ) ) { - if ( !attributes.at( pointCol ).isNull() ) + if ( pointCol >= 0 + && !attributes.at( pointCol ).isNull() ) { QVariant pointAsVariant = attributes.at( pointCol ); if ( pointAsVariant.canConvert() ) { - const QgsGeometry geometry = pointAsVariant.value(); - if ( const QgsPoint *point = ( geometry.constGet() ? qgsgeometry_cast( geometry.constGet()->simplifiedTypeRef() ) : nullptr ) ) - { + const QgsGeometry geometry = pointAsVariant.value(); + if ( const QgsPoint *point = ( geometry.constGet() ? qgsgeometry_cast( geometry.constGet()->simplifiedTypeRef() ) : nullptr ) ) + { x = point->x(); y = point->y(); - xSuccess = true; - ySuccess = true; - } - } - else if ( !pointAsVariant.toByteArray().isEmpty() ) - { - QgsPoint point; - QgsConstWkbPtr wkbPtr( pointAsVariant.toByteArray() ); - if ( point.fromWkb( wkbPtr ) ) - { - x = point.x(); - y = point.y(); - xSuccess = true; ySuccess = true; } @@ -809,7 +756,7 @@ bool QgsMapToolLabel::changeCurrentLabelDataDefinedPosition( const QVariant &x, QString pointColName = dataDefinedColumnName( QgsPalLayerSettings::PositionPoint, mCurrentLabel.settings, mCurrentLabel.layer ); int pointCol = mCurrentLabel.layer->fields().lookupField( pointColName ); - if ( !mCurrentLabel.layer->changeAttributeValue( mCurrentLabel.pos.featureId, pointCol, QgsPoint( x.toDouble(), y.toDouble() ).asWkt() ) ) + if ( !mCurrentLabel.layer->changeAttributeValue( mCurrentLabel.pos.featureId, pointCol, QVariant::fromValue( QgsReferencedGeometry( QgsGeometry::fromPointXY( QgsPoint( x.toDouble(), y.toDouble() ) ), mCurrentLabel.layer->crs() ) ) ) ) return false; } else @@ -905,12 +852,8 @@ bool QgsMapToolLabel::labelMoveable( QgsVectorLayer *vlayer, int &xCol, int &yCo bool QgsMapToolLabel::labelMoveable( QgsVectorLayer *vlayer, const QgsPalLayerSettings &settings, int &xCol, int &yCol ) const { - QString xColName = dataDefinedColumnName( QgsPalLayerSettings::PositionX, settings, vlayer ); - QString yColName = dataDefinedColumnName( QgsPalLayerSettings::PositionY, settings, vlayer ); - //return !xColName.isEmpty() && !yColName.isEmpty(); - xCol = vlayer->fields().lookupField( xColName ); - yCol = vlayer->fields().lookupField( yColName ); - return ( xCol != -1 && yCol != -1 ); + int pointCol = -1; + return labelMoveable( vlayer, settings, xCol, yCol, pointCol ); } bool QgsMapToolLabel::layerCanPin( QgsVectorLayer *vlayer, int &xCol, int &yCol ) const @@ -1071,7 +1014,7 @@ bool QgsMapToolLabel::createAuxiliaryFields( LabelDetails &details, QgsPalIndexe { index = vlayer->fields().lookupField( prop.field() ); } - else if ( prop.propertyType() != QgsProperty::ExpressionBasedProperty ) + else { index = QgsAuxiliaryLayer::createProperty( p, vlayer, false ); changed = true; diff --git a/src/app/labeling/qgsmaptoollabel.h b/src/app/labeling/qgsmaptoollabel.h index b64f410ffd0..e14b4ba560d 100644 --- a/src/app/labeling/qgsmaptoollabel.h +++ b/src/app/labeling/qgsmaptoollabel.h @@ -83,12 +83,6 @@ class APP_EXPORT QgsMapToolLabel: public QgsMapToolAdvancedDigitizing */ bool labelCanShowHide( QgsVectorLayer *vlayer, int &showCol ) const; - /** - * Checks if labels in a layer can be rotated - * \param rotationCol out: attribute column for data defined label rotation - */ - bool layerIsRotatable( QgsVectorLayer *vlayer, int &rotationCol ) const; - /** * Checks if labels in a layer can be rotated * \param rotationCol out: attribute column for data defined label rotation diff --git a/src/app/labeling/qgsmaptoolmovelabel.cpp b/src/app/labeling/qgsmaptoolmovelabel.cpp index 8d2f4e93360..023ef314e48 100644 --- a/src/app/labeling/qgsmaptoolmovelabel.cpp +++ b/src/app/labeling/qgsmaptoolmovelabel.cpp @@ -187,48 +187,30 @@ 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 ); + + mPalProperties.clear(); + mPalProperties << QgsPalLayerSettings::PositionX; + mPalProperties << QgsPalLayerSettings::PositionY; + QgsPalIndexes indexes; + if ( createAuxiliaryFields( indexes ) ) + return; + + if ( !labelMoveable( vlayer, mCurrentLabel.settings, xCol, yCol ) ) { - mPalProperties.clear(); - mPalProperties << QgsPalLayerSettings::PositionPoint; - QgsPalIndexes indexes; - if ( createAuxiliaryFields( indexes ) ) - return; - - if ( !labelMoveable( vlayer, mCurrentLabel.settings, xCol, yCol, pointCol ) ) - { - QString pointColName = dataDefinedColumnName( QgsPalLayerSettings::PositionPoint, mCurrentLabel.settings, vlayer ); - if ( pointCol < 0 ) - QgisApp::instance()->messageBar()->pushWarning( tr( "Move Label" ), tr( "The label Point column “%1” does not exist in the layer" ).arg( pointColName ) ); - return; - } - - pointCol = indexes[ QgsPalLayerSettings::PositionPoint ]; + QString xColName = dataDefinedColumnName( QgsPalLayerSettings::PositionX, mCurrentLabel.settings, vlayer ); + QString yColName = dataDefinedColumnName( QgsPalLayerSettings::PositionY, mCurrentLabel.settings, vlayer ); + if ( xCol < 0 && yCol < 0 ) + QgisApp::instance()->messageBar()->pushWarning( tr( "Move Label" ), tr( "The label X/Y columns “%1” and “%2” do not exist in the layer" ).arg( xColName, yColName ) ); + else if ( xCol < 0 ) + QgisApp::instance()->messageBar()->pushWarning( tr( "Move Label" ), tr( "The label X column “%1” does not exist in the layer" ).arg( xColName ) ); + else if ( yCol < 0 ) + QgisApp::instance()->messageBar()->pushWarning( tr( "Move Label" ), tr( "The label Y column “%1” does not exist in the layer" ).arg( yColName ) ); + return; } - else - { - mPalProperties.clear(); - mPalProperties << QgsPalLayerSettings::PositionX; - mPalProperties << QgsPalLayerSettings::PositionY; - QgsPalIndexes indexes; - if ( createAuxiliaryFields( indexes ) ) - return; - if ( !labelMoveable( vlayer, mCurrentLabel.settings, xCol, yCol ) ) - { - QString xColName = dataDefinedColumnName( QgsPalLayerSettings::PositionX, mCurrentLabel.settings, vlayer ); - QString yColName = dataDefinedColumnName( QgsPalLayerSettings::PositionY, mCurrentLabel.settings, vlayer ); - if ( xCol < 0 && yCol < 0 ) - QgisApp::instance()->messageBar()->pushWarning( tr( "Move Label" ), tr( "The label X/Y columns “%1” and “%2” do not exist in the layer" ).arg( xColName, yColName ) ); - else if ( xCol < 0 ) - QgisApp::instance()->messageBar()->pushWarning( tr( "Move Label" ), tr( "The label X column “%1” does not exist in the layer" ).arg( xColName ) ); - else if ( yCol < 0 ) - QgisApp::instance()->messageBar()->pushWarning( tr( "Move Label" ), tr( "The label Y column “%1” does not exist in the layer" ).arg( yColName ) ); - return; - } - - xCol = indexes[ QgsPalLayerSettings::PositionX ]; - yCol = indexes[ QgsPalLayerSettings::PositionY ]; - } + xCol = indexes[ QgsPalLayerSettings::PositionX ]; + yCol = indexes[ QgsPalLayerSettings::PositionY ]; } else if ( mCurrentLabel.pos.isDiagram && !diagramMoveable( vlayer, xCol, yCol ) ) { @@ -244,8 +226,11 @@ void QgsMapToolMoveLabel::cadCanvasPressEvent( QgsMapMouseEvent *e ) yCol = indexes[ QgsDiagramLayerSettings::PositionY ]; } - bool usesAuxFields = vlayer->fields().fieldOrigin( xCol ) == QgsFields::OriginJoin - && vlayer->fields().fieldOrigin( yCol ) == QgsFields::OriginJoin; + bool usesAuxFields = false; + if ( xCol >= 0 && yCol >= 0 ) + usesAuxFields = vlayer->fields().fieldOrigin( xCol ) == QgsFields::OriginJoin + && vlayer->fields().fieldOrigin( yCol ) == QgsFields::OriginJoin; + if ( mCurrentLabel.settings.dataDefinedProperties().isActive( QgsPalLayerSettings::PositionPoint ) && !mCurrentLabel.pos.isDiagram ) usesAuxFields = vlayer->fields().fieldOrigin( pointCol ) == QgsFields::OriginJoin; diff --git a/src/core/labeling/qgspallabeling.cpp b/src/core/labeling/qgspallabeling.cpp index 65c36c37ba8..16b2a5e8a7f 100644 --- a/src/core/labeling/qgspallabeling.cpp +++ b/src/core/labeling/qgspallabeling.cpp @@ -2383,47 +2383,53 @@ std::unique_ptr QgsPalLayerSettings::registerFeatureWithDetails bool ddPosition = false; if ( mDataDefinedProperties.isActive( QgsPalLayerSettings::PositionX ) - && mDataDefinedProperties.isActive( QgsPalLayerSettings::PositionY ) - && !mDataDefinedProperties.value( QgsPalLayerSettings::PositionX, context.expressionContext() ).isNull() - && !mDataDefinedProperties.value( QgsPalLayerSettings::PositionY, context.expressionContext() ).isNull() ) + && mDataDefinedProperties.isActive( QgsPalLayerSettings::PositionY ) ) { - ddPosition = true; - - bool ddXPos = false, ddYPos = false; - xPos = mDataDefinedProperties.value( QgsPalLayerSettings::PositionX, context.expressionContext() ).toDouble( &ddXPos ); - yPos = mDataDefinedProperties.value( QgsPalLayerSettings::PositionY, context.expressionContext() ).toDouble( &ddYPos ); - if ( ddXPos && ddYPos ) - hasDataDefinedPosition = true; - } - else if ( mDataDefinedProperties.isActive( QgsPalLayerSettings::PositionPoint ) - && !mDataDefinedProperties.value( QgsPalLayerSettings::PositionPoint, context.expressionContext() ).isNull() ) - { - ddPosition = true; - - QVariant pointAsVariant = mDataDefinedProperties.value( QgsPalLayerSettings::PositionPoint, context.expressionContext() ); - QgsPoint point; - if ( pointAsVariant.canConvert() ) + const QVariant xPosProperty = mDataDefinedProperties.value( QgsPalLayerSettings::PositionX, context.expressionContext() ); + const QVariant yPosProperty = mDataDefinedProperties.value( QgsPalLayerSettings::PositionY, context.expressionContext() ); + if ( !xPosProperty.isNull() + && !yPosProperty.isNull() ) { - point = QgsPoint( pointAsVariant.value().asPoint() ); - } - else if ( pointAsVariant.canConvert() ) - { - point = QgsPoint( pointAsVariant.value().asPoint() ); - } - else if ( !pointAsVariant.toString().isEmpty() ) - { - point.fromWkt( pointAsVariant.toString() ); - } + ddPosition = true; - if ( !point.isEmpty() ) - { - hasDataDefinedPosition = true; - - xPos = point.x(); - yPos = point.y(); + bool ddXPos = false, ddYPos = false; + xPos = xPosProperty.toDouble( &ddXPos ); + yPos = yPosProperty.toDouble( &ddYPos ); + if ( ddXPos && ddYPos ) + hasDataDefinedPosition = true; } } + else if ( mDataDefinedProperties.isActive( QgsPalLayerSettings::PositionPoint ) ) + { + const QVariant pointPosProperty = mDataDefinedProperties.value( QgsPalLayerSettings::PositionPoint, context.expressionContext() ); + if ( !pointPosProperty.isNull() ) + { + ddPosition = true; + QgsPoint point; + if ( pointPosProperty.canConvert() ) + { + QgsReferencedGeometry referencedGeometryPoint = pointPosProperty.value(); + point = QgsPoint( referencedGeometryPoint.asPoint() ); + + if ( !referencedGeometryPoint.isNull() + && ct.sourceCrs() != referencedGeometryPoint.crs() ) + QgsMessageLog::logMessage( QObject::tr( "Label position geometry is not in layer coordinates reference system. Layer CRS: '%1', Geometry CRS: '%2'" ).arg( ct.sourceCrs().userFriendlyIdentifier(), referencedGeometryPoint.crs().userFriendlyIdentifier() ), QObject::tr( "Labeling" ), Qgis::Warning ); + } + else if ( pointPosProperty.canConvert() ) + { + point = QgsPoint( pointPosProperty.value().asPoint() ); + } + + if ( !point.isEmpty() ) + { + hasDataDefinedPosition = true; + + xPos = point.x(); + yPos = point.y(); + } + } + } if ( ddPosition ) { @@ -2525,12 +2531,6 @@ std::unique_ptr QgsPalLayerSettings::registerFeatureWithDetails else { anchorPosition = QgsPointXY( xPos, yPos ); - - // only rotate non-pinned OverPoint placements until other placements are supported in pal::Feature - if ( dataDefinedRotation && placement != QgsPalLayerSettings::OverPoint ) - { - angle = 0.0; - } } } } diff --git a/src/gui/labeling/qgslabelinggui.cpp b/src/gui/labeling/qgslabelinggui.cpp index 109efd63818..0f77a80ded2 100644 --- a/src/gui/labeling/qgslabelinggui.cpp +++ b/src/gui/labeling/qgslabelinggui.cpp @@ -482,8 +482,6 @@ void QgsLabelingGui::setLayer( QgsMapLayer *mapLayer ) // do this after other widgets are configured, so they can be enabled/disabled populateDataDefinedButtons(); - updateDataDefinedAlignment(); - updateUi(); // should come after data defined button setup } diff --git a/src/gui/qgspropertyoverridebutton.cpp b/src/gui/qgspropertyoverridebutton.cpp index 8b5ab09acad..294d247235c 100644 --- a/src/gui/qgspropertyoverridebutton.cpp +++ b/src/gui/qgspropertyoverridebutton.cpp @@ -685,9 +685,12 @@ void QgsPropertyOverrideButton::showExpressionDialog() if ( d.exec() == QDialog::Accepted ) { mExpressionString = d.expressionText().trimmed(); + bool active = mProperty.isActive(); mProperty.setExpressionString( mExpressionString ); mProperty.setTransformer( nullptr ); - setActivePrivate( !mExpressionString.isEmpty() ); + mProperty.setActive( !mExpressionString.isEmpty() ); + if ( mProperty.isActive() != active ) + emit activated( mProperty.isActive() ); updateSiblingWidgets( isActive() ); updateGui(); emit changed(); diff --git a/src/gui/qgstextformatwidget.cpp b/src/gui/qgstextformatwidget.cpp index e75483c3991..fe75fec39f0 100644 --- a/src/gui/qgstextformatwidget.cpp +++ b/src/gui/qgstextformatwidget.cpp @@ -813,6 +813,8 @@ void QgsTextFormatWidget::populateDataDefinedButtons() registerDataDefinedButton( mCoordAlignmentVDDBtn, QgsPalLayerSettings::Vali ); registerDataDefinedButton( mCoordRotationDDBtn, QgsPalLayerSettings::LabelRotation ); + updateDataDefinedAlignment(); + // rendering const QString ddScaleVisInfo = tr( "Value < 0 represents a scale closer than 1:1, e.g. -10 = 10:1
" "Value of 0 disables the specific limit." ); @@ -1862,6 +1864,13 @@ void QgsTextFormatWidget::updateCalloutFrameStatus() mCalloutFrame->setEnabled( mCalloutDrawDDBtn->isActive() || mCalloutsDrawCheckBox->isChecked() ); } +void QgsTextFormatWidget::updateDataDefinedAlignment() +{ + // no data defined alignment without data defined position + mCoordAlignmentFrame->setEnabled( ( mCoordXDDBtn->isActive() && mCoordYDDBtn->isActive() ) + || mCoordPointDDBtn->isActive() ); +} + void QgsTextFormatWidget::setFormatFromStyle( const QString &name, QgsStyle::StyleEntity type ) { switch ( type ) @@ -2056,13 +2065,6 @@ void QgsTextFormatWidget::showBackgroundRadius( bool show ) mShapeRadiusUnitsDDBtn->setVisible( show ); } -void QgsTextFormatWidget::updateDataDefinedAlignment() -{ - // no data defined alignment without data defined position - mCoordAlignmentFrame->setEnabled( ( mCoordXDDBtn->isActive() && mCoordYDDBtn->isActive() ) - || mCoordPointDDBtn->isActive() ); -} - QgsExpressionContext QgsTextFormatWidget::createExpressionContext() const { if ( auto *lExpressionContext = mContext.expressionContext() ) diff --git a/src/gui/qgstextformatwidget.h b/src/gui/qgstextformatwidget.h index 7015dff76be..7306243a814 100644 --- a/src/gui/qgstextformatwidget.h +++ b/src/gui/qgstextformatwidget.h @@ -155,8 +155,10 @@ class GUI_EXPORT QgsTextFormatWidget : public QWidget, public QgsExpressionConte /** * Update the enabled state of the data defined alignment buttons. + * + * \deprecated QGIS 3.24 */ - void updateDataDefinedAlignment(); + Q_DECL_DEPRECATED void enableDataDefinedAlignment( bool enable ) SIP_DEPRECATED { Q_UNUSED( enable ) } QgsExpressionContext createExpressionContext() const override; @@ -311,6 +313,7 @@ class GUI_EXPORT QgsTextFormatWidget : public QWidget, public QgsExpressionConte void updateBufferFrameStatus(); void updateShadowFrameStatus(); void updateCalloutFrameStatus(); + void updateDataDefinedAlignment(); }; diff --git a/src/ui/qgstextformatwidgetbase.ui b/src/ui/qgstextformatwidgetbase.ui index 08500f08c43..384f50120d0 100644 --- a/src/ui/qgstextformatwidgetbase.ui +++ b/src/ui/qgstextformatwidgetbase.ui @@ -5424,12 +5424,92 @@ font-style: italic; 8 - - - - Alignment - - + + + + + + … + + + + + + + + + + Uncheck to write labeling engine derived rotation on pin and NULL on unpin + + + margin-left: 12px; margin-top: 3px; + + + Preserve data rotation values + + + true + + + + + + + + + + + + 0 + 0 + + + + X + + + + + + + … + + + + + + + + 0 + 0 + + + + Y + + + + + + + … + + + + + + + Qt::Horizontal + + + + 40 + 20 + + + + + @@ -5438,14 +5518,7 @@ font-style: italic; - - - - Rotation - - - - + @@ -5516,68 +5589,15 @@ font-style: italic; - - - - - - - 0 - 0 - - - - X - - - - - - - … - - - - - - - - 0 - 0 - - - - Y - - - - - - - … - - - - - - - Qt::Horizontal - - - - 40 - 20 - - - - - - - - Point - - - + + + + Rotation + + + + + @@ -5600,35 +5620,19 @@ font-style: italic; - - - - - - … - - - - - - - - - - Uncheck to write labeling engine derived rotation on pin and NULL on unpin - - - margin-left: 12px; margin-top: 3px; - - - Preserve data rotation values - - - true - - - - + + + + Alignment + + + + + + + Point + +