Fix for bug in trac ticket #134 and #131.

* (#134) If a feature is edited using the Identify tool and QgsAttributeDialog, now only user-edited values are saved to the mChangedAttributes of the layer, previously it was all values.  This now means that only those user-edited values are attempted to be committed when the user stops editing and saves (similar to if those values were edited using the Attribute Table).

* (#131) Modify QgsVectorLayer::stopEditing so that if a commit fails, the editing state is left alone (previously editing was still turned off anyway).  This should be a better fix than r5591.  I triggered the bug in #131 while testing for #134, therefore getting a 2-for-1 fix.

Some bonus features to assist people in the triggered situations described in #131 and #134:

* The Postgres Provider now reports an error to the user if a commit of changed attributes fails.  This brings it up to the same behaviour of if a commit of changed geometries fails.  Previously there was no response given on an changed attribute error, leaving the user to think the commit was successful.

* The Postgres Provider now reports the contents of the SQL used in a failed UPDATE statement.  This may help the user to pinpoint which of his edits caused the error.



git-svn-id: http://svn.osgeo.org/qgis/trunk/qgis@5694 c8812cc2-4d05-0410-92ff-de0c093fc19c

Author: morb_au

src/gui/qgsattributedialog.cpp

@@ -21,23 +21,41 @@
 #include <QSettings>
 
 QgsAttributeDialog::QgsAttributeDialog(const std::vector<QgsFeatureAttribute>* attributes)
-  : QDialog(), _settingsPath("/Windows/AttributeDialog/")
+  : QDialog(),
+    _settingsPath("/Windows/AttributeDialog/"),
+    mRowIsDirty(attributes->size(), FALSE)
 {
     restorePositionAndColumnWidth();
 
     setupUi(this);
     mTable->setRowCount(attributes->size());
 
     int index=0;
-    for(std::vector<QgsFeatureAttribute>::const_iterator it=attributes->begin();it!=attributes->end();++it)
+    for ( std::vector<QgsFeatureAttribute>::const_iterator
+            it  = attributes->begin();
+            it != attributes->end();
+          ++it)
     {
+      // set attribute name
+
       QTableWidgetItem * myFieldItem = new QTableWidgetItem((*it).fieldName());
       myFieldItem->setFlags(Qt::ItemIsSelectable | Qt::ItemIsEnabled);
       mTable->setItem(index, 0, myFieldItem);
+
+      // set attribute value
+
       QTableWidgetItem * myValueItem = new QTableWidgetItem((*it).fieldValue());
       mTable->setItem(index, 1, myValueItem);
+
       ++index;
     }
+
+    // setup the mechanism to track edited attribute values
+    // if we do it this way, only edited attributes will
+    // be attempted to be saved when the editing session stops.
+    connect(mTable, SIGNAL(cellChanged(int, int)),
+            this,   SLOT  (setAttributeValueChanged(int, int)));
+
     mTable->resizeColumnsToContents();
 }
 
@@ -48,26 +66,33 @@ QgsAttributeDialog::~QgsAttributeDialog()
 
 QString QgsAttributeDialog::value(int row)
 {
-    return mTable->item(row,1)->text();
+  return mTable->item(row,1)->text();
+}
+
+bool QgsAttributeDialog::isDirty(int row)
+{
+  return mRowIsDirty.at(row);
 }
 
 bool QgsAttributeDialog::queryAttributes(QgsFeature& f)
 {
   const std::vector<QgsFeatureAttribute> featureAttributes = f.attributeMap();
   QgsAttributeDialog attdialog(&featureAttributes);
-  if(attdialog.exec()==QDialog::Accepted)
+
+  if (attdialog.exec() == QDialog::Accepted)
+  {
+    for (int i = 0; i < featureAttributes.size(); ++i)
     {
-      for(int i=0;i<featureAttributes.size();++i)
-	{
-	  f.changeAttributeValue(featureAttributes[i].fieldName(), attdialog.value(i));
-	}
-      return true;
+      f.changeAttributeValue(featureAttributes[i].fieldName(), attdialog.value(i));
     }
+    return true;
+  }
   else
-    {
-      return false;
-    }
+  {
+    return false;
+  }
 }
+
 void QgsAttributeDialog::savePositionAndColumnWidth()
 {
   QSettings settings;
@@ -91,3 +116,15 @@ void QgsAttributeDialog::restorePositionAndColumnWidth()
   resize(ww,wh);
   move(wx,wy);
 }
+
+void QgsAttributeDialog::setAttributeValueChanged(int row, int column)
+{
+#ifdef QGISDEBUG
+        std::cout << "QgsAttributeDialog::setAttributeValueChanged: Entered with "
+          << "row " << row
+          << "column " << column
+          << "." << std::endl;
+#endif
+
+  mRowIsDirty.at(row) = TRUE;
+}

src/gui/qgsattributedialog.h

@@ -29,22 +29,38 @@ class QgsFeature;
 class QgsAttributeDialog: public QDialog, private Ui::QgsAttributeDialogBase
 {
     Q_OBJECT
-	public:
+
+  public:
     QgsAttributeDialog(const std::vector<QgsFeatureAttribute>* attributes);
+
     ~QgsAttributeDialog();
-    /**Returns the field value of a row*/
+
+    /** Returns the field value of a row */
     QString value(int row);
-    /**Opens an attribute dialog and queries the attributes for a given feature. The
+
+    /** Returns if the field value of a row was edited since this dialog opened */
+    bool isDirty(int row);
+
+    /** Opens an attribute dialog and queries the attributes for a given feature. The
      attribute values are set to the feature if the dialog is accepted.
-    Returns true if accepted and false if canceled*/
+     \retval true if accepted
+     \retval false if canceled */
     static bool queryAttributes(QgsFeature& f);
 
     // Saves and restores the size and position from the last time
     // this dialog box was used.
     void savePositionAndColumnWidth();
+
     void restorePositionAndColumnWidth();
- private:
+
+  public slots:
+    //! Slot to be called when an attribute value is edited in the table.
+    void setAttributeValueChanged(int row, int column);
+
+  private:
     QString _settingsPath;
+
+    std::vector<bool> mRowIsDirty;
 };
 
 #endif

src/gui/qgsmaptoolidentify.cpp

@@ -314,7 +314,7 @@ void QgsMapToolIdentify::identifyVectorLayer(QgsVectorLayer* layer, const QgsPoi
 
     mResults->show();
   } 
-  else
+  else // ( layer->isEditable() )
   {
     // Edit attributes 
     // TODO: what to do if more features were selected? - nearest?
@@ -324,48 +324,70 @@ void QgsMapToolIdentify::identifyVectorLayer(QgsVectorLayer* layer, const QgsPoi
 
     if ( (fet = dataProvider->getNextFeature(true)) )
     {
-      // Was already changed?
-      changed_attr_map::iterator it = changedAttributes.find(fet->featureId());
-
+      // these are the values to populate the dialog with
       std::vector < QgsFeatureAttribute > old;
+
+      // start off with list of committed attribute values
+      old = fet->attributeMap();
+
+      // Test if this feature already changed since the last commit
+
+      changed_attr_map::iterator it = changedAttributes.find(fet->featureId());
       if ( it != changedAttributes.end() )
       {
+        // Yes, this feature already has in-memory attribute changes
+
+        // go through and apply the modified-but-not-committed values
         std::map<QString,QString> oldattr = (*it).second;
-        for( std::map<QString,QString>::iterator ait = oldattr.begin(); ait!=oldattr.end(); ++ait )
+        int index=0;
+        for ( std::vector<QgsFeatureAttribute>::const_iterator
+                oldit  = old.begin();
+                oldit != old.end();
+              ++oldit)
         {
-          old.push_back ( QgsFeatureAttribute ( (*ait).first, (*ait).second ) );
+          std::map<QString,QString>::iterator ait =
+            oldattr.find( (*oldit).fieldName() );
+          if ( ait != oldattr.end() )
+          {
+            // replace the committed value with the
+            // modified-but-not-committed value
+            old[index] = QgsFeatureAttribute ( (*ait).first, (*ait).second );
+          }
+
+          ++index;
         }
       }
-      else
-      {
-        old = fet->attributeMap();
-      }
-      
+
       QApplication::restoreOverrideCursor();
-      
+
+      // Show the attribute value editing dialog
       QgsAttributeDialog ad( &old );
 
-      if ( ad.exec()==QDialog::Accepted )
+      if (ad.exec() == QDialog::Accepted)
       {
-        std::map<QString,QString> attr;
-        
-        // Do this only once rather than each pass through the loop
+
         int oldSize = old.size();
-        for(register int i= 0; i < oldSize; ++i)
+
+
+        for (int i = 0; i < oldSize; ++i)
         {
-          attr.insert ( std::make_pair( old[i].fieldName(), ad.value(i) ) );
-        }
-        
-        // Remove old if exists
-        it = changedAttributes.find(fet->featureId());
+          // only apply changed values if they were edited by the user
+          if (ad.isDirty(i))
+          {
+#ifdef QGISDEBUG
+        std::cout << "QgsMapToolIdentify::identifyVectorLayer: found an changed attribute: "
+          << old[i].fieldName().toLocal8Bit().data()
+          << " = "
+          << ad.value(i).toLocal8Bit().data()
+          << "." << std::endl;
+#endif
+            changedAttributes[ fet->featureId() ][ old[i].fieldName() ] = ad.value(i);
 
-        if ( it != changedAttributes.end() )
-        { // found
-          changedAttributes.erase ( it );
+            // propagate "dirtyness" to the layer
+            layer->setModified();
+          }
         }
 
-        changedAttributes.insert ( std::make_pair( fet->featureId(), attr ) );
-        layer->setModified();
       }
     }
     else

src/gui/qgsvectorlayer.cpp

@@ -1935,7 +1935,9 @@ void QgsVectorLayer::startEditing()
 
 void QgsVectorLayer::stopEditing()
 {
-  deleteCachedGeometries();
+  bool commitSuccessful = FALSE;
+  bool rollbackSuccessful = FALSE;
+
   if(dataProvider)
   {
     if(mModified)
@@ -1945,9 +1947,15 @@ void QgsVectorLayer::stopEditing()
 
       if(commit==0)
       {
-        if(!commitChanges())
+        commitSuccessful = commitChanges();
+
+        if(!commitSuccessful)
         {
           QMessageBox::information(0,tr("Error"),tr("Could not commit changes"),QMessageBox::Ok);
+
+          // Leave the in-memory editing state alone,
+          // to give the user a chance to enter different values
+          // and try the commit again later
         }
         else
         {
@@ -1976,22 +1984,36 @@ void QgsVectorLayer::stopEditing()
           delete tabledisplay;
           tabledisplay=0;
         }
+
+        // Force this to TRUE otherwise the user will never be able
+        // to get out of an ediitng session
+        rollbackSuccessful = TRUE;
       }
       emit editingStopped(true);
     }
-    else
+    else  // if (!mModified)
     {
       emit editingStopped(false);
     }
-    mEditable=false;
-    triggerRepaint();
-    mModified=false;
-    if(isValid())
+
+    if (
+        (commitSuccessful) ||
+        (rollbackSuccessful)
+       )
     {
-      updateItemPixmap();
-      if(mToggleEditingAction)
+      // convert state to non-editing mode
+      deleteCachedGeometries();
+
+      mEditable=false;
+      triggerRepaint();
+      mModified=false;
+      if(isValid())
       {
-        mToggleEditingAction->setChecked(false);
+        updateItemPixmap();
+        if(mToggleEditingAction)
+        {
+          mToggleEditingAction->setChecked(false);
+        }
       }
     }
   }

src/providers/postgres/qgspostgresprovider.cpp

@@ -2005,17 +2005,31 @@ bool QgsPostgresProvider::changeAttributeValues(std::map<int,std::map<QString,QS
       qWarning(sql);
 #endif
 
-      //send sql statement and do error handling
+      // s end sql statement and do error handling
+      // TODO: Make all error handling like this one
       PGresult* result=PQexec(connection, (const char *)(sql.utf8()));
-      if(result==0)
+      if (result==0)
       {
-        returnvalue=false;
-        ExecStatusType message=PQresultStatus(result);
-        if(message==PGRES_FATAL_ERROR)
-        {
-          QMessageBox::information(0,"UPDATE error",QString(PQresultErrorMessage(result)),QMessageBox::Ok);
-        }
+        QMessageBox::critical(0, tr("PostGIS error"), 
+                                 tr("An error occured contacting the PostgreSQL databse"),
+                                 QMessageBox::Ok,
+                                 Qt::NoButton);
+        return false;
+      }
+      ExecStatusType message=PQresultStatus(result);
+      if(message==PGRES_FATAL_ERROR)
+      {
+        QMessageBox::information(0, tr("PostGIS error"), 
+                                 tr("The PostgreSQL databse returned: ")
+                                   + QString(PQresultErrorMessage(result))
+                                   + "\n"
+                                   + tr("When trying: ")
+                                   + sql,
+                                 QMessageBox::Ok,
+                                 Qt::NoButton);
+        return false;
       }
+
     }
   }
   PQexec(connection,"COMMIT");
@@ -2153,7 +2167,10 @@ bool QgsPostgresProvider::changeGeometryValues(std::map<int, QgsGeometry> & geom
       {
         QMessageBox::information(0, tr("PostGIS error"), 
                                  tr("The PostgreSQL databse returned: ")
-                                   + QString(PQresultErrorMessage(result)),
+                                   + QString(PQresultErrorMessage(result))
+                                   + "\n"
+                                   + tr("When trying: ")
+                                   + sql,
                                  QMessageBox::Ok,
                                  Qt::NoButton);
         return false;