Fix cannot modify diagram attribute expression (fix #15514)

Also clean up some code

src/app/qgsdiagramproperties.cpp

@@ -19,7 +19,6 @@
 #include "diagram/qgspiediagram.h"
 #include "diagram/qgstextdiagram.h"
 
-#include "qgisapp.h"
 #include "qgsproject.h"
 #include "qgsapplication.h"
 #include "qgsdiagramproperties.h"
@@ -36,18 +35,21 @@
 #include "qgsmapcanvas.h"
 #include "qgsexpressionbuilderdialog.h"
 #include "qgslogger.h"
+#include "qgisapp.h"
 
 #include <QList>
 #include <QMessageBox>
 #include <QSettings>
 
+
+
 QgsExpressionContext QgsDiagramProperties::createExpressionContext() const
 {
   QgsExpressionContext expContext;
   expContext << QgsExpressionContextUtils::globalScope()
   << QgsExpressionContextUtils::projectScope()
   << QgsExpressionContextUtils::atlasScope( nullptr )
-  << QgsExpressionContextUtils::mapSettingsScope( QgisApp::instance()->mapCanvas()->mapSettings() )
+  << QgsExpressionContextUtils::mapSettingsScope( mMapCanvas->mapSettings() )
   << QgsExpressionContextUtils::layerScope( mLayer );
 
   return expContext;
@@ -79,7 +81,7 @@ QgsDiagramProperties::QgsDiagramProperties( QgsVectorLayer* layer, QWidget* pare
   mDiagramTypeComboBox->addItem( pix, tr( "Histogram" ), DIAGRAM_NAME_HISTOGRAM );
   mDiagramTypeComboBox->blockSignals( false );
 
-  mScaleRangeWidget->setMapCanvas( QgisApp::instance()->mapCanvas() );
+  mScaleRangeWidget->setMapCanvas( mMapCanvas );
   mSizeFieldExpressionWidget->registerExpressionContextGenerator( this );
 
   mBackgroundColorButton->setColorDialogTitle( tr( "Select background color" ) );
@@ -95,6 +97,9 @@ QgsDiagramProperties::QgsDiagramProperties( QgsVectorLayer* layer, QWidget* pare
 
   mMaxValueSpinBox->setShowClearButton( false );
 
+  mDiagramAttributesTreeWidget->setItemDelegateForColumn( ColumnAttributeExpression, new EditBlockerDelegate( this ) );
+  mDiagramAttributesTreeWidget->setItemDelegateForColumn( ColumnColor, new EditBlockerDelegate( this ) );
+
   connect( mFixedSizeRadio, SIGNAL( toggled( bool ) ), this, SLOT( scalingTypeChanged() ) );
   connect( mAttributeBasedScalingRadio, SIGNAL( toggled( bool ) ), this, SLOT( scalingTypeChanged() ) );
 
@@ -172,7 +177,7 @@ QgsDiagramProperties::QgsDiagramProperties( QgsVectorLayer* layer, QWidget* pare
   mSizeFieldExpressionWidget->setLayer( mLayer );
   QgsDistanceArea myDa;
   myDa.setSourceCrs( mLayer->crs().srsid() );
-  myDa.setEllipsoidalMode( QgisApp::instance()->mapCanvas()->mapSettings().hasCrsTransformEnabled() );
+  myDa.setEllipsoidalMode( mMapCanvas->mapSettings().hasCrsTransformEnabled() );
   myDa.setEllipsoid( QgsProject::instance()->readEntry( "Measure", "/Ellipsoid", GEO_NONE ) );
   mSizeFieldExpressionWidget->setGeomCalculator( myDa );
 
@@ -183,7 +188,7 @@ QgsDiagramProperties::QgsDiagramProperties( QgsVectorLayer* layer, QWidget* pare
     QTreeWidgetItem *newItem = new QTreeWidgetItem( mAttributesTreeWidget );
     QString name = QString( "\"%1\"" ).arg( layerFields.at( idx ).name() );
     newItem->setText( 0, name );
-    newItem->setData( 0, Qt::UserRole, name );
+    newItem->setData( 0, RoleAttributeExpression, name );
     newItem->setFlags( newItem->flags() & ~Qt::ItemIsDropEnabled );
 
     mDataDefinedXComboBox->addItem( layerFields.at( idx ).name(), idx );
@@ -340,7 +345,7 @@ QgsDiagramProperties::QgsDiagramProperties( QgsVectorLayer* layer, QWidget* pare
       {
         QTreeWidgetItem *newItem = new QTreeWidgetItem( mDiagramAttributesTreeWidget );
         newItem->setText( 0, *catIt );
-        newItem->setData( 0, Qt::UserRole, *catIt );
+        newItem->setData( 0, RoleAttributeExpression, *catIt );
         newItem->setFlags( newItem->flags() & ~Qt::ItemIsDropEnabled );
         QColor col( *coIt );
         col.setAlpha( 255 );
@@ -518,7 +523,7 @@ void QgsDiagramProperties::addAttribute( QTreeWidgetItem * item )
 
   newItem->setText( 0, item->text( 0 ) );
   newItem->setText( 2, guessLegendText( item->text( 0 ) ) );
-  newItem->setData( 0, Qt::UserRole, item->data( 0, Qt::UserRole ) );
+  newItem->setData( 0, RoleAttributeExpression, item->data( 0, RoleAttributeExpression ) );
   newItem->setFlags(( newItem->flags() | Qt::ItemIsEditable ) & ~Qt::ItemIsDropEnabled );
 
   //set initial color for diagram category
@@ -567,7 +572,7 @@ void QgsDiagramProperties::on_mFindMaximumValueButton_clicked()
     QgsExpressionContext context;
     context << QgsExpressionContextUtils::globalScope()
     << QgsExpressionContextUtils::projectScope()
-    << QgsExpressionContextUtils::mapSettingsScope( QgisApp::instance()->mapCanvas()->mapSettings() )
+    << QgsExpressionContextUtils::mapSettingsScope( mMapCanvas->mapSettings() )
     << QgsExpressionContextUtils::layerScope( mLayer );
 
     exp.prepare( &context );
@@ -604,13 +609,33 @@ void QgsDiagramProperties::on_mDiagramFontButton_clicked()
 
 void QgsDiagramProperties::on_mDiagramAttributesTreeWidget_itemDoubleClicked( QTreeWidgetItem * item, int column )
 {
-  if ( column == 1 ) //change color
+  switch ( column )
   {
-    QColor newColor = QgsColorDialog::getColor( item->background( 1 ).color(), nullptr );
+    case ColumnAttributeExpression:
-    if ( newColor.isValid() )
     {
-      item->setBackground( 1, QBrush( newColor ) );
+      QString currentExpression = item->data( 0, RoleAttributeExpression ).toString();
+
+      QString newExpression = showExpressionBuilder( currentExpression );
+      if ( !newExpression.isEmpty() )
+      {
+        item->setData( 0, Qt::DisplayRole, newExpression );
+        item->setData( 0, RoleAttributeExpression, newExpression );
+      }
+      break;
     }
+
+    case ColumnColor:
+    {
+      QColor newColor = QgsColorDialog::getColor( item->background( 1 ).color(), nullptr );
+      if ( newColor.isValid() )
+      {
+        item->setBackground( 1, QBrush( newColor ) );
+      }
+      break;
+    }
+
+    case ColumnLegendText:
+      break;
   }
 }
 
@@ -713,7 +738,7 @@ void QgsDiagramProperties::apply()
     QColor color = mDiagramAttributesTreeWidget->topLevelItem( i )->background( 1 ).color();
     color.setAlpha( 255 - ds.transparency );
     categoryColors.append( color );
-    categoryAttributes.append( mDiagramAttributesTreeWidget->topLevelItem( i )->data( 0, Qt::UserRole ).toString() );
+    categoryAttributes.append( mDiagramAttributesTreeWidget->topLevelItem( i )->data( 0, RoleAttributeExpression ).toString() );
     categoryLabels.append( mDiagramAttributesTreeWidget->topLevelItem( i )->text( 2 ) );
   }
   ds.categoryColors = categoryColors;
@@ -827,10 +852,38 @@ void QgsDiagramProperties::apply()
   mLayer->setDiagramLayerSettings( dls );
 
   // refresh
-  QgisApp::instance()->markDirty();
+  QgsProject::instance()->setDirty( true );
   mLayer->triggerRepaint();
 }
 
+QString QgsDiagramProperties::showExpressionBuilder( const QString& initialExpression )
+{
+  QgsExpressionContext context;
+  context << QgsExpressionContextUtils::globalScope()
+  << QgsExpressionContextUtils::projectScope()
+  << QgsExpressionContextUtils::atlasScope( nullptr )
+  << QgsExpressionContextUtils::mapSettingsScope( mMapCanvas->mapSettings() )
+  << QgsExpressionContextUtils::layerScope( mLayer );
+
+  QgsExpressionBuilderDialog dlg( mLayer, initialExpression, this, "generic", context );
+  dlg.setWindowTitle( tr( "Expression based attribute" ) );
+
+  QgsDistanceArea myDa;
+  myDa.setSourceCrs( mLayer->crs().srsid() );
+  myDa.setEllipsoidalMode( mMapCanvas->mapSettings().hasCrsTransformEnabled() );
+  myDa.setEllipsoid( QgsProject::instance()->readEntry( "Measure", "/Ellipsoid", GEO_NONE ) );
+  dlg.setGeomCalculator( myDa );
+
+  if ( dlg.exec() == QDialog::Accepted )
+  {
+    return dlg.expressionText();
+  }
+  else
+  {
+    return QString();
+  }
+}
+
 void QgsDiagramProperties::showAddAttributeExpressionDialog()
 {
   QString expression;
@@ -840,43 +893,25 @@ void QgsDiagramProperties::showAddAttributeExpressionDialog()
     expression = selections[0]->text( 0 );
   }
 
-  QgsExpressionContext context;
+  QString newExpression = showExpressionBuilder( expression );
-  context << QgsExpressionContextUtils::globalScope()
-  << QgsExpressionContextUtils::projectScope()
-  << QgsExpressionContextUtils::atlasScope( nullptr )
-  << QgsExpressionContextUtils::mapSettingsScope( QgisApp::instance()->mapCanvas()->mapSettings() )
-  << QgsExpressionContextUtils::layerScope( mLayer );
 
-  QgsExpressionBuilderDialog dlg( mLayer, expression, this, "generic", context );
+  //Only add the expression if the user has entered some text.
-  dlg.setWindowTitle( tr( "Expression based attribute" ) );
+  if ( !newExpression.isEmpty() )
-
-  QgsDistanceArea myDa;
-  myDa.setSourceCrs( mLayer->crs().srsid() );
-  myDa.setEllipsoidalMode( QgisApp::instance()->mapCanvas()->mapSettings().hasCrsTransformEnabled() );
-  myDa.setEllipsoid( QgsProject::instance()->readEntry( "Measure", "/Ellipsoid", GEO_NONE ) );
-  dlg.setGeomCalculator( myDa );
-
-  if ( dlg.exec() == QDialog::Accepted )
   {
-    QString expression =  dlg.expressionText();
+    QTreeWidgetItem *newItem = new QTreeWidgetItem( mDiagramAttributesTreeWidget );
-    //Only add the expression if the user has entered some text.
-    if ( !expression.isEmpty() )
-    {
-      QTreeWidgetItem *newItem = new QTreeWidgetItem( mDiagramAttributesTreeWidget );
 
-      newItem->setText( 0, expression );
+    newItem->setText( 0, newExpression );
-      newItem->setText( 2, expression );
+    newItem->setText( 2, newExpression );
-      newItem->setData( 0, Qt::UserRole, expression );
+    newItem->setData( 0, RoleAttributeExpression, newExpression );
-      newItem->setFlags(( newItem->flags() | Qt::ItemIsEditable ) & ~Qt::ItemIsDropEnabled );
+    newItem->setFlags(( newItem->flags() | Qt::ItemIsEditable ) & ~Qt::ItemIsDropEnabled );
 
-      //set initial color for diagram category
+    //set initial color for diagram category
-      int red = 1 + ( int )( 255.0 * qrand() / ( RAND_MAX + 1.0 ) );
+    int red = 1 + ( int )( 255.0 * qrand() / ( RAND_MAX + 1.0 ) );
-      int green = 1 + ( int )( 255.0 * qrand() / ( RAND_MAX + 1.0 ) );
+    int green = 1 + ( int )( 255.0 * qrand() / ( RAND_MAX + 1.0 ) );
-      int blue = 1 + ( int )( 255.0 * qrand() / ( RAND_MAX + 1.0 ) );
+    int blue = 1 + ( int )( 255.0 * qrand() / ( RAND_MAX + 1.0 ) );
-      QColor randomColor( red, green, blue );
+    QColor randomColor( red, green, blue );
-      newItem->setBackground( 1, QBrush( randomColor ) );
+    newItem->setBackground( 1, QBrush( randomColor ) );
-      mDiagramAttributesTreeWidget->addTopLevelItem( newItem );
+    mDiagramAttributesTreeWidget->addTopLevelItem( newItem );
-    }
   }
   activateWindow(); // set focus back parent
 }

src/app/qgsdiagramproperties.h

@@ -20,6 +20,7 @@
 
 #include <QDialog>
 #include <ui_qgsdiagrampropertiesbase.h>
+#include <QStyledItemDelegate>
 
 class QgsVectorLayer;
 class QgsMapCanvas;
@@ -58,6 +59,21 @@ class APP_EXPORT QgsDiagramProperties : public QWidget, private Ui::QgsDiagramPr
     QgsVectorLayer* mLayer;
 
   private:
+
+    enum Columns
+    {
+      ColumnAttributeExpression = 0,
+      ColumnColor,
+      ColumnLegendText,
+    };
+
+    enum Roles
+    {
+      RoleAttributeExpression = Qt::UserRole,
+    };
+
+    QString showExpressionBuilder( const QString& initialExpression );
+
     // Keeps track of the diagram type to properly save / restore settings when the diagram type combo box is set to no diagram.
     QString mDiagramType;
     QScopedPointer< QgsMarkerSymbol > mSizeLegendSymbol;
@@ -68,4 +84,18 @@ class APP_EXPORT QgsDiagramProperties : public QWidget, private Ui::QgsDiagramPr
     QgsExpressionContext createExpressionContext() const override;
 };
 
+class EditBlockerDelegate: public QStyledItemDelegate
+{
+  public:
+    EditBlockerDelegate( QObject* parent = nullptr )
+        : QStyledItemDelegate( parent )
+    {}
+
+    virtual QWidget* createEditor( QWidget *, const QStyleOptionViewItem &, const QModelIndex & ) const override
+    {
+      return nullptr;
+    }
+};
+
+
 #endif // QGSDIAGRAMPROPERTIES_H

src/app/qgsvectorlayerproperties.cpp

@@ -236,7 +236,7 @@ QgsVectorLayerProperties::QgsVectorLayerProperties(
 
   QVBoxLayout* diagLayout = new QVBoxLayout( mDiagramFrame );
   diagLayout->setMargin( 0 );
-  diagramPropertiesDialog = new QgsDiagramProperties( mLayer, mDiagramFrame, nullptr );
+  diagramPropertiesDialog = new QgsDiagramProperties( mLayer, mDiagramFrame, QgisApp::instance()->mapCanvas() );
   diagramPropertiesDialog->layout()->setContentsMargins( -1, 0, -1, 0 );
   diagLayout->addWidget( diagramPropertiesDialog );
   mDiagramFrame->setLayout( diagLayout );