Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add options to override locale and number group sep. #7337

Merged
merged 7 commits into from
Jul 3, 2018

Conversation

elpaso
Copy link
Contributor

@elpaso elpaso commented Jun 29, 2018

@m-kuhn would you like to give this a try?

@m-kuhn
Copy link
Member

m-kuhn commented Jun 29, 2018

Can you add a screenshot?

@elpaso
Copy link
Contributor Author

elpaso commented Jun 29, 2018

immagine

@m-kuhn
Copy link
Member

m-kuhn commented Jun 29, 2018

I think it's quite obscure to know what happens without in-depth knowledge of locales.

I'd be in favour of

  • a "No locale formatting" radio button as a shortcut to the C locale
  • a preview of what numbers are rendered with the current setting

@elpaso
Copy link
Contributor Author

elpaso commented Jun 29, 2018

The preview is actually a good idea, I was going to make it.
Btw, locale also affects dates formatting, that's why C locale would probably be a bad choice in most (non US) situations.

@m-kuhn
Copy link
Member

m-kuhn commented Jun 29, 2018

So how would one get to the desired result of no separators but local date representation?

@elpaso
Copy link
Contributor Author

elpaso commented Jun 29, 2018

AFAIK: no way

@elpaso
Copy link
Contributor Author

elpaso commented Jun 29, 2018

... unless you change the settings at the OS level

@m-kuhn
Copy link
Member

m-kuhn commented Jun 29, 2018

Maybe it's worth a try to overwrite LC_NUMERIC on environment variable level and test it for portability?

@elpaso
Copy link
Contributor Author

elpaso commented Jun 29, 2018

Is that supported on not-unix?

@elpaso
Copy link
Contributor Author

elpaso commented Jun 29, 2018

@m-kuhn
Copy link
Member

m-kuhn commented Jun 29, 2018

I've got no idea, that's why I wrote "test it for portability".

Options that I can see at the moment

  • Upstream patch for QLocale
  • Find an approach to trick OS settings (like env variables)
  • if (user.reallyPrefers('SystemLocale')) behavior for numeric representations in attribute table and friends (do not completely rely on broken QLocale)
  • Revert

@elpaso
Copy link
Contributor Author

elpaso commented Jun 29, 2018

locales

@nyalldawson
Copy link
Collaborator

Just thinking...

I like the direction this is going in general, but for attribute formatting my preference would probably be:

  • always default to no thousand separator
  • have an option in the numeric editor widgets to show thousand separator, but not-enabled by default

What's everyone elses thoughts?

@elpaso
Copy link
Contributor Author

elpaso commented Jun 30, 2018

I don't really mind if it is enabled by default or not as long as it's possible to enable it.

Thousands grouping increase readability and it is an important feature for visually impaired and older people.

@m-kuhn
Copy link
Member

m-kuhn commented Jun 30, 2018

I'm still afraid Germans (just an example) with a preference for mm.dd.yyyy date style, as currency and . as separator don't prefer the current behaviour over 3.0 behaviour and I can't see what they are supposed to do. And in case it's unclear, I'm blaming Qt here for the QLocale implementation.

@jef-n
Copy link
Member

jef-n commented Jun 30, 2018

I for one never missed the thousands separator - and now that it's there I find it annoying - not as much as I would hate mm.dd.yyyy - but close ;) Adding a option to get rid of it is nice. Thanks.

BTW I use ISO yyyy-mm-dd where I can (as it sorts well lexically), then dd.mm.yyyy, which is at least still in reversed order, but no idea why anyone would ever consider m/d/yyyy a preferable order.

@elpaso
Copy link
Contributor Author

elpaso commented Jul 2, 2018

Ok, I don't think the situation is too bad now:

show-group-separator

  • group separator is now hidden by default
  • default locale are taken from system locale (so you'll get your locale's decimal point)
  • dates/time formats are not affected because the date widget use its own formatting options (and it uses ISO by default anyway)

@andreasneumann
Copy link
Member

Hi @elpaso - thanks for looking into this and working on it!

In Switzerland, we have the problem that both the "." and the "," can be used as decimal separator. However, the system locale "de_CH" forces one into using the ",". Something that the majority of the Swiss QGIS users probably don't like. We had the "." as decimal point for almost two decades now, and suddenly everything switches to the ",". Very annoying for us.

Can we make also the decimal separator configurable?

See https://en.wikipedia.org/wiki/Decimal_separator where you can see that some provinces in Switzerland use the "." and some the ",".

@elpaso
Copy link
Contributor Author

elpaso commented Jul 2, 2018

@andreasneumann the decimal point is configurable: just choose a locale which uses a dot as decimal point (such as EN_us for .)

@andreasneumann
Copy link
Member

@elpaso - ok that's what we'll have to do. Hope it doesn't have any other side effects.

@elpaso
Copy link
Contributor Author

elpaso commented Jul 2, 2018

@andreasneumann there shouldn't be any side-effect, but you can check it right now in you 3.2 installation: just run this code right after launching QGIS:

from qgis.PyQt.QtCore import  QLocale
locale = QLocale('EN_us')  # Or whatever you want to test
locale.setNumberOptions(locale.numberOptions() | QLocale.OmitGroupSeparator) # Hide thousand group
locale.setDefault(locale)

this is in effect immediately but you'll need to re-open the attribute table of the forms to get them updated.

... the translations available in QGIS.
@@ -1142,9 +1142,18 @@ int main( int argc, char *argv[] )
/* Translation file for QGIS.
*/
QString i18nPath = QgsApplication::i18nPath();
QString myUserLocale = mySettings.value( QStringLiteral( "locale/userLocale" ), "" ).toString();
QString myUserTranslation = mySettings.value( QStringLiteral( "locale/userLocale" ), "" ).toString();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we drop the outdated "my" prefix here?

src/app/main.cpp Outdated
QLocale currentLocale;
if ( myShowGroupSeparatorFlag )
{
currentLocale.setNumberOptions( currentLocale.numberOptions() &= !QLocale::NumberOption::OmitGroupSeparator );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be ~QLocale::NumberOption::OmitGroupSeparator?

@@ -96,6 +96,8 @@ QgsOptions::QgsOptions( QWidget *parent, Qt::WindowFlags fl, const QList<QgsOpti
connect( mCustomVariablesChkBx, &QCheckBox::toggled, this, &QgsOptions::mCustomVariablesChkBx_toggled );
connect( mCurrentVariablesQGISChxBx, &QCheckBox::toggled, this, &QgsOptions::mCurrentVariablesQGISChxBx_toggled );
connect( buttonBox, &QDialogButtonBox::helpRequested, this, &QgsOptions::showHelp );
connect( cboGlobalLocale, static_cast<void ( QComboBox::* )( int )>( &QComboBox::currentIndexChanged ), [ = ]( int ) { updateSampleLocaleText( ); } );
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI (not a blocker) you can use qgis::overload< int >::of( &QComboBox::currentIndexChanged ) instead - it's a bit more compact than the static_cast version

QLocale locale( cboGlobalLocale->currentData( ).toString() );
if ( cbShowGroupSeparator->isChecked( ) )
{
locale.setNumberOptions( locale.numberOptions() &= ! QLocale::NumberOption::OmitGroupSeparator );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again - should be ~, not !

@elpaso elpaso merged commit 85da39a into qgis:master Jul 3, 2018
@elpaso elpaso deleted the locale-options branch July 3, 2018 07:47
@elpaso elpaso mentioned this pull request Jul 3, 2018
@veikkoeeva
Copy link

veikkoeeva commented Nov 4, 2018

I realize this is an old thread, but I got here via search engine and seems to have the most relevant discussion (I'm not sure if I should open a new issue).

I experience a bit of a bugger issue with fi-Fi (I have fi-FI Windows 10) that shows in the picture. If I put in comma separated decimal values, it appears the raster data set bounds are just set to [0, 0] and nothing shows on the map. It's not also possible to insert . in the field either from the keyboard or copying. On the other hand I tried to change the locale to en-GB whilee keeping the language in Finnish, but there's some other problem I think interpreting decimal numbers wrong and hence the visualization fails in other ways.

I do appreciate the translation and respecting system locale. I also like the idea being able to set program locale independently of the system locale (i.e. I'd set it into Finnish on a company machine that's English). It appears to be the internal reprsentation isn't in some invariant form and things go wrong. I don't know enough to tell where's the problem (calling command line software?). I have produced the geotiff raster data set using gdalwrap and gdal_translate in fi-FI system without anything else, in case this is an important piece of information.

An issue with fi_Fi when working with geotiff

QGISin versio 3.4.0-Madeira QGIS koodin versio 4a4b62e
Käännetty käyttäen Qt:tä 5.11.2 Suoritetaan käyttäen QT:tä 5.11.2
Käännetty käyttäen GDAL/OGR:ia 2.3.2 Suoritetaan käyttäen GDAL/OGR:ia 2.3.2
Käännetty käyttäen GEOS 3.7.0-CAPI-1.11.0 Suoritetaan käyttäen GEOSia 3.7.0-CAPI-1.11.0 673b9939
PostgreSQL Client Versio 9.2.4 SpatiaLite-versio 4.3.0
QWT Versio 6.1.3 QScintilla2 Versio 2.10.8
Käännetty käyttäen PROJ:ia 520 Suoritetaan käyttäen PROJ:ia 5.2.0

< edit: It appears I was too hasty. QGIS crashed and now that I've restarted with en-GB it appears to be working. I'm good to go, I believe. I leave this here in case someone finds this useful. :)

@elpaso
Copy link
Contributor Author

elpaso commented Nov 5, 2018

@veikkoeeva please file and issue and attach a sample project we can use to reproduce the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants