Localize long number strings
ClosedPublic

Authored by ngraham on Jun 24 2019, 10:20 AM.

Details

Summary

Localize long number strings so that they get the proper locale-specific separator.

This is based on @broulik's old patch from https://git.reviewboard.kde.org/r/127800

BUG: 409077
FIXED-IN: 5.62

Test Plan

Unit tests pass.

With US English locale, before:

With US English locale, after:

Diff Detail

Repository
R249 KI18n
Branch
localized-long-number-strings (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 16029
Build 16047: arc lint + arc unit
ngraham created this revision.Jun 24 2019, 10:20 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptJun 24 2019, 10:20 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
ngraham requested review of this revision.Jun 24 2019, 10:20 AM
ilic added a subscriber: ilic.Jun 24 2019, 10:46 AM

Still lacks the change in unit test in autotests/klocalizedstringtest.cpp, from " 4.20" to " 4,20".

The test passes for me, but I guess because I'm using English, right?

So I'm trying this:

1diff --git a/autotests/klocalizedstringtest.cpp b/autotests/klocalizedstringtest.cpp
2index e68ec32..e1863c4 100644
3--- a/autotests/klocalizedstringtest.cpp
4+++ b/autotests/klocalizedstringtest.cpp
5@@ -218,6 +218,12 @@ void KLocalizedStringTest::correctSubs()
6 QString(" 42"));
7 QCOMPARE(ki18n("%1").subs(42, -5, 10, QChar('_')).toString(),
8 QString("42___"));
9+
10+ QLocale::setDefault(QLocale(QLocale::French));
11+ QCOMPARE(ki18n("%1").subs(4.2, 5, 'f', 2).toString(),
12+ QString(" 4,20"));
13+
14+ QLocale::setDefault(QLocale(QLocale::English, QLocale::UnitedStates));
15 QCOMPARE(ki18n("%1").subs(4.2, 5, 'f', 2).toString(),
16 QString(" 4.20"));
17 }

But it doesn't seem to work:

FAIL!  : KLocalizedStringTest::correctSubs() Compared values are not the same
   Actual   (ki18n("%1").subs(4.2, 5, 'f', 2).toString()): " 4.20"
   Expected (QString(" 4,20"))                           : " 4,20"
   Loc: [/home/nate/kde/src/ki18n/autotests/klocalizedstringtest.cpp(226)]

What am I doing wrong?

Actual   (ki18n("%1").subs(4.2, 5, 'f', 2).toString()): " 4.20"
Expected (QString(" 4,20"))                           : " 4,20"

What am I doing wrong?

Oops, I answered without looking at the code.

This subs() call in turn calls QString::arg() overload for double: https://doc.qt.io/qt-5/qstring.html#arg-9
I'm not sure how to interpret this part of Qt documentation, may be the authors meant that "%L1".subs(4.2, 5, 'f', 2) would produce locale-aware " 4.20"/" 4,20" and just "%1".subs(... 'f' ...) always uses a decimal point.

Dammit, I'm still not on the same page with the review request.

In fact you had to trust your unit test and go fix the code :)

diff --git a/src/klocalizedstring.cpp b/src/klocalizedstring.cpp
index b1ba745..fed5b8a 100644
--- a/src/klocalizedstring.cpp
+++ b/src/klocalizedstring.cpp
@@ -1198,7 +1198,7 @@ KLocalizedString KLocalizedString::subs(double a, int fieldWidth,
                                         QChar fillChar) const
 {
     KLocalizedString kls(*this);
-    kls.d->arguments.append(QStringLiteral("%1").arg(a, fieldWidth, format, precision, fillChar));
+    kls.d->arguments.append(QStringLiteral("%L1").arg(a, fieldWidth, format, precision, fillChar));
     kls.d->values.append(static_cast<realn>(a));
     return kls;
 }
ngraham updated this revision to Diff 60622.Jun 24 2019, 9:48 PM

Imagine that, a unit test exposing a bug in the code :)

ngraham edited the summary of this revision. (Show Details)Jun 25 2019, 7:31 AM
ilic added inline comments.Jun 25 2019, 8:01 AM
autotests/klocalizedstringtest.cpp
225

The French locale is already being set in line 45 using setlocale, for which the system has to have fr_FR.utf8 glibc locale installed. Any further setting up of locale details, if needed, should happen in lines 70-76, which are currently commented out with "until locale system is ready" (and if not needed, this whole commented out part should be deleted).

229

The tests work with French locale, there is no reason introduce English here only. It also clobbers the globaly set locale elements in initTestCase.

@ilic As far as I can tell, the locale needs to be set again to french because it's in a new function. At the end of the function, it doesn't matter that English was set as the default locale because the next test won't be re-using that state; it will revert to the global state.

If my understanding is incorrect, I'll need a bit more hand-holding to implement your requested changes.

ilic added a comment.Jun 25 2019, 9:47 AM

I'm not sure about what happens with global state, but at any rate, what I expected is that in klocalizedstringtest.cpp exactly one character is modified, replacing dot with comma in string " 4.20". Do you have fr_FR.utf8 glibc locale installed on your system?

This is great! Thanks!
Same issue in here: https://phabricator.kde.org/D13219
I'm not sure if this is the correct way of fixing it, but I think there isn't any other prvoided by Qt, other then QLocale::toString. Maybe it's the easy way here :)

Ping! What is the path forward here?

ilic added a comment.Jun 28 2019, 10:40 PM

The way forward is to find out why making that one dot-to-comma change and having fr_FR.utf8 glibc locale installed is not sufficient to get the test to pass. To my understanding, an explicit QLocale::setDefault() call either 1) should not be necessary, or 2) it should be somewhere in the source and not in the test.

I'm not sure I understand about about localization to figure that out. Would you be able to help me out?

huftis added a subscriber: huftis.Sep 1 2019, 6:13 PM

I'm not sure I understand about about localization to figure that out. Would you be able to help me out?

Perhaps https://phabricator.kde.org/D10757 could offer a clue?

aacid added a subscriber: aacid.Sep 1 2019, 6:56 PM

I'm not sure I understand about about localization to figure that out. Would you be able to help me out?

Figure *what* out?

Just remove this

diff --git a/autotests/klocalizedstringtest.cpp b/autotests/klocalizedstringtest.cpp
index e1863c4..d8ac31b 100644
--- a/autotests/klocalizedstringtest.cpp
+++ b/autotests/klocalizedstringtest.cpp
@@ -219,13 +219,8 @@ void KLocalizedStringTest::correctSubs()
     QCOMPARE(ki18n("%1").subs(42, -5, 10, QChar('_')).toString(),
              QString("42___"));
 
-    QLocale::setDefault(QLocale(QLocale::French));
     QCOMPARE(ki18n("%1").subs(4.2, 5, 'f', 2).toString(),
              QString(" 4,20"));
-
-    QLocale::setDefault(QLocale(QLocale::English, QLocale::UnitedStates));
-    QCOMPARE(ki18n("%1").subs(4.2, 5, 'f', 2).toString(),
-             QString(" 4.20"));

And that's it?

If what you're saying is that that you run ./bin/ki18n-klocalizedstringtest without those lines and the test fails for you, well you'll have to give me access to your machine since i can't fix what i can't debug (i.e. it works fine here)

If what you're saying is that that you run ./bin/ki18n-klocalizedstringtest without those lines and the test fails for you, well you'll have to give me access to your machine since i can't fix what i can't debug (i.e. it works fine here)

Yes that is exactly what I'm saying:

~/kde/src/ki18n$  (localized-long-number-strings) ../../build/ki18n/bin/ki18n-klocalizedstringtest 
********* Start testing of KLocalizedStringTest *********
Config: Using QtTest library 5.13.0, Qt 5.13.0 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 9.1.0)
PASS   : KLocalizedStringTest::initTestCase()
FAIL!  : KLocalizedStringTest::correctSubs() Compared values are not the same
   Actual   (ki18n("%1").subs(4.2, 5, 'f', 2).toString()): " 4.20"
   Expected (QString(" 4,20"))                           : " 4,20"
   Loc: [/home/nate/kde/src/ki18n/autotests/klocalizedstringtest.cpp(221)]
PASS   : KLocalizedStringTest::wrongSubs()
PASS   : KLocalizedStringTest::removeAcceleratorMarker()
PASS   : KLocalizedStringTest::miscMethods()
SKIP   : KLocalizedStringTest::translateToFrenchLowlevel() French test files not usable.
   Loc: [/home/nate/kde/src/ki18n/autotests/klocalizedstringtest.cpp(466)]
SKIP   : KLocalizedStringTest::translateToFrench() French test files not usable.
   Loc: [/home/nate/kde/src/ki18n/autotests/klocalizedstringtest.cpp(479)]
PASS   : KLocalizedStringTest::translateQt()
SKIP   : KLocalizedStringTest::addCustomDomainPath() French test files not usable.
   Loc: [/home/nate/kde/src/ki18n/autotests/klocalizedstringtest.cpp(536)]
SKIP   : KLocalizedStringTest::testThreads() French test files not usable.
   Loc: [/home/nate/kde/src/ki18n/autotests/klocalizedstringtest.cpp(479)]
FAIL!  : KLocalizedStringTest::testThreads() Compared values are not the same
   Actual   (ki18n("%1").subs(4.2, 5, 'f', 2).toString()): " 4.20"
   Expected (QString(" 4,20"))                           : " 4,20"
   Loc: [/home/nate/kde/src/ki18n/autotests/klocalizedstringtest.cpp(221)]
FAIL!  : KLocalizedStringTest::testThreads() Compared values are not the same
   Actual   (ki18n("%1").subs(4.2, 5, 'f', 2).toString()): " 4.20"
   Expected (QString(" 4,20"))                           : " 4,20"
   Loc: [/home/nate/kde/src/ki18n/autotests/klocalizedstringtest.cpp(221)]
FAIL!  : KLocalizedStringTest::testThreads() Compared values are not the same
   Actual   (ki18n("%1").subs(4.2, 5, 'f', 2).toString()): " 4.20"
   Expected (QString(" 4,20"))                           : " 4,20"
   Loc: [/home/nate/kde/src/ki18n/autotests/klocalizedstringtest.cpp(221)]
SKIP   : KLocalizedStringTest::testLocalizedTranslator() French test files not usable.
   Loc: [/home/nate/kde/src/ki18n/autotests/klocalizedstringtest.cpp(511)]
QWARN  : KLocalizedStringTest::semanticTags() "Tag 'numid' is not defined in message {<__kuit_internal_top__>Connecting to <numid>22</numid>...</__kuit_internal_top__...}."
XFAIL  : KLocalizedStringTest::semanticTags() what happened to <numid/>? TODO.
   Loc: [/home/nate/kde/src/ki18n/autotests/klocalizedstringtest.cpp(351)]
PASS   : KLocalizedStringTest::semanticTags()
SKIP   : KLocalizedStringTest::multipleLanguages() French or Catalan test files not usable.
   Loc: [/home/nate/kde/src/ki18n/autotests/klocalizedstringtest.cpp(550)]
PASS   : KLocalizedStringTest::cleanupTestCase()
Totals: 7 passed, 4 failed, 6 skipped, 0 blacklisted, 20ms
********* Finished testing of KLocalizedStringTest *********

Unfortunately I won't be able to make it to Akademy next week so I'm not sure when will be the next time for us to share a room.

If the code with your proposed removal looks good to you and you're confident that thee issue is a local mis-configuration on my side, can I land the patch?

aacid added a comment.Sep 2 2019, 6:20 AM

Other tests are telling you "French test files not usable."

You don't have the setup correct for the test to work.

You need to fix your setup for the test to pass, but first since you already have a "wrong" setup, please add the qskip like the other tests have.

ngraham updated this revision to Diff 65283.Sep 3 2019, 1:04 AM

Correct test and don't run it if it can't succeed

ngraham marked 2 inline comments as done.Sep 3 2019, 1:04 AM
aacid accepted this revision.Sep 3 2019, 5:55 PM
This revision is now accepted and ready to land.Sep 3 2019, 5:55 PM
ngraham edited the summary of this revision. (Show Details)Sep 3 2019, 6:07 PM
This revision was automatically updated to reflect the committed changes.
dfaure added a subscriber: dfaure.Sep 5 2019, 8:18 AM

This broke the unittest in CI.
https://build.kde.org/job/Frameworks/job/ki18n/job/kf5-qt5%20SUSEQt5.12/35/testReport/junit/projectroot/autotests/ki18n_klocalizedstringtest/

I can reproduce the failure locally.

FAIL!  : KLocalizedStringTest::correctSubs() Compared values are not the same
   Actual   (ki18n("%1").subs(4.2, 5, 'f', 2).toString()): " 4.20"
   Expected (QString(" 4,20"))                           : " 4,20"
   Loc: [/d/kde/src/5/frameworks/ki18n/autotests/klocalizedstringtest.cpp(225)]

That's the same failure I have too, but @aacid said it was because my system was mis-configured. I don't really understand the issue, I'm afraid.

huftis added a comment.Sep 5 2019, 5:11 PM

That's the same failure I have too, but @aacid said it was because my system was mis-configured. I don't really understand the issue, I'm afraid.

On my system, the test passes with my default locale, but fails (with the same error) if I have LC_ALL set to English:

LC_ALL=en_US.utf8 ./ki18n-klocalizedstringtest

Shouldn’t the test override the LC_ALL settings? (I have export LC_ALL=nn_NO.utf8 (along with similar commands for LANG and LANGUAGE) in my .bashrc to ensure that I get all applications in my language.)

aacid added a comment.Sep 5 2019, 5:14 PM

That's the same failure I have too, but @aacid said it was because my system was mis-configured. I don't really understand the issue, I'm afraid.

Are you really saying that when i said "this is because you don't have the french stuff installed" you didn't install it to check i was right and you just trusted me?

That's the same failure I have too, but @aacid said it was because my system was mis-configured. I don't really understand the issue, I'm afraid.

Are you really saying that when i said "this is because you don't have the french stuff installed" you didn't install it to check i was right and you just trusted me?

Yes, I interpreted your "Accepted" status to mean that you had tested the patch yourself in your capacity as the patch reviewer and had concluded that it worked. My apologies if I misinterpreted this and it was not in fact the case.

I actually don't know how to properly install other language stuff. When I add French in the languages KCM, I suffer from https://bugs.kde.org/show_bug.cgi?id=327757.

What is the correct way to install the language?

aacid added a comment.Sep 5 2019, 5:31 PM

That's the same failure I have too, but @aacid said it was because my system was mis-configured. I don't really understand the issue, I'm afraid.

Are you really saying that when i said "this is because you don't have the french stuff installed" you didn't install it to check i was right and you just trusted me?

Yes, I interpreted your "Accepted" status to mean that you had tested the patch yourself in your capacity as the patch reviewer and had concluded that it worked. My apologies if I misinterpreted this and it was not in fact the case.

I actually don't know how to properly install other language stuff. When I add French in the languages KCM, I suffer from https://bugs.kde.org/show_bug.cgi?id=327757.

What is the correct way to install the language?

On archlinux you uncomment the language you want in /etc/locale.gen and then run locale-gen

aacid added a comment.Sep 5 2019, 5:33 PM

But i guess @huftis is right, the test is not setting the language "enough" to be french, just "a bit" (there's too many layers of stuff to set) and it just works for me because i'm also in a locale tht uses , like the french

But i guess @huftis is right, the test is not setting the language "enough" to be french, just "a bit" (there's too many layers of stuff to set) and it just works for me because i'm also in a locale tht uses , like the french

Apologies for my ignorance but how do I do that?

huftis added a comment.Sep 7 2019, 4:28 PM

I actually don't know how to properly install other language stuff. When I add French in the languages KCM, I suffer from https://bugs.kde.org/show_bug.cgi?id=327757.

What is the correct way to install the language?

I think this problem is just because you have installed a new version of ki18n (libKF5I18n.so.*). I get it too (also when compiling and installing versions of ki18n *before* the patch), so I guess the applications (or some libraries they depend on) have to be recompiled after updating ki18n.

I'm afraid this was a breaking API change -- it changes the behaviour, and not all numbers are intended to have separators added in this way.

See https://bugs.kde.org/show_bug.cgi?id=413390

A similar pattern occurs three different times just in KDevelop, and I can't even see how to work around this without breaking translations.

Darn. :/

Is the problem only with years, or with other kinds of numbers too? Maybe we need a "don't localize this number" flag.

<TeleFuchs> on IRC pointed out that this could affect telephone numbers.

Also: numeric IDs [I bet at least something prints PIDs this way].

I don't think it makes sense to try and enumerate specific cases -- this needs a flag to determine whether localization is done, and at least for KF5 the default must be "no" to avoid breaking compatibility.

I'm afraid this was a breaking API change -- it changes the behaviour, and not all numbers are intended to have separators added in this way.

See https://bugs.kde.org/show_bug.cgi?id=413390

The breaking API change happened earlier. This commit *fixed* it. Localised number formatting *used* to work on earlier versions of KDE (which is what KF5/Plasma/… was called then), but broke with the switch from KLocale to Qlocale.

A similar pattern occurs three different times just in KDevelop, and I can't even see how to work around this without breaking translations.

For non-localised formatting of numbers, use QString::number() instead of QLocale::toString(), as detailed in https://api.kde.org/frameworks/ki18n/html/prg_guide.html#subs_notes. Note that this is not KDE-specific; it’s true for all Qt applications (KF5 just had a bug which made the user’s locale not being used).

<TeleFuchs> on IRC pointed out that this could affect telephone numbers.

Telephone numbers are not numbers, and should never be represented internally as numbers. If they are, then that’s a bug, that will leads to all sorts of problems. For proper representation and presentation of telephone numbers, see RFC 3966 (https://tools.ietf.org/html/rfc3966) and ITU E.123 (https://www.itu.int/rec/T-REC-E.123-200102-I/en).

For non-localised formatting of numbers, use QString::number() instead of QLocale::toString(), as detailed in https://api.kde.org/frameworks/ki18n/html/prg_guide.html#subs_notes.

I think I used the wrong link there. This one is more up to date and useful: https://techbase.kde.org/Development/Tutorials/Localization/i18n_Semantics

The relevant quote is:

<numid>
By default, numbers supplied as arguments to i18n calls are formatted into localized form. If the number is supposed to be a numeric identifier instead, like a port number, use this tag to signal numeric-id environment.
i18nc("@info:progress",
      "Connecting to <numid>%1</numid>...", portNo);

For non-localised formatting of numbers, use QString::number() instead of QLocale::toString(), as detailed in https://api.kde.org/frameworks/ki18n/html/prg_guide.html#subs_notes.

I think I used the wrong link there. This one is more up to date and useful: https://techbase.kde.org/Development/Tutorials/Localization/i18n_Semantics

According to https://bugs.kde.org/show_bug.cgi?id=413390, the first link is the most up to date one. So use QString::number() instead of <numid>%1</numid> (which doesn’t work anymore). Not as elegant, perhaps, but it has the added advantage that it doesn’t break string freeze.

aacid added a comment.Oct 24 2019, 6:02 PM

numid is something we want to fix, we have a test for it, so if someone feels like working, ki18n is waiting for you :)