Consolidate all qvariant sort methods to use qgsVariantLessThan,

make sure qgsVariantLessThan incorporates all functionality from
other duplicate implementations, and add tests

(fixes #14671)

(cherry-picked from 647f32)

Author: nyalldawson

python/core/qgis.sip

@@ -233,6 +233,17 @@ class QGis
     static double DEFAULT_HIGHLIGHT_MIN_WIDTH_MM;
 };
 
+//! Compares two QVariant values and returns whether the first is less than the second.
+//! Useful for sorting lists of variants, correctly handling sorting of the various
+//! QVariant data types (such as strings, numeric values, dates and times)
+//! @see qgsVariantGreaterThan()
+bool qgsVariantLessThan( const QVariant& lhs, const QVariant& rhs );
+
+//! Compares two QVariant values and returns whether the first is greater than the second.
+//! Useful for sorting lists of variants, correctly handling sorting of the various
+//! QVariant data types (such as strings, numeric values, dates and times)
+//! @see qgsVariantLessThan()
+bool qgsVariantGreaterThan( const QVariant& lhs, const QVariant& rhs );
 
 /** Wkt string that represents a geographic coord sys
  * @note added to replace GEOWkt

src/core/composer/qgsatlascomposition.cpp

@@ -166,23 +166,10 @@ class FieldSorter
 
     bool operator()( const QPair< QgsFeatureId, QString > & id1, const QPair< QgsFeatureId, QString >& id2 )
     {
-      bool result = true;
-
-      if ( mKeys[ id1.first ].type() == QVariant::Int )
-      {
-        result = mKeys[ id1.first ].toInt() < mKeys[ id2.first ].toInt();
-      }
-      else if ( mKeys[ id1.first ].type() == QVariant::Double )
-      {
-        result = mKeys[ id1.first ].toDouble() < mKeys[ id2.first ].toDouble();
-      }
-      else if ( mKeys[ id1.first ].type() == QVariant::String )
-      {
-        result = ( QString::localeAwareCompare( mKeys[ id1.first ].toString(), mKeys[ id2.first ].toString() ) < 0 );
-      }
-
-      return mAscending ? result : !result;
+      return mAscending ? qgsVariantLessThan( mKeys.value( id1.first ), mKeys.value( id2.first ) )
+             : qgsVariantGreaterThan( mKeys.value( id1.first ), mKeys.value( id2.first ) );
     }
+
   private:
     QgsAtlasComposition::SorterKeys& mKeys;
     bool mAscending;

src/core/composer/qgscomposerattributetable.cpp

@@ -32,60 +32,8 @@ QgsComposerAttributeTableCompare::QgsComposerAttributeTableCompare()
 
 bool QgsComposerAttributeTableCompare::operator()( const QgsAttributeMap& m1, const QgsAttributeMap& m2 )
 {
-  QVariant v1 = m1[mCurrentSortColumn];
-  QVariant v2 = m2[mCurrentSortColumn];
-
-  bool less = false;
-
-  //sort null values first
-  if ( v1.isNull() && v2.isNull() )
-  {
-    less = false;
-  }
-  else if ( v1.isNull() )
-  {
-    less = true;
-  }
-  else if ( v2.isNull() )
-  {
-    less = false;
-  }
-  else
-  {
-    //otherwise sort by converting to corresponding type and comparing
-    switch ( v1.type() )
-    {
-      case QVariant::Int:
-      case QVariant::UInt:
-      case QVariant::LongLong:
-      case QVariant::ULongLong:
-        less = v1.toLongLong() < v2.toLongLong();
-        break;
-
-      case QVariant::Double:
-        less = v1.toDouble() < v2.toDouble();
-        break;
-
-      case QVariant::Date:
-        less = v1.toDate() < v2.toDate();
-        break;
-
-      case QVariant::DateTime:
-        less = v1.toDateTime() < v2.toDateTime();
-        break;
-
-      case QVariant::Time:
-        less = v1.toTime() < v2.toTime();
-        break;
-
-      default:
-        //use locale aware compare for strings
-        less = v1.toString().localeAwareCompare( v2.toString() ) < 0;
-        break;
-    }
-  }
-
-  return ( mAscending ? less : !less );
+  return ( mAscending ? qgsVariantLessThan( m1[mCurrentSortColumn], m2[mCurrentSortColumn] )
+           : qgsVariantGreaterThan( m1[mCurrentSortColumn], m2[mCurrentSortColumn] ) );
 }
 
 

src/core/composer/qgscomposerattributetablev2.cpp

@@ -37,60 +37,8 @@ QgsComposerAttributeTableCompareV2::QgsComposerAttributeTableCompareV2()
 
 bool QgsComposerAttributeTableCompareV2::operator()( const QgsComposerTableRow& m1, const QgsComposerTableRow& m2 )
 {
-  QVariant v1 = m1[mCurrentSortColumn];
-  QVariant v2 = m2[mCurrentSortColumn];
-
-  bool less = false;
-
-  //sort null values first
-  if ( v1.isNull() && v2.isNull() )
-  {
-    less = false;
-  }
-  else if ( v1.isNull() )
-  {
-    less = true;
-  }
-  else if ( v2.isNull() )
-  {
-    less = false;
-  }
-  else
-  {
-    //otherwise sort by converting to corresponding type and comparing
-    switch ( v1.type() )
-    {
-      case QVariant::Int:
-      case QVariant::UInt:
-      case QVariant::LongLong:
-      case QVariant::ULongLong:
-        less = v1.toLongLong() < v2.toLongLong();
-        break;
-
-      case QVariant::Double:
-        less = v1.toDouble() < v2.toDouble();
-        break;
-
-      case QVariant::Date:
-        less = v1.toDate() < v2.toDate();
-        break;
-
-      case QVariant::DateTime:
-        less = v1.toDateTime() < v2.toDateTime();
-        break;
-
-      case QVariant::Time:
-        less = v1.toTime() < v2.toTime();
-        break;
-
-      default:
-        //use locale aware compare for strings
-        less = v1.toString().localeAwareCompare( v2.toString() ) < 0;
-        break;
-    }
-  }
-
-  return ( mAscending ? less : !less );
+  return ( mAscending ? qgsVariantLessThan( m1[mCurrentSortColumn], m2[mCurrentSortColumn] )
+           : qgsVariantGreaterThan( m1[mCurrentSortColumn], m2[mCurrentSortColumn] ) );
 }
 
 //

src/core/qgis.cpp

@@ -260,6 +260,14 @@ void qgsFree( void *ptr )
 
 bool qgsVariantLessThan( const QVariant& lhs, const QVariant& rhs )
 {
+  // invalid < NULL < any value
+  if ( !lhs.isValid() )
+    return rhs.isValid();
+  else if ( lhs.isNull() )
+    return rhs.isValid() && !rhs.isNull();
+  else if ( !rhs.isValid() || rhs.isNull() )
+    return false;
+
   switch ( lhs.type() )
   {
     case QVariant::Int:
@@ -280,6 +288,39 @@ bool qgsVariantLessThan( const QVariant& lhs, const QVariant& rhs )
       return lhs.toTime() < rhs.toTime();
     case QVariant::DateTime:
       return lhs.toDateTime() < rhs.toDateTime();
+    case QVariant::Bool:
+      return lhs.toBool() < rhs.toBool();
+
+    case QVariant::List:
+    {
+      const QList<QVariant> &lhsl = lhs.toList();
+      const QList<QVariant> &rhsl = rhs.toList();
+
+      int i, n = qMin( lhsl.size(), rhsl.size() );
+      for ( i = 0; i < n && lhsl[i].type() == rhsl[i].type() && lhsl[i].isNull() == rhsl[i].isNull() && lhsl[i] == rhsl[i]; i++ )
+        ;
+
+      if ( i == n )
+        return lhsl.size() < rhsl.size();
+      else
+        return qgsVariantLessThan( lhsl[i], rhsl[i] );
+    }
+
+    case QVariant::StringList:
+    {
+      const QStringList &lhsl = lhs.toStringList();
+      const QStringList &rhsl = rhs.toStringList();
+
+      int i, n = qMin( lhsl.size(), rhsl.size() );
+      for ( i = 0; i < n && lhsl[i] == rhsl[i]; i++ )
+        ;
+
+      if ( i == n )
+        return lhsl.size() < rhsl.size();
+      else
+        return lhsl[i] < rhsl[i];
+    }
+
     default:
       return QString::localeAwareCompare( lhs.toString(), rhs.toString() ) < 0;
   }

src/core/qgis.h

@@ -314,9 +314,17 @@ inline double qgsRound( double x )
   return x < 0.0 ? std::ceil( x - 0.5 ) : std::floor( x + 0.5 );
 }
 
-bool qgsVariantLessThan( const QVariant& lhs, const QVariant& rhs );
-
-bool qgsVariantGreaterThan( const QVariant& lhs, const QVariant& rhs );
+//! Compares two QVariant values and returns whether the first is less than the second.
+//! Useful for sorting lists of variants, correctly handling sorting of the various
+//! QVariant data types (such as strings, numeric values, dates and times)
+//! @see qgsVariantGreaterThan()
+CORE_EXPORT bool qgsVariantLessThan( const QVariant& lhs, const QVariant& rhs );
+
+//! Compares two QVariant values and returns whether the first is greater than the second.
+//! Useful for sorting lists of variants, correctly handling sorting of the various
+//! QVariant data types (such as strings, numeric values, dates and times)
+//! @see qgsVariantLessThan()
+CORE_EXPORT bool qgsVariantGreaterThan( const QVariant& lhs, const QVariant& rhs );
 
 CORE_EXPORT QString qgsVsiPrefix( const QString& path );
 

src/core/symbology-ng/qgssymbollayerv2utils.cpp

@@ -3590,40 +3590,6 @@ void QgsSymbolLayerV2Utils::premultiplyColor( QColor &rgb, int alpha )
   }
 }
 
-#if 0
-static bool _QVariantLessThan( const QVariant& lhs, const QVariant& rhs )
-{
-  switch ( lhs.type() )
-  {
-    case QVariant::Int:
-      return lhs.toInt() < rhs.toInt();
-    case QVariant::UInt:
-      return lhs.toUInt() < rhs.toUInt();
-    case QVariant::LongLong:
-      return lhs.toLongLong() < rhs.toLongLong();
-    case QVariant::ULongLong:
-      return lhs.toULongLong() < rhs.toULongLong();
-    case QVariant::Double:
-      return lhs.toDouble() < rhs.toDouble();
-    case QVariant::Char:
-      return lhs.toChar() < rhs.toChar();
-    case QVariant::Date:
-      return lhs.toDate() < rhs.toDate();
-    case QVariant::Time:
-      return lhs.toTime() < rhs.toTime();
-    case QVariant::DateTime:
-      return lhs.toDateTime() < rhs.toDateTime();
-    default:
-      return QString::localeAwareCompare( lhs.toString(), rhs.toString() ) < 0;
-  }
-}
-
-static bool _QVariantGreaterThan( const QVariant& lhs, const QVariant& rhs )
-{
-  return ! _QVariantLessThan( lhs, rhs );
-}
-#endif
-
 void QgsSymbolLayerV2Utils::sortVariantList( QList<QVariant>& list, Qt::SortOrder order )
 {
   if ( order == Qt::AscendingOrder )

src/gui/attributetable/qgsattributetablefiltermodel.cpp

@@ -15,6 +15,7 @@
 
 #include <QItemSelectionModel>
 
+#include "qgis.h"
 #include "qgsattributetablefiltermodel.h"
 #include "qgsattributetablemodel.h"
 #include "qgsvectorlayer.h"
@@ -56,39 +57,8 @@ bool QgsAttributeTableFilterModel::lessThan( const QModelIndex &left, const QMod
     }
   }
 
-
-  QVariant leftData = left.data( QgsAttributeTableModel::SortRole );
-  QVariant rightData = right.data( QgsAttributeTableModel::SortRole );
-
-  if ( leftData.isNull() )
-    return true;
-
-  if ( rightData.isNull() )
-    return false;
-
-  switch ( leftData.type() )
-  {
-    case QVariant::Int:
-    case QVariant::UInt:
-    case QVariant::LongLong:
-    case QVariant::ULongLong:
-      return leftData.toLongLong() < rightData.toLongLong();
-
-    case QVariant::Double:
-      return leftData.toDouble() < rightData.toDouble();
-
-    case QVariant::Date:
-      return leftData.toDate() < rightData.toDate();
-
-    case QVariant::Time:
-      return leftData.toTime() < rightData.toTime();
-
-    case QVariant::DateTime:
-      return leftData.toDateTime() < rightData.toDateTime();
-
-    default:
-      return leftData.toString().localeAwareCompare( rightData.toString() ) < 0;
-  }
+  return qgsVariantLessThan( left.data( QgsAttributeTableModel::SortRole ),
+                             right.data( QgsAttributeTableModel::SortRole ) );
 }
 
 void QgsAttributeTableFilterModel::sort( int column, Qt::SortOrder order )

src/gui/editorwidgets/qgsrelationreferencewidget.cpp

@@ -34,22 +34,6 @@
 #include "qgsvectorlayer.h"
 #include "qgsattributetablemodel.h"
 
-bool orderByLessThan( const QgsRelationReferenceWidget::ValueRelationItem& p1
-                      , const QgsRelationReferenceWidget::ValueRelationItem& p2 )
-{
-  switch ( p1.first.type() )
-  {
-    case QVariant::String:
-      return p1.first.toString() < p2.first.toString();
-
-    case QVariant::Double:
-      return p1.first.toDouble() < p2.first.toDouble();
-
-    default:
-      return p1.first.toInt() < p2.first.toInt();
-  }
-}
-
 QgsRelationReferenceWidget::QgsRelationReferenceWidget( QWidget* parent )
     : QWidget( parent )
     , mEditorContext( QgsAttributeEditorContext() )

src/gui/editorwidgets/qgsvaluerelationwidgetwrapper.cpp

@@ -15,6 +15,7 @@
 
 #include "qgsvaluerelationwidgetwrapper.h"
 
+#include "qgis.h"
 #include "qgsfield.h"
 #include "qgsmaplayerregistry.h"
 #include "qgsvaluerelationwidgetfactory.h"
@@ -27,23 +28,13 @@
 bool QgsValueRelationWidgetWrapper::orderByKeyLessThan( const QgsValueRelationWidgetWrapper::ValueRelationItem& p1
     , const QgsValueRelationWidgetWrapper::ValueRelationItem& p2 )
 {
-  switch ( p1.first.type() )
-  {
-    case QVariant::String:
-      return p1.first.toString() < p2.first.toString();
-
-    case QVariant::Double:
-      return p1.first.toDouble() < p2.first.toDouble();
-
-    default:
-      return p1.first.toInt() < p2.first.toInt();
-  }
+  return qgsVariantLessThan( p1.first, p2.first );
 }
 
 bool QgsValueRelationWidgetWrapper::orderByValueLessThan( const QgsValueRelationWidgetWrapper::ValueRelationItem& p1
     , const QgsValueRelationWidgetWrapper::ValueRelationItem& p2 )
 {
-  return p1.second < p2.second;
+  return qgsVariantLessThan( p1.second, p2.second );
 }
 
 QgsValueRelationWidgetWrapper::QgsValueRelationWidgetWrapper( QgsVectorLayer* vl, int fieldIdx, QWidget* editor, QWidget* parent )