D22069: Localize long number strings

2019-10-24 Thread Albert Astals Cid
aacid added a comment. numid is something we want to fix, we have a test for it, so if someone feels like working, ki18n is waiting for you :) REPOSITORY R249 KI18n REVISION DETAIL https://phabricator.kde.org/D22069 To: ngraham, #localization, #frameworks, broulik, aacid Cc: flherne,

D22069: Localize long number strings

2019-10-24 Thread Karl Ove Hufthammer
huftis added a comment. In D22069#553556 , @huftis wrote: > In D22069#553527 , @huftis wrote: > > > For non-localised formatting of numbers, use QString::number() instead of QLocale::toString(), as

D22069: Localize long number strings

2019-10-24 Thread Karl Ove Hufthammer
huftis added a comment. In D22069#553527 , @huftis wrote: > 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.

D22069: Localize long number strings

2019-10-24 Thread Karl Ove Hufthammer
huftis added a comment. In D22069#553374 , @flherne wrote: > 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

D22069: Localize long number strings

2019-10-24 Thread Karl Ove Hufthammer
huftis added a comment. In D22069#553328 , @flherne wrote: > 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

D22069: Localize long number strings

2019-10-24 Thread Francis Herne
flherne added a comment. 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,

D22069: Localize long number strings

2019-10-24 Thread Nathaniel Graham
ngraham added a comment. Darn. :/ Is the problem only with years, or with other kinds of numbers too? Maybe we need a "don't localize this number" flag. REPOSITORY R249 KI18n REVISION DETAIL https://phabricator.kde.org/D22069 To: ngraham, #localization, #frameworks, broulik, aacid

D22069: Localize long number strings

2019-10-24 Thread Francis Herne
flherne added a comment. 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

D22069: Localize long number strings

2019-09-07 Thread Karl Ove Hufthammer
huftis added a comment. In D22069#526284 , @ngraham wrote: > 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

D22069: Localize long number strings

2019-09-05 Thread Nathaniel Graham
ngraham added a comment. In D22069#526291 , @aacid wrote: > 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

D22069: Localize long number strings

2019-09-05 Thread Albert Astals Cid
aacid added a comment. 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 REPOSITORY R249 KI18n REVISION DETAIL

D22069: Localize long number strings

2019-09-05 Thread Albert Astals Cid
aacid added a comment. In D22069#526284 , @ngraham wrote: > In D22069#526278 , @aacid wrote: > > > In D22069#526243 , @ngraham wrote: > > > > > That's

D22069: Localize long number strings

2019-09-05 Thread Nathaniel Graham
ngraham added a comment. In D22069#526278 , @aacid wrote: > In D22069#526243 , @ngraham wrote: > > > That's the same failure I have too, but @aacid said it was because my system was mis-configured.

D22069: Localize long number strings

2019-09-05 Thread Albert Astals Cid
aacid added a comment. In D22069#526243 , @ngraham wrote: > 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

D22069: Localize long number strings

2019-09-05 Thread Karl Ove Hufthammer
huftis added a comment. In D22069#526243 , @ngraham wrote: > 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

D22069: Localize long number strings

2019-09-05 Thread Nathaniel Graham
ngraham added a comment. 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. REPOSITORY R249 KI18n REVISION DETAIL https://phabricator.kde.org/D22069 To: ngraham, #localization, #frameworks,

D22069: Localize long number strings

2019-09-05 Thread David Faure
dfaure added a comment. 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()

D22069: Localize long number strings

2019-09-03 Thread Nathaniel Graham
This revision was automatically updated to reflect the committed changes. Closed by commit R249:b0dbb285bd21: Localize long number strings (authored by ngraham). REPOSITORY R249 KI18n CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22069?vs=65283=65351 REVISION DETAIL

D22069: Localize long number strings

2019-09-03 Thread Nathaniel Graham
ngraham edited the summary of this revision. REPOSITORY R249 KI18n BRANCH localized-long-number-strings (branched from master) REVISION DETAIL https://phabricator.kde.org/D22069 To: ngraham, #localization, #frameworks, broulik, aacid Cc: aacid, huftis, safaalfulaij, mikeroyal,

D22069: Localize long number strings

2019-09-03 Thread Albert Astals Cid
aacid accepted this revision. This revision is now accepted and ready to land. REPOSITORY R249 KI18n BRANCH localized-long-number-strings (branched from master) REVISION DETAIL https://phabricator.kde.org/D22069 To: ngraham, #localization, #frameworks, broulik, aacid Cc: aacid, huftis,

D22069: Localize long number strings

2019-09-02 Thread Nathaniel Graham
ngraham updated this revision to Diff 65283. ngraham added a comment. Correct test and don't run it if it can't succeed REPOSITORY R249 KI18n CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22069?vs=60622=65283 BRANCH localized-long-number-strings (branched from master)

D22069: Localize long number strings

2019-09-02 Thread Nathaniel Graham
ngraham marked 2 inline comments as done. REPOSITORY R249 KI18n REVISION DETAIL https://phabricator.kde.org/D22069 To: ngraham, #localization, #frameworks, broulik Cc: aacid, huftis, safaalfulaij, mikeroyal, aspotashev, ilic, kde-frameworks-devel, broulik, LeGast00n, GB_2, michaelh,

D22069: Localize long number strings

2019-09-02 Thread Nathaniel Graham
ngraham added a comment. Thanks @aacid! REPOSITORY R249 KI18n REVISION DETAIL https://phabricator.kde.org/D22069 To: ngraham, #localization, #frameworks, broulik Cc: aacid, huftis, safaalfulaij, mikeroyal, aspotashev, ilic, kde-frameworks-devel, broulik, LeGast00n, GB_2, michaelh,

D22069: Localize long number strings

2019-09-02 Thread Albert Astals Cid
aacid added a comment. 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

D22069: Localize long number strings

2019-09-01 Thread Nathaniel Graham
ngraham added a comment. In D22069#523824 , @aacid wrote: > 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

D22069: Localize long number strings

2019-09-01 Thread Albert Astals Cid
aacid added a comment. In D22069#523253 , @ngraham wrote: > 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

D22069: Localize long number strings

2019-09-01 Thread Karl Ove Hufthammer
huftis added a comment. In D22069#523253 , @ngraham wrote: > 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?

D22069: Localize long number strings

2019-08-31 Thread Nathaniel Graham
ngraham added a comment. I'm not sure I understand about about localization to figure that out. Would you be able to help me out? REPOSITORY R249 KI18n REVISION DETAIL https://phabricator.kde.org/D22069 To: ngraham, #localization, #frameworks, broulik Cc: safaalfulaij, mikeroyal,

D22069: Localize long number strings

2019-06-28 Thread Chusslove Illich
ilic added a comment. 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

D22069: Localize long number strings

2019-06-28 Thread Nathaniel Graham
ngraham added a comment. Ping! What is the path forward here? REPOSITORY R249 KI18n REVISION DETAIL https://phabricator.kde.org/D22069 To: ngraham, #localization, #frameworks, broulik Cc: safaalfulaij, mikeroyal, aspotashev, ilic, kde-frameworks-devel, broulik, LeGast00n, michaelh,

D22069: Localize long number strings

2019-06-25 Thread Safa Alfulaij
safaalfulaij added a comment. 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 :) REPOSITORY

D22069: Localize long number strings

2019-06-25 Thread Nathaniel Graham
ngraham added a comment. Yeah, I do. REPOSITORY R249 KI18n REVISION DETAIL https://phabricator.kde.org/D22069 To: ngraham, #localization, #frameworks, broulik Cc: mikeroyal, aspotashev, ilic, kde-frameworks-devel, broulik, LeGast00n, michaelh, ngraham, bruns

D22069: Localize long number strings

2019-06-25 Thread Chusslove Illich
ilic added a comment. 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?

D22069: Localize long number strings

2019-06-25 Thread Nathaniel Graham
ngraham added a comment. @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

D22069: Localize long number strings

2019-06-25 Thread Chusslove Illich
ilic added inline comments. INLINE COMMENTS > klocalizedstringtest.cpp:222 > + > +QLocale::setDefault(QLocale(QLocale::French)); > +QCOMPARE(ki18n("%1").subs(4.2, 5, 'f', 2).toString(), The French locale is already being set in line 45 using setlocale, for which the system has to have

D22069: Localize long number strings

2019-06-25 Thread Nathaniel Graham
ngraham edited the summary of this revision. REPOSITORY R249 KI18n REVISION DETAIL https://phabricator.kde.org/D22069 To: ngraham, #localization, #frameworks, broulik Cc: mikeroyal, aspotashev, ilic, kde-frameworks-devel, broulik, LeGast00n, michaelh, ngraham, bruns

D22069: Localize long number strings

2019-06-25 Thread Nathaniel Graham
ngraham added a comment. Thanks :) REPOSITORY R249 KI18n REVISION DETAIL https://phabricator.kde.org/D22069 To: ngraham, #localization, #frameworks, broulik Cc: mikeroyal, aspotashev, ilic, kde-frameworks-devel, broulik, LeGast00n, michaelh, ngraham, bruns

D22069: Localize long number strings

2019-06-24 Thread Mike Royal
mikeroyal added a comment. Nice work! :) REPOSITORY R249 KI18n REVISION DETAIL https://phabricator.kde.org/D22069 To: ngraham, #localization, #frameworks, broulik Cc: mikeroyal, aspotashev, ilic, kde-frameworks-devel, broulik, LeGast00n, michaelh, ngraham, bruns

D22069: Localize long number strings

2019-06-24 Thread Nathaniel Graham
ngraham updated this revision to Diff 60622. ngraham added a comment. Imagine that, a unit test exposing a bug in the code :) REPOSITORY R249 KI18n CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22069?vs=60562=60622 BRANCH localized-long-number-strings (branched from master)

D22069: Localize long number strings

2019-06-24 Thread Nathaniel Graham
ngraham added a comment. LOL REPOSITORY R249 KI18n REVISION DETAIL https://phabricator.kde.org/D22069 To: ngraham, #localization, #frameworks, broulik Cc: aspotashev, ilic, kde-frameworks-devel, broulik, LeGast00n, michaelh, ngraham, bruns

D22069: Localize long number strings

2019-06-24 Thread Alexander Potashev
aspotashev added a comment. 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 ---

D22069: Localize long number strings

2019-06-24 Thread Alexander Potashev
aspotashev added a comment. In D22069#485921 , @ngraham wrote: > Actual (ki18n("%1").subs(4.2, 5, 'f', 2).toString()): " 4.20" > Expected (QString(" 4,20")) : " 4,20" > > > What am I doing wrong?

D22069: Localize long number strings

2019-06-24 Thread Nathaniel Graham
ngraham added a comment. So I'm trying this: P421 localized number tests But it doesn't seem to work: FAIL! : KLocalizedStringTest::correctSubs() Compared values are not the same Actual (ki18n("%1").subs(4.2, 5, 'f',

D22069: Localize long number strings

2019-06-24 Thread Alexander Potashev
aspotashev added a comment. You might want to test against different locales, like this: https://cgit.kde.org/ktimetracker.git/tree/src/tests/formattimetest.cpp REPOSITORY R249 KI18n REVISION DETAIL https://phabricator.kde.org/D22069 To: ngraham, #localization, #frameworks, broulik Cc:

D22069: Localize long number strings

2019-06-24 Thread Nathaniel Graham
ngraham added a comment. The test passes for me, but I guess because I'm using English, right? REPOSITORY R249 KI18n REVISION DETAIL https://phabricator.kde.org/D22069 To: ngraham, #localization, #frameworks, broulik Cc: ilic, kde-frameworks-devel, broulik, LeGast00n, michaelh, ngraham,

D22069: Localize long number strings

2019-06-24 Thread Chusslove Illich
ilic added a comment. Still lacks the change in unit test in autotests/klocalizedstringtest.cpp, from " 4.20" to " 4,20". REPOSITORY R249 KI18n REVISION DETAIL https://phabricator.kde.org/D22069 To: ngraham, #localization, #frameworks, broulik Cc: ilic, kde-frameworks-devel, broulik,

D22069: Localize long number strings

2019-06-24 Thread Nathaniel Graham
ngraham created this revision. ngraham added reviewers: Localization, Frameworks, broulik. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. ngraham requested review of this revision. REVISION SUMMARY Localize long number strings so that they get the proper