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
broulik | |
aacid |
Localization | |
Frameworks |
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
Unit tests pass.
With US English locale, before:
With US English locale, after:
No Linters Available |
No Unit Test Coverage |
Buildable 13282 | |
Build 13300: arc lint + arc unit |
Still lacks the change in unit test in autotests/klocalizedstringtest.cpp, from " 4.20" to " 4,20".
You might want to test against different locales, like this: https://cgit.kde.org/ktimetracker.git/tree/src/tests/formattimetest.cpp
So I'm trying this:
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?
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; }
autotests/klocalizedstringtest.cpp | ||
---|---|---|
222 | 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). | |
226 | 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.
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 :)
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?
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)
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?
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.
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.
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.)
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
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
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.
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).
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).
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);
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.
numid is something we want to fix, we have a test for it, so if someone feels like working, ki18n is waiting for you :)