Version: Git (using Devel) OS: Linux I'm trying to look for the cause of calculator bug (https://bugs.kde.org/show_bug.cgi?id=281484). I found out this line KGlobal::locale()->readNumber(inputText); always return 0 for the input above 4 digits. My locale is US. Below are result using 12345 as input: - using readNumber it will return 0 - using QString::toDouble will return 12345 Reproducible: Always Steps to Reproduce: Open calculator plasmoid. Try input 12345, it will displayed 0 Actual Results: 0 is displayed. Expected Results: It must display 12,345 (in US locale) in string representation or 12345 in double (non 0 value)
I got the result by putting some kDebug line in calculator plasmoid. I tried displaying the content of currentValue. I compared the result of these: inputText="12345"; double currentValue = KGlobal::locale()->readNumber(inputText); with double currentValue = inputText.toDouble(); the first one return 0 while the second return 12345
Test which exposes the bug diff --git a/kdecore/tests/klocaletest.cpp b/kdecore/tests/klocaletest.cpp index 97a3bce..df6e454 100644 --- a/kdecore/tests/klocaletest.cpp +++ b/kdecore/tests/klocaletest.cpp @@ -159,6 +159,8 @@ KLocaleTest::readNumber() QVERIFY(ok); QCOMPARE(locale.readNumber("1.12345678912", &ok), 1.12345678912); QVERIFY(ok); + QCOMPARE(locale.readNumber("12345", &ok), 12345.0); + QVERIFY(ok); // Test Grouping locale.d->setNumericDigitGrouping(QList<int>()); Test fails: FAIL! : KLocaleTest::readNumber() Compared doubles are not the same (fuzzy compare) Actual (locale.readNumber("12345", &ok)): 0 Expected (12345.0): 12345 Loc: [/usr/local/kde-master-src/kdelibs/kdecore/tests/klocaletest.cpp(162)] This bug affects the Calculator plasmoid which can't get more than 4 digit numbers. When you enter 5 digits, the Calculator sets itself to 0
*** Bug 281484 has been marked as a duplicate of this bug. ***
The faulty code is in /kdelibs/kdecore/localization/klocale_kde.cpp in the QString KLocalePrivate::parseDigitGroup method. When you enter the 4th digit, the thousand separator is correctly added (like 1,234) but for the 5th digit this thousand separator does not move correctly to the left (should be 12,345) and something fails, setting a bool to false and then returning 0. This code is difficult to understand and fix for me and I'll seek help in trying to fix this in time for 4.8!
To start: thanks for identifying the real issue!!!! But does this really need to be so complicated? sscanf in libc has been doing this right for years, taking care of the locale. strtod too. And if you have a compliant C++ implementation, you can depend upon the existence of all the C library functions from C90. Sticking to C++, as mentioned QString::toDouble does the right thing. In fact, I suspect that the calculator plasmoid is almost the only user of this function, otherwise this bug would have been spotted much earlier. [A later note: apparently also Calligra, Kghostview and other stuff use it, so they may be all broken by now]. More seriously, I also strongly suspect that this comes from changes made to klocale as of 4.7. See the digit grouping stuff at http://osdir.com/ml/kde-i18n-doc/2011-05/msg00047.html. So maybe for 4.8 it can be enough to revert it.
Git commit 90b2ad6a7354ca4360fb993494e188ba661ae769 by Aaron Seigo. Committed on 16/12/2011 at 16:56. Pushed by aseigo into branch 'master'. work around KLocale::readNumber suddenly being much more strict :/ CCBUG:288963 M +6 -9 applets/calculator/calculator.cpp http://commits.kde.org/kdeplasma-addons/90b2ad6a7354ca4360fb993494e188ba661ae769
Thanks for tracking this down Anne-Marie, I'll work to fix this tomorrow. I suspect the bug was introduced with a patch I did during 4.7 to fix another bug rather than the original code.
Git commit 2993b24bc21a340695ad35b4f014a684f4d0c651 by John Layt. Committed on 17/12/2011 at 17:27. Pushed by jlayt into branch 'KDE/4.7'. KLocale: Fix readNumber() and readMoney() for lenient group parsing Restore the old pre-4.7 behaviour of accepting as valid any numbers that do not contain any group separators but strictly enforce group rules when the number contains 1 or more group separators. Distro's will really want to backport this fix to all versions of 4.7 as previously number entry for all KDE apps would have been seriously broken. I'm amazed I wasn't beaten up for this earlier! BUG: 288963 FIXED-IN: 4.7.5 CCMAIL: kde-packager@kde.org M +8 -2 kdecore/localization/klocale_kde.cpp M +29 -5 kdecore/tests/klocaletest.cpp http://commits.kde.org/kdelibs/2993b24bc21a340695ad35b4f014a684f4d0c651
Thanks a lot John!
> I'm amazed I wasn't beaten up for this earlier! I guess this shows that this function isn't used widely in KDE. Is there a way to find the places where it should be used, but isn't yet?
lxr.kde.org says readNumber() is used about 125 times, and readMoney() about 12 times, but they do appear to be used in utility classes in Calligra that could up the count. I suspect a few things made it less obvious, it only applied if you have group separators set and some countries don't use them, it only affected more than 3 digits, and some users may have typed the separators in anyway. I do wonder though how many apps are just using the Qt methods directly, but as they are called about 25,000 times according to lxr it would be impossible to know which are right uses and which are wrong. Ah well, come Qt5 we'll just use QLocale / QString anyway :-)
Hope this will not confuse/pollute the discussion. But from one of the initial reporters, many thanks for the upcoming fix!