Fix attr table sorting

Fix #34935
qgis · Feb 1, 2022 · acdbc56 · acdbc56
1 parent e947698
commit acdbc56
Show file tree

Hide file tree

Showing 4 changed files with 84 additions and 3 deletions.
diff --git a/src/gui/attributetable/qgsattributetablemodel.cpp b/src/gui/attributetable/qgsattributetablemodel.cpp
@@ -33,6 +33,7 @@
 #include "qgsvectordataprovider.h"
 #include "qgssymbollayerutils.h"
 #include "qgsfieldformatterregistry.h"
+#include "qgsfallbackfieldformatter.h"
 #include "qgsgui.h"
 #include "qgsexpressionnodeimpl.h"
 #include "qgsvectorlayerjoininfo.h"
@@ -235,7 +236,13 @@ void QgsAttributeTableModel::featureAdded( QgsFeatureId fid )
         QgsFieldFormatter *fieldFormatter = mFieldFormatters.at( cache.sortFieldIndex );
         const QVariant &widgetCache = mAttributeWidgetCaches.at( cache.sortFieldIndex );
         const QVariantMap &widgetConfig = mWidgetConfigs.at( cache.sortFieldIndex );
-        const QVariant sortValue = fieldFormatter->representValue( mLayer, cache.sortFieldIndex, widgetConfig, widgetCache, mFeat.attribute( cache.sortFieldIndex ) );
+        // If using the default formatter use the raw value for sorting
+        // (keep the correct QVariant type and do not convert to a possibly localized string)
+        // See: https://github.com/qgis/QGIS/issues/34935
+        const bool isFallbackFormatter { fieldFormatter->id().isEmpty() };
+        const QVariant sortValue = isFallbackFormatter ?
+                                   mFeat.attribute( cache.sortFieldIndex ) :
+                                   fieldFormatter->representValue( mLayer, cache.sortFieldIndex, widgetConfig, widgetCache, mFeat.attribute( cache.sortFieldIndex ) );
         cache.sortCache.insert( mFeat.id(), sortValue );
       }
       else if ( cache.sortCacheExpression.isValid() )

diff --git a/src/gui/attributetable/qgsattributetableview.cpp b/src/gui/attributetable/qgsattributetableview.cpp
@@ -94,6 +94,7 @@ void QgsAttributeTableView::setAttributeTableConfig( const QgsAttributeTableConf
 {
   int i = 0;
   const auto constColumns = config.columns();
+  QMap<QString, int> columns;
   for ( const QgsAttributeTableConfig::ColumnConfig &columnConfig : constColumns )
   {
     if ( columnConfig.hidden )
@@ -106,12 +107,29 @@ void QgsAttributeTableView::setAttributeTableConfig( const QgsAttributeTableConf
     else
     {
       setColumnWidth( i, horizontalHeader()->defaultSectionSize() );
+      columns.insert( columnConfig.name, i );
     }
     i++;
   }
   mConfig = config;
   if ( config.sortExpression().isEmpty() )
+  {
     horizontalHeader()->setSortIndicatorShown( false );
+  }
+  else
+  {
+    if ( mSortExpression != config.sortExpression() )
+    {
+      const QgsExpression sortExp { config.sortExpression() };
+      if ( sortExp.isField() )
+      {
+        const QStringList refCols { sortExp.referencedColumns().values() };
+        horizontalHeader()->setSortIndicatorShown( true );
+        horizontalHeader()->setSortIndicator( columns.value( refCols.constFirst() ), config.sortOrder() );
+      }
+    }
+  }
+  mSortExpression = config.sortExpression();
 }
 
 QList<QgsFeatureId> QgsAttributeTableView::selectedFeaturesIds() const

diff --git a/src/gui/attributetable/qgsattributetableview.h b/src/gui/attributetable/qgsattributetableview.h
@@ -205,6 +205,7 @@ class GUI_EXPORT QgsAttributeTableView : public QTableView
     QItemSelectionModel::SelectionFlag mCtrlDragSelectionFlag = QItemSelectionModel::Select;
     QMap< QModelIndex, QWidget * > mActionWidgets;
     QgsAttributeTableConfig mConfig;
+    QString mSortExpression;
 };
 
 #endif
diff --git a/tests/src/app/testqgsattributetable.cpp b/tests/src/app/testqgsattributetable.cpp
@@ -42,9 +42,11 @@ class TestQgsAttributeTable : public QObject
     TestQgsAttributeTable();
 
   private slots:
+
     void initTestCase();// will be called before the first testfunction is executed.
     void cleanupTestCase();// will be called after the last testfunction was executed.
-    void init() {} // will be called before each testfunction is executed.
+    void init();
+    // will be called before each testfunction is executed.
     void cleanup() {} // will be called after every testfunction.
     void testRegression15974();
     void testFieldCalculation();
@@ -58,7 +60,7 @@ class TestQgsAttributeTable : public QObject
     void testFilteredFeatures();
     void testVisibleTemporal();
     void testCopySelectedRows();
-
+    void testSortNumbers();
 
   private:
     QgisApp *mQgisApp = nullptr;
@@ -87,6 +89,11 @@ void TestQgsAttributeTable::cleanupTestCase()
   QgsApplication::exitQgis();
 }
 
+void TestQgsAttributeTable::init()
+{
+  QLocale::setDefault( QLocale::c() );
+}
+
 void TestQgsAttributeTable::testFieldCalculation()
 {
   //test field calculation
@@ -411,6 +418,54 @@ void TestQgsAttributeTable::testSortByDisplayExpression()
   QCOMPARE( listModel->index( 2, 0 ).data( Qt::DisplayRole ), QVariant( 5.0 ) );
 }
 
+void TestQgsAttributeTable::testSortNumbers()
+{
+
+  QLocale::setDefault( QLocale::Italian );
+
+  std::unique_ptr< QgsVectorLayer> tempLayer( new QgsVectorLayer( QStringLiteral( "LineString?crs=epsg:3111&field=pk:int&field=col1:double" ), QStringLiteral( "vl" ), QStringLiteral( "memory" ) ) );
+  QVERIFY( tempLayer->isValid() );
+
+  QgsFeature f1( tempLayer->dataProvider()->fields(), 1 );
+  f1.setAttribute( 0, 1 );
+  f1.setAttribute( 1, 2.001 );
+  QgsFeature f2( tempLayer->dataProvider()->fields(), 2 );
+  f2.setAttribute( 0, 2 );
+  f2.setAttribute( 1, 1001 );
+  QgsFeature f3( tempLayer->dataProvider()->fields(), 3 );
+  f3.setAttribute( 0, 3 );
+  f3.setAttribute( 1, 10.0001 );
+  QVERIFY( tempLayer->dataProvider()->addFeatures( QgsFeatureList() << f1 << f2 << f3 ) );
+
+  std::unique_ptr< QgsAttributeTableDialog > dlg( new QgsAttributeTableDialog( tempLayer.get() ) );
+
+  QgsAttributeTableConfig cfg;
+  cfg.setSortExpression( QStringLiteral( R"("col1")" ) );
+  cfg.setSortOrder( Qt::SortOrder::DescendingOrder );
+  QgsAttributeTableConfig::ColumnConfig cfg1;
+  QgsAttributeTableConfig::ColumnConfig cfg2;
+  cfg1.name = QStringLiteral( "pk" );
+  cfg2.name = QStringLiteral( "col1" );
+  cfg.setColumns( {{ cfg1, cfg2 }} );
+
+  dlg->mMainView->setAttributeTableConfig( cfg );
+
+  auto model { dlg->mMainView->mFilterModel };
+
+  QCOMPARE( model->data( model->index( 2, 1 ), Qt::ItemDataRole::DisplayRole ).toString(), QString( "2,00100" ) );
+  QCOMPARE( model->data( model->index( 1, 1 ), Qt::ItemDataRole::DisplayRole ).toString(), QString( "10,00010" ) );
+  QCOMPARE( model->data( model->index( 0, 1 ), Qt::ItemDataRole::DisplayRole ).toString(), QString( "1.001,00000" ) );
+
+  QCOMPARE( model->data( model->index( 2, 2 ), QgsAttributeTableModel::Role::SortRole ).toDouble(), 2.001 );
+  QCOMPARE( model->data( model->index( 1, 2 ), QgsAttributeTableModel::Role::SortRole ).toDouble(), 10.0001 );
+  QCOMPARE( model->data( model->index( 0, 2 ), QgsAttributeTableModel::Role::SortRole ).toDouble(), 1001.0 );
+
+  QCOMPARE( dlg->mMainView->mTableView->horizontalHeader()->sortIndicatorSection(), 1 );
+  QCOMPARE( dlg->mMainView->mTableView->horizontalHeader()->sortIndicatorOrder(), Qt::SortOrder::DescendingOrder );
+  QVERIFY( dlg->mMainView->mTableView->horizontalHeader()->isSortIndicatorShown() );
+
+}
+
 void TestQgsAttributeTable::testRegression15974()
 {
   // Test duplicated rows in attribute table + two crashes.