Fix confusing sort order of vector formats in save as dialog

We were sorting on the driver name, not the display name, so MS OpenOffice ("XLSX") was appearing after Sqlite.
qgis · Jun 21, 2018 · f332971 · f332971
1 parent 4ddfd13
commit f332971
Show file tree

Hide file tree

Showing 2 changed files with 38 additions and 47 deletions.
diff --git a/src/core/qgsvectorfilewriter.cpp b/src/core/qgsvectorfilewriter.cpp
@@ -2806,9 +2806,6 @@ QList< QgsVectorFileWriter::FilterFormatDetails > QgsVectorFileWriter::supported
   QgsApplication::registerOgrDrivers();
   int const drvCount = OGRGetDriverCount();
 
-  FilterFormatDetails shapeFormat;
-  FilterFormatDetails gpkgFormat;
-
   for ( int i = 0; i < drvCount; ++i )
   {
     OGRSFDriverH drv = OGRGetDriver( i );
@@ -2846,41 +2843,27 @@ QList< QgsVectorFileWriter::FilterFormatDetails > QgsVectorFileWriter::supported
         details.driverName = drvName;
         details.filterString = filterString;
 
-        if ( options & SortRecommended )
-        {
-          if ( drvName == QLatin1String( "ESRI Shapefile" ) )
-          {
-            shapeFormat = details;
-            continue;
-          }
-          else if ( drvName == QLatin1String( "GPKG" ) )
-          {
-            gpkgFormat = details;
-            continue;
-          }
-        }
-
         results << details;
       }
     }
   }
 
-  std::sort( results.begin(), results.end(), []( const FilterFormatDetails & a, const FilterFormatDetails & b ) -> bool
+  std::sort( results.begin(), results.end(), [options]( const FilterFormatDetails & a, const FilterFormatDetails & b ) -> bool
   {
-    return a.driverName < b.driverName;
-  } );
-
-  if ( options & SortRecommended )
-  {
-    if ( !shapeFormat.filterString.isEmpty() )
-    {
-      results.insert( 0, shapeFormat );
-    }
-    if ( !gpkgFormat.filterString.isEmpty() )
+    if ( options & SortRecommended )
     {
-      results.insert( 0, gpkgFormat );
+      if ( a.driverName == QLatin1String( "GPKG" ) )
+        return true; // Make https://twitter.com/shapefiIe a sad little fellow
+      else if ( b.driverName == QLatin1String( "GPKG" ) )
+        return false;
+      else if ( a.driverName == QLatin1String( "ESRI Shapefile" ) )
+        return true;
+      else if ( b.driverName == QLatin1String( "ESRI Shapefile" ) )
+        return false;
     }
-  }
+
+    return a.driverName.toLower().localeAwareCompare( b.driverName.toLower() ) < 0;
+  } );
 
   return results;
 }
@@ -2966,23 +2949,8 @@ QList< QgsVectorFileWriter::DriverDetails > QgsVectorFileWriter::ogrDriverList(
       }
     }
   }
-  std::sort( writableDrivers.begin(), writableDrivers.end() );
-  if ( options & SortRecommended )
-  {
-    // recommended order sorting, so we shift certain formats to the top
-    if ( writableDrivers.contains( QStringLiteral( "ESRI Shapefile" ) ) )
-    {
-      writableDrivers.removeAll( QStringLiteral( "ESRI Shapefile" ) );
-      writableDrivers.insert( 0, QStringLiteral( "ESRI Shapefile" ) );
-    }
-    if ( writableDrivers.contains( QStringLiteral( "GPKG" ) ) )
-    {
-      // Make https://twitter.com/shapefiIe a sad little fellow
-      writableDrivers.removeAll( QStringLiteral( "GPKG" ) );
-      writableDrivers.insert( 0, QStringLiteral( "GPKG" ) );
-    }
-  }
 
+  results.reserve( writableDrivers.count() );
   for ( const QString &drvName : qgis::as_const( writableDrivers ) )
   {
     MetaData metadata;
@@ -2994,6 +2962,23 @@ QList< QgsVectorFileWriter::DriverDetails > QgsVectorFileWriter::ogrDriverList(
       results << details;
     }
   }
+
+  std::sort( results.begin(), results.end(), [options]( const DriverDetails & a, const DriverDetails & b ) -> bool
+  {
+    if ( options & SortRecommended )
+    {
+      if ( a.driverName == QLatin1String( "GPKG" ) )
+        return true; // Make https://twitter.com/shapefiIe a sad little fellow
+      else if ( b.driverName == QLatin1String( "GPKG" ) )
+        return false;
+      else if ( a.driverName == QLatin1String( "ESRI Shapefile" ) )
+        return true;
+      else if ( b.driverName == QLatin1String( "ESRI Shapefile" ) )
+        return false;
+    }
+
+    return a.longName.toLower().localeAwareCompare( b.longName.toLower() ) < 0;
+  } );
   return results;
 }
 

diff --git a/tests/src/python/test_qgsvectorfilewriter.py b/tests/src/python/test_qgsvectorfilewriter.py
@@ -791,6 +791,12 @@ def testOgrDriverList(self):
         self.assertEqual(drivers[1].longName, 'ESRI Shapefile')
         self.assertEqual(drivers[1].driverName, 'ESRI Shapefile')
         self.assertTrue('ODS' in [f.driverName for f in drivers])
+
+        # ensure that XLSX comes before SQLite, because we should sort on longName, not driverName!
+        ms_xlsx_index = next(i for i, v in enumerate(drivers) if v.driverName == 'XLSX')
+        sqlite_index = next(i for i, v in enumerate(drivers) if v.driverName == 'SQLite')
+        self.assertLess(ms_xlsx_index, sqlite_index)
+
         # alphabetical sorting
         drivers2 = QgsVectorFileWriter.ogrDriverList(QgsVectorFileWriter.VectorFormatOptions())
         self.assertTrue(drivers2[0].longName < drivers2[1].longName)
@@ -822,7 +828,7 @@ def testFileFilterString(self):
         formats = QgsVectorFileWriter.fileFilterString()
         self.assertTrue('gpkg' in formats)
         self.assertTrue('shp' in formats)
-        self.assertTrue(formats.index('gpkg') < formats.index('shp'))
+        self.assertLess(formats.index('gpkg'), formats.index('shp'))
         self.assertTrue('ods' in formats)
 
         # alphabetical sorting