Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Apply suggestions from code review
  • Loading branch information
domi4484 committed Jan 10, 2022
1 parent 34fd68f commit 2973735
Show file tree
Hide file tree
Showing 10 changed files with 205 additions and 270 deletions.
5 changes: 4 additions & 1 deletion python/gui/auto_generated/qgstextformatwidget.sip.in
Expand Up @@ -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;
Expand Down
81 changes: 12 additions & 69 deletions src/app/labeling/qgsmaptoollabel.cpp
Expand Up @@ -36,6 +36,7 @@
#include "qgsstatusbar.h"
#include "qgslabelingresults.h"
#include "qgsexpressionnodeimpl.h"
#include "qgsreferencedgeometry.h"

#include <QMouseEvent>

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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<QgsGeometry>() )
{
const QgsGeometry geometry = pointAsVariant.value<QgsGeometry>();
if ( const QgsPoint *point = ( geometry.constGet() ? qgsgeometry_cast<QgsPoint *>( geometry.constGet()->simplifiedTypeRef() ) : nullptr ) )
{
const QgsGeometry geometry = pointAsVariant.value<QgsGeometry>();
if ( const QgsPoint *point = ( geometry.constGet() ? qgsgeometry_cast<QgsPoint *>( 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;
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down
6 changes: 0 additions & 6 deletions src/app/labeling/qgsmaptoollabel.h
Expand Up @@ -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
Expand Down
65 changes: 25 additions & 40 deletions src/app/labeling/qgsmaptoolmovelabel.cpp
Expand Up @@ -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 ) )
{
mPalProperties.clear();
mPalProperties << QgsPalLayerSettings::PositionPoint;
QgsPalIndexes indexes;
if ( createAuxiliaryFields( indexes ) )
return;
mCurrentLabel.settings.dataDefinedProperties().property( QgsPalLayerSettings::PositionPoint ).setActive( false );

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;
}
mPalProperties.clear();
mPalProperties << QgsPalLayerSettings::PositionX;
mPalProperties << QgsPalLayerSettings::PositionY;
QgsPalIndexes indexes;
if ( createAuxiliaryFields( indexes ) )
return;

pointCol = indexes[ QgsPalLayerSettings::PositionPoint ];
}
else
if ( !labelMoveable( vlayer, mCurrentLabel.settings, xCol, yCol ) )
{
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 ];
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 ];
}
else if ( mCurrentLabel.pos.isDiagram && !diagramMoveable( vlayer, xCol, yCol ) )
{
Expand All @@ -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;
Expand Down
76 changes: 38 additions & 38 deletions src/core/labeling/qgspallabeling.cpp
Expand Up @@ -2383,48 +2383,54 @@ std::unique_ptr<QgsLabelFeature> 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;
const QVariant xPosProperty = mDataDefinedProperties.value( QgsPalLayerSettings::PositionX, context.expressionContext() );
const QVariant yPosProperty = mDataDefinedProperties.value( QgsPalLayerSettings::PositionY, context.expressionContext() );
if ( !xPosProperty.isNull()
&& !yPosProperty.isNull() )
{
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;
bool ddXPos = false, ddYPos = false;
xPos = xPosProperty.toDouble( &ddXPos );
yPos = yPosProperty.toDouble( &ddYPos );
if ( ddXPos && ddYPos )
hasDataDefinedPosition = true;
}
}
else if ( mDataDefinedProperties.isActive( QgsPalLayerSettings::PositionPoint )
&& !mDataDefinedProperties.value( QgsPalLayerSettings::PositionPoint, context.expressionContext() ).isNull() )
else if ( mDataDefinedProperties.isActive( QgsPalLayerSettings::PositionPoint ) )
{
ddPosition = true;

QVariant pointAsVariant = mDataDefinedProperties.value( QgsPalLayerSettings::PositionPoint, context.expressionContext() );
QgsPoint point;
if ( pointAsVariant.canConvert<QgsReferencedGeometry>() )
const QVariant pointPosProperty = mDataDefinedProperties.value( QgsPalLayerSettings::PositionPoint, context.expressionContext() );
if ( !pointPosProperty.isNull() )
{
point = QgsPoint( pointAsVariant.value<QgsReferencedGeometry>().asPoint() );
}
else if ( pointAsVariant.canConvert<QgsGeometry>() )
{
point = QgsPoint( pointAsVariant.value<QgsGeometry>().asPoint() );
}
else if ( !pointAsVariant.toString().isEmpty() )
{
point.fromWkt( pointAsVariant.toString() );
}
ddPosition = true;

if ( !point.isEmpty() )
{
hasDataDefinedPosition = true;
QgsPoint point;
if ( pointPosProperty.canConvert<QgsReferencedGeometry>() )
{
QgsReferencedGeometry referencedGeometryPoint = pointPosProperty.value<QgsReferencedGeometry>();
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<QgsGeometry>() )
{
point = QgsPoint( pointPosProperty.value<QgsGeometry>().asPoint() );
}

xPos = point.x();
yPos = point.y();
if ( !point.isEmpty() )
{
hasDataDefinedPosition = true;

xPos = point.x();
yPos = point.y();
}
}
}


if ( ddPosition )
{
//data defined position. But field values could be NULL -> positions will be generated by PAL
Expand Down Expand Up @@ -2525,12 +2531,6 @@ std::unique_ptr<QgsLabelFeature> 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;
}
}
}
}
Expand Down
2 changes: 0 additions & 2 deletions src/gui/labeling/qgslabelinggui.cpp
Expand Up @@ -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
}

Expand Down
5 changes: 4 additions & 1 deletion src/gui/qgspropertyoverridebutton.cpp
Expand Up @@ -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();
Expand Down
16 changes: 9 additions & 7 deletions src/gui/qgstextformatwidget.cpp
Expand Up @@ -813,6 +813,8 @@ void QgsTextFormatWidget::populateDataDefinedButtons()
registerDataDefinedButton( mCoordAlignmentVDDBtn, QgsPalLayerSettings::Vali );
registerDataDefinedButton( mCoordRotationDDBtn, QgsPalLayerSettings::LabelRotation );

updateDataDefinedAlignment();

// rendering
const QString ddScaleVisInfo = tr( "Value &lt; 0 represents a scale closer than 1:1, e.g. -10 = 10:1<br>"
"Value of 0 disables the specific limit." );
Expand Down Expand Up @@ -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 )
Expand Down Expand Up @@ -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() )
Expand Down

0 comments on commit 2973735

Please sign in to comment.