Navigation Menu

Skip to content

Commit

Permalink
Consolidate all qvariant sort methods to use qgsVariantLessThan,
Browse files Browse the repository at this point in the history 
make sure qgsVariantLessThan incorporates all functionality from
other duplicate implementations, and add tests

(fixes #14671)
  • Loading branch information
@nyalldawson
nyalldawson committed Apr 22, 2016
1 parent 7290e8c commit 647f322
Show file tree
Hide file tree
Showing 11 changed files with 154 additions and 234 deletions.
11 changes: 11 additions & 0 deletions python/core/qgis.sip
View file
Expand Up @@ -243,6 +243,17 @@ class QGis
static double SCALE_PRECISION;
};

//! 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
Expand Down
19 changes: 3 additions & 16 deletions src/core/composer/qgsatlascomposition.cpp
View file
Expand Up @@ -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;
Expand Down
56 changes: 2 additions & 54 deletions src/core/composer/qgscomposerattributetable.cpp
View file
Expand Up @@ -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] ) );
}


Expand Down
56 changes: 2 additions & 54 deletions src/core/composer/qgscomposerattributetablev2.cpp
View file
Expand Up @@ -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] ) );
}

//
Expand Down
41 changes: 41 additions & 0 deletions src/core/qgis.cpp
View file
Expand Up @@ -262,6 +262,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:
Expand All @@ -282,6 +290,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;
}
Expand Down
35 changes: 20 additions & 15 deletions src/core/qgis.h
View file
Expand Up @@ -330,9 +330,9 @@ template<class Object> inline QgsSignalBlocker<Object> whileBlocking( Object* ob
return QgsSignalBlocker<Object>( object );
}

//
// return a string representation of a double
//
//! Returns a string representation of a double
//! @param a double value
//! @param precision number of decimal places to retain
inline QString qgsDoubleToString( double a, int precision = 17 )
{
if ( precision )
Expand All @@ -341,18 +341,17 @@ inline QString qgsDoubleToString( double a, int precision = 17 )
return QString::number( a, 'f', precision );
}

//
// compare two doubles (but allow some difference)
//
//! Compare two doubles (but allow some difference)
//! @param a first double
//! @param b second double
//! @param epsilon maximum difference allowable between doubles
inline bool qgsDoubleNear( double a, double b, double epsilon = 4 * DBL_EPSILON )
{
const double diff = a - b;
return diff > -epsilon && diff <= epsilon;
}

//
// compare two doubles using specified number of significant digits
//
//! Compare two doubles using specified number of significant digits
inline bool qgsDoubleNearSig( double a, double b, int significantDigits = 10 )
{
// The most simple would be to print numbers as %.xe and compare as strings
Expand All @@ -368,17 +367,23 @@ inline bool qgsDoubleNearSig( double a, double b, int significantDigits = 10 )
qRound( ar * pow( 10.0, significantDigits ) ) == qRound( br * pow( 10.0, significantDigits ) );
}

//
// a round function which returns a double to guard against overflows
//
//! A round function which returns a double to guard against overflows
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 );

Expand Down
34 changes: 0 additions & 34 deletions src/core/symbology-ng/qgssymbollayerv2utils.cpp
View file
Expand Up @@ -3642,40 +3642,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 )
Expand Down
36 changes: 3 additions & 33 deletions src/gui/attributetable/qgsattributetablefiltermodel.cpp
View file
Expand Up @@ -15,6 +15,7 @@

#include <QItemSelectionModel>

#include "qgis.h"
#include "qgsattributetablefiltermodel.h"
#include "qgsattributetablemodel.h"
#include "qgsvectorlayer.h"
Expand Down Expand Up @@ -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 )
Expand Down

3 comments on commit 647f322

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on 647f322 Apr 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testwise we could check if the Qt5 < operator on QVariant behaves equally.

@nyalldawson
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wondered about using that, but in the end decided that we're probably better in the long run with our own custom implementation.

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on 647f322 Apr 22, 2016 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.