Always disable GDAL side shapefile encoding handling

And instead always do the decoding on QGIS' side. This unifies the encoding handling whether or not we are using the underlying shapefile declared encoding (e.g. via LDID or .cpg file) OR are overriding it manually by a user-set encoding. Why? - if we DON'T disable GDAL side encoding support, then there's NO way to change the encoding used when reading shapefiles. And unfortunately the embedded encoding (which is read by GDAL) is sometimes wrong (because shapefiles!), so we need to expose support for users to be able to change and correct this - we can't change this setting on-the-fly. If we don't set it upfront, we can't reverse this decision later when a user does want/need to manually specify the encoding This also removes a lot of confusing code logic in the provider! Fixes qgis#21264, user frustration on mailing lists e.g. http://osgeo-org.1560.x6.nabble.com/Shapefile-with-file-cpg-codepage-td5275106.html http://osgeo-org.1560.x6.nabble.com/QGIS-ignore-the-cpg-files-when-loading-shapefiles-td5348021.html
nyalldawson · Feb 12, 2020 · 8561418 · 8561418
1 parent 4a53dbc
commit 8561418
Show file tree

Hide file tree

Showing 4 changed files with 64 additions and 70 deletions.
diff --git a/src/app/qgsoptions.cpp b/src/app/qgsoptions.cpp
@@ -687,7 +687,6 @@ QgsOptions::QgsOptions( QWidget *parent, Qt::WindowFlags fl, const QList<QgsOpti
   mComboCopyFeatureFormat->addItem( tr( "GeoJSON" ), QgsClipboard::GeoJSON );
   mComboCopyFeatureFormat->setCurrentIndex( mComboCopyFeatureFormat->findData( mSettings->enumValue( QStringLiteral( "/qgis/copyFeatureFormat" ), QgsClipboard::AttributesWithWKT ) ) );
   leNullValue->setText( QgsApplication::nullRepresentation() );
-  cbxIgnoreShapeEncoding->setChecked( mSettings->value( QStringLiteral( "/qgis/ignoreShapeEncoding" ), true ).toBool() );
 
   cmbLegendDoubleClickAction->setCurrentIndex( mSettings->value( QStringLiteral( "/qgis/legendDoubleClickAction" ), 0 ).toInt() );
 
@@ -1486,7 +1485,6 @@ void QgsOptions::saveOptions()
                        cmbScanItemsInBrowser->currentData().toString() );
   mSettings->setValue( QStringLiteral( "/qgis/scanZipInBrowser2" ),
                        cmbScanZipInBrowser->currentData().toString() );
-  mSettings->setValue( QStringLiteral( "/qgis/ignoreShapeEncoding" ), cbxIgnoreShapeEncoding->isChecked() );
   mSettings->setValue( QStringLiteral( "/qgis/mainSnappingWidgetMode" ), mSnappingMainDialogComboBox->currentData() );
 
   mSettings->setValue( QStringLiteral( "/qgis/compileExpressions" ), cbxCompileExpressions->isChecked() );

diff --git a/src/core/providers/ogr/qgsogrprovider.cpp b/src/core/providers/ogr/qgsogrprovider.cpp
@@ -489,7 +489,14 @@ QgsOgrProvider::QgsOgrProvider( QString const &uri, const ProviderOptions &optio
   QgsApplication::registerOgrDrivers();
 
   QgsSettings settings;
-  CPLSetConfigOption( "SHAPE_ENCODING", settings.value( QStringLiteral( "qgis/ignoreShapeEncoding" ), true ).toBool() ? "" : nullptr );
+  // we always disable GDAL side shapefile encoding handling, and do it on the QGIS side.
+  // why? it's not the ideal choice, but...
+  // - if we DON'T disable GDAL side encoding support, then there's NO way to change the encoding used when reading
+  //   shapefiles. And unfortunately the embedded encoding (which is read by GDAL) is sometimes wrong, so we need
+  //   to expose support for users to be able to change and correct this
+  // - we can't change this setting on-the-fly. If we don't set it upfront, we can't reverse this decision later when
+  //   a user does want/need to manually specify the encoding
+  CPLSetConfigOption( "SHAPE_ENCODING", "" );
 
 #ifndef QT_NO_NETWORKPROXY
   setupProxy();
@@ -1003,8 +1010,7 @@ QStringList QgsOgrProvider::_subLayers( bool withFeatureCount )  const
 void QgsOgrProvider::setEncoding( const QString &e )
 {
   QgsSettings settings;
-  if ( ( mGDALDriverName == QLatin1String( "ESRI Shapefile" ) &&
-         settings.value( QStringLiteral( "qgis/ignoreShapeEncoding" ), true ).toBool() ) ||
+  if ( mGDALDriverName == QLatin1String( "ESRI Shapefile" ) ||
        ( mOgrLayer && !mOgrLayer->TestCapability( OLCStringsAsUTF8 ) ) )
   {
     QgsVectorDataProvider::setEncoding( e );
@@ -3580,12 +3586,6 @@ bool QgsOgrProviderUtils::createEmptyDataSource( const QString &uri,
   layer = GDALDatasetCreateLayer( dataSource.get(), QFileInfo( uri ).completeBaseName().toUtf8().constData(), reference, OGRvectortype, papszOptions );
   CSLDestroy( papszOptions );
 
-  QgsSettings settings;
-  if ( !settings.value( QStringLiteral( "qgis/ignoreShapeEncoding" ), true ).toBool() )
-  {
-    CPLSetConfigOption( "SHAPE_ENCODING", nullptr );
-  }
-
   if ( !layer )
   {
     errorMessage = QString::fromUtf8( CPLGetLastErrorMsg() );
@@ -4531,7 +4531,20 @@ void QgsOgrProvider::open( OpenMode mode )
     mOgrLayer = mOgrOrigLayer.get();
 
     // check that the initial encoding setting is fit for this layer
-    setEncoding( encoding() );
+
+    if ( mode == OpenModeInitial && mGDALDriverName == QLatin1String( "ESRI Shapefile" ) )
+    {
+      // determine encoding from shapefile cpg or LDID information, if possible
+      const QString shpEncoding = QgsOgrUtils::readShapefileEncoding( mFilePath );
+      if ( !shpEncoding.isEmpty() )
+        setEncoding( shpEncoding );
+      else
+        setEncoding( encoding() );
+    }
+    else
+    {
+      setEncoding( encoding() );
+    }
 
     // Ensure subset is set (setSubsetString does nothing if the passed sql subset string is equal to mSubsetString, which is the case when reloading the dataset)
     QString origSubsetString = mSubsetString;

diff --git a/src/core/qgsvectorfilewriter.cpp b/src/core/qgsvectorfilewriter.cpp
@@ -497,12 +497,6 @@ void QgsVectorFileWriter::init( QString vectorFileName,
     options = nullptr;
   }
 
-  QgsSettings settings;
-  if ( !settings.value( QStringLiteral( "qgis/ignoreShapeEncoding" ), true ).toBool() )
-  {
-    CPLSetConfigOption( "SHAPE_ENCODING", nullptr );
-  }
-
   if ( srs.isValid() )
   {
     if ( mOgrDriverName == QLatin1String( "ESRI Shapefile" ) )

diff --git a/src/ui/qgsoptionsbase.ui b/src/ui/qgsoptionsbase.ui
@@ -1995,44 +1995,46 @@
                  <property name="title">
                   <string>Data source handling</string>
                  </property>
-                 <layout class="QGridLayout" name="gridLayout_23" columnstretch="0,2,1,0">
+                 <layout class="QGridLayout" name="gridLayout_23" columnstretch="0,0,0,0">
                   <item row="0" column="2">
                    <widget class="QComboBox" name="cmbScanItemsInBrowser"/>
                   </item>
-                  <item row="1" column="0">
-                   <widget class="QLabel" name="label_29">
-                    <property name="text">
-                     <string>Scan for contents of compressed files (.zip) in browser dock</string>
-                    </property>
+                  <item row="2" column="2">
+                   <widget class="QComboBox" name="cmbPromptRasterSublayers">
+                    <item>
+                     <property name="text">
+                      <string/>
+                     </property>
+                    </item>
                    </widget>
                   </item>
-                  <item row="2" column="0">
-                   <widget class="QLabel" name="textLabel1_13">
-                    <property name="sizePolicy">
-                     <sizepolicy hsizetype="Fixed" vsizetype="Fixed">
-                      <horstretch>0</horstretch>
-                      <verstretch>0</verstretch>
-                     </sizepolicy>
+                  <item row="1" column="1">
+                   <spacer name="horizontalSpacer_3">
+                    <property name="orientation">
+                     <enum>Qt::Horizontal</enum>
                     </property>
-                    <property name="text">
-                     <string>Prompt for raster sublayers when opening</string>
+                    <property name="sizeHint" stdset="0">
+                     <size>
+                      <width>40</width>
+                      <height>20</height>
+                     </size>
                     </property>
-                   </widget>
+                   </spacer>
                   </item>
-                  <item row="6" column="0" colspan="4">
-                   <widget class="QCheckBox" name="cbxCompileExpressions">
+                  <item row="1" column="0">
+                   <widget class="QLabel" name="label_29">
                     <property name="text">
-                     <string>Execute expressions on server-side if possible</string>
+                     <string>Scan for contents of compressed files (.zip) in browser dock</string>
                     </property>
                    </widget>
                   </item>
-                  <item row="3" column="0" colspan="4">
-                   <widget class="QCheckBox" name="cbxIgnoreShapeEncoding">
+                  <item row="6" column="0" colspan="4">
+                   <widget class="QCheckBox" name="cbxEvaluateDefaultValues">
                     <property name="toolTip">
-                     <string>Disable OGR on-the-fly conversion from declared encoding to UTF-8</string>
+                     <string>&lt;html&gt;&lt;head/&gt;&lt;body&gt;&lt;p&gt;When digitizing a new feature, default values are retrieved from the database. With this option turned on, the default values will be evaluated at the time of digitizing. With this option turned off, the default values will be evaluated at the time of saving.&lt;/p&gt;&lt;/body&gt;&lt;/html&gt;</string>
                     </property>
                     <property name="text">
-                     <string>Ignore shapefile encoding declaration</string>
+                     <string>Evaluate default values</string>
                     </property>
                    </widget>
                   </item>
@@ -2043,44 +2045,32 @@
                     </property>
                    </widget>
                   </item>
-                  <item row="7" column="0" colspan="4">
-                   <widget class="QCheckBox" name="cbxEvaluateDefaultValues">
-                    <property name="toolTip">
-                     <string>&lt;html&gt;&lt;head/&gt;&lt;body&gt;&lt;p&gt;When digitizing a new feature, default values are retrieved from the database. With this option turned on, the default values will be evaluated at the time of digitizing. With this option turned off, the default values will be evaluated at the time of saving.&lt;/p&gt;&lt;/body&gt;&lt;/html&gt;</string>
-                    </property>
+                  <item row="0" column="0">
+                   <widget class="QLabel" name="label_30">
                     <property name="text">
-                     <string>Evaluate default values</string>
+                     <string>Scan for valid items in the browser dock</string>
                     </property>
                    </widget>
                   </item>
-                  <item row="2" column="2">
-                   <widget class="QComboBox" name="cmbPromptRasterSublayers">
-                    <item>
-                     <property name="text">
-                      <string/>
-                     </property>
-                    </item>
-                   </widget>
-                  </item>
-                  <item row="0" column="0">
-                   <widget class="QLabel" name="label_30">
+                  <item row="5" column="0" colspan="4">
+                   <widget class="QCheckBox" name="cbxCompileExpressions">
                     <property name="text">
-                     <string>Scan for valid items in the browser dock</string>
+                     <string>Execute expressions on server-side if possible</string>
                     </property>
                    </widget>
                   </item>
-                  <item row="1" column="1">
-                   <spacer name="horizontalSpacer_3">
-                    <property name="orientation">
-                     <enum>Qt::Horizontal</enum>
+                  <item row="2" column="0">
+                   <widget class="QLabel" name="textLabel1_13">
+                    <property name="sizePolicy">
+                     <sizepolicy hsizetype="Fixed" vsizetype="Fixed">
+                      <horstretch>0</horstretch>
+                      <verstretch>0</verstretch>
+                     </sizepolicy>
                     </property>
-                    <property name="sizeHint" stdset="0">
-                     <size>
-                      <width>40</width>
-                      <height>20</height>
-                     </size>
+                    <property name="text">
+                     <string>Prompt for raster sublayers when opening</string>
                     </property>
-                   </spacer>
+                   </widget>
                   </item>
                  </layout>
                 </widget>
@@ -5767,7 +5757,6 @@ p, li { white-space: pre-wrap; }
   <tabstop>cmbScanItemsInBrowser</tabstop>
   <tabstop>cmbScanZipInBrowser</tabstop>
   <tabstop>cmbPromptRasterSublayers</tabstop>
-  <tabstop>cbxIgnoreShapeEncoding</tabstop>
   <tabstop>cbxCompileExpressions</tabstop>
   <tabstop>cbxEvaluateDefaultValues</tabstop>
   <tabstop>mBtnRemoveHiddenPath</tabstop>