D12164: Add overloads to Value/Unit::toString/toSymbolString taking a QLocale
Restricted Application edited subscribers, added: kde-frameworks-devel; removed: Frameworks. REPOSITORY R292 KUnitConversion REVISION DETAIL https://phabricator.kde.org/D12164 To: kossebau, ilic Cc: kde-frameworks-devel, huftis, broulik, michaelh, ngraham, bruns, #frameworks
D12164: Add overloads to Value/Unit::toString/toSymbolString taking a QLocale
aacid added a comment. The docu of ki18n says we localize numbers (as far as i remember), so that patch you discarded is "the right thing" imho REPOSITORY R292 KUnitConversion REVISION DETAIL https://phabricator.kde.org/D12164 To: kossebau, ilic Cc: aacid, kde-frameworks-devel, huftis, broulik, michaelh, ngraham, bruns
D12164: Add overloads to Value/Unit::toString/toSymbolString taking a QLocale
kossebau created this revision. kossebau added a reviewer: ilic. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. kossebau requested review of this revision. REVISION SUMMARY The existing toString/toSymbolString methods use KLocalizedString::subs(), which itself uses QString::arg() for creating the number. Which does not take any QLocale into account. The new overload method with a QLocale argument allow the caller full control over which locale is used. Sadly QLocale does not have a toString() method taking a fieldWidth parameter, so the overloads cannot offer that feature. TEST PLAN Unit tests still pass (need english system locale though, needs separate fix) REPOSITORY R292 KUnitConversion BRANCH addToStringWithLocale REVISION DETAIL https://phabricator.kde.org/D12164 AFFECTED FILES autotests/valuetest.cpp src/unit.cpp src/unit.h src/value.cpp src/value.h To: kossebau, ilic Cc: #frameworks, michaelh, ngraham, bruns
D12164: Add overloads to Value/Unit::toString/toSymbolString taking a QLocale
broulik added a comment. +1 always annoyed me that the converter runner didn't use localed decimal points See also https://git.reviewboard.kde.org/r/127800/ REPOSITORY R292 KUnitConversion REVISION DETAIL https://phabricator.kde.org/D12164 To: kossebau, ilic Cc: broulik, #frameworks, michaelh, ngraham, bruns
D12164: Add overloads to Value/Unit::toString/toSymbolString taking a QLocale
kossebau added a comment. As discussed on irc with @broulik will give that other patch https://git.reviewboard.kde.org/r/127800/ some look again, as I would agree numbers rather should be localized there. Had considered something like that before as well, but then discarded due to not sure if changing behaviour at age of kf 5.46 is good and also as https://doc.qt.io/qt-5/qstring.html#arg-8 hinted that without QLocale::setDefault() being called, which the average app seems not to do, still C locale would be used. Which might not be the complete truth in the docs, will give some testing tonight. REPOSITORY R292 KUnitConversion REVISION DETAIL https://phabricator.kde.org/D12164 To: kossebau, ilic Cc: broulik, #frameworks, michaelh, ngraham, bruns
D12164: Add overloads to Value/Unit::toString/toSymbolString taking a QLocale
kossebau abandoned this revision. kossebau added a comment. Sadly lost track of things, and no plans to look soon again at it, so cleaning up from stack for now. REPOSITORY R292 KUnitConversion REVISION DETAIL https://phabricator.kde.org/D12164 To: kossebau, ilic Cc: aacid, kde-frameworks-devel, huftis, broulik, LeGast00n, GB_2, michaelh, ngraham, bruns