[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-frameworks-devel
Subject: Re: Review Request 127800: Format number in KLocalizedString::subs
From: Chusslove Illich <caslav.ilic () gmx ! net>
Date: 2017-02-11 17:49:07
Message-ID: 20170211174907.21138.92634 () mimi ! kde ! org
[Download RAW message or body]
--===============3220958510124418229==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
> On Мај 1, 2016, 6:40 по п., Chusslove Illich wrote:
> > To have localized numbers is the expected behavior (and documented as such). But \
> > it was "temporarily" disabled between kdelibs and KF5, until replacements for \
> > KLocale were decided. I don't remember why I didn't switch to %L1 at that moment, \
> > since it was available too... Well, I guess best to commit this and see later if \
> > there is any trouble.
> > Did you run unit tests? Could be some of them needs adapting.
>
> Kai Uwe Broulik wrote:
> The unit tests fail (2 out of 4) locally both with and without this patch.
>
> Chusslove Illich wrote:
> Hm, and which are these failing tests? When I run them, they all pass (including \
> expected fails).
> With the patch, I had to change in autotests/klocalizedstringtest.cpp one \
> comparison literal from " 4.20" to " 4,20".
> Kai Uwe Broulik wrote:
> Ah, they pass, but only if I run them with LANG=en_US
>
> ```
> FAIL! : KI18nDeclarativeTest::testLocalizedContext(plural translation with domain) \
> Compared values are not the same Actual \
> (object->property(propertyName.toUtf8().constData()).toString()): "In 3 Sekunden" \
> Expected (value) : "in 3 \
> seconds"
> Loc: [/home/kaiuwe/Projekte/kf5/src/frameworks/ki18n/autotests/ki18ndeclarativetest.cpp(60)]
>
> FAIL! : KLocalizedStringTest::semanticTags() Compared values are not the same
> Actual (xi18nc("@info", "Apply color scheme <resource>%1</resource>?", "XXX")): \
> "<html>Apply color scheme \u201EXXX\u201C?</html>" Expected (QString("<html>Apply \
> color scheme "XXX"?</html>")) : "<html>Apply color scheme \
> \u201CXXX\u201D?</html>"
> Loc: [/home/kaiuwe/Projekte/kf5/src/frameworks/ki18n/autotests/klocalizedstringtest.cpp(351)]
> ```
>
> Chusslove Illich wrote:
> Ok, there's something strange with those two failing tests, that has nothing to do \
> with the current patch.
> But then what is also strange is that with patched version you don't get a failure \
> on that number format test. Maybe you don't have locale fr_FR.UTF-8 generated on \
> the system, and then it falls back to C locale.
> Anyway, then just make that change in literals (" 4.20" to " 4,20"), and we call it \
> good. (Even if then you should get failure on that test :)
Hm, wasn't the only thing missing here the modification of the unit test?
- Chusslove
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127800/#review95056
-----------------------------------------------------------
On Феб. 8, 2017, 3:10 по п., Kai Uwe Broulik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127800/
> -----------------------------------------------------------
>
> (Updated Феб. 8, 2017, 3:10 по п.)
>
>
> Review request for KDE Frameworks.
>
>
> Repository: ki18n
>
>
> Description
> -------
>
> Use %L1 instead of %1 which tells it it's a number.
>
> This way I get decimal indicator and thousands separators according to my locale in \
> eg. KUnitConversion
>
> Diffs
> -----
>
> src/klocalizedstring.cpp fc80135
>
> Diff: https://git.reviewboard.kde.org/r/127800/diff/
>
>
> Testing
> -------
>
> 0,00094697205065430554 Meilen and 1.524 Millimeter
> instead of
> 0.00094697205065430554 Meilen and 1524 Millimeter
>
>
> Thanks,
>
> Kai Uwe Broulik
>
>
--===============3220958510124418229==
MIME-Version: 1.0
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: 8bit
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 \
solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;"> \
<tr> <td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/127800/">https://git.reviewboard.kde.org/r/127800/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On мај 1st, 2016, 6:40 по п. CEST, \
<b>Chusslove Illich</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: \
2px solid #d0d0d0; padding-left: 10px;"> <pre style="white-space: pre-wrap; \
white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; \
word-wrap: break-word;">To have localized numbers is the expected behavior (and \
documented as such). But it was "temporarily" disabled between kdelibs and \
KF5, until replacements for KLocale were decided. I don't remember why I \
didn't switch to %L1 at that moment, since it was available too... Well, I guess \
best to commit this and see later if there is any trouble.
Did you run unit tests? Could be some of them needs adapting.</pre>
</blockquote>
<p>On мај 1st, 2016, 6:42 по п. CEST, <b>Kai Uwe Broulik</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The \
unit tests fail (2 out of 4) locally both with and without this patch.</p></pre> \
</blockquote>
<p>On мај 1st, 2016, 7:14 по п. CEST, <b>Chusslove Illich</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Hm, and which are these \
failing tests? When I run them, they all pass (including expected fails).
With the patch, I had to change in autotests/klocalizedstringtest.cpp one comparison \
literal from " 4.20" to " 4,20".</pre> </blockquote>
<p>On мај 1st, 2016, 7:18 по п. CEST, <b>Kai Uwe Broulik</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Ah, \
they pass, but only if I run them with LANG=en_US</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div \
class="codehilite" style="background: #f8f8f8"><pre style="line-height: \
125%"><span></span>FAIL! : KI18nDeclarativeTest::testLocalizedContext(plural \
translation with domain) Compared values are not the same Actual \
(object->property(propertyName.toUtf8().constData()).toString()): "In 3 \
Sekunden" Expected (value) \
: "in 3 seconds" Loc: \
[/home/kaiuwe/Projekte/kf5/src/frameworks/ki18n/autotests/ki18ndeclarativetest.cpp(60)]
FAIL! : KLocalizedStringTest::semanticTags() Compared values are not the same
Actual (xi18nc("@info", "Apply color scheme <span style="color: \
#008000; font-weight: bold"><resource></span>%1<span style="color: #008000; \
font-weight: bold"></resource></span>?", "XXX")): "<span \
style="color: #008000; font-weight: bold"><html></span>Apply color scheme \
\u201EXXX\u201C?<span style="color: #008000; font-weight: \
bold"></html></span>" Expected (QString("<span style="color: \
#008000; font-weight: bold"><html></span>Apply color scheme "XXX"?<span \
style="color: #008000; font-weight: bold"></html></span>")) \
: "<span style="color: #008000; font-weight: bold"><html></span>Apply \
color scheme \u201CXXX\u201D?<span style="color: #008000; font-weight: \
bold"></html></span>" Loc: \
[/home/kaiuwe/Projekte/kf5/src/frameworks/ki18n/autotests/klocalizedstringtest.cpp(351)]
</pre></div>
</p></pre>
</blockquote>
<p>On мај 1st, 2016, 7:29 по п. CEST, <b>Chusslove Illich</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Ok, there's \
something strange with those two failing tests, that has nothing to do with the \
current patch.
But then what is also strange is that with patched version you don't get a \
failure on that number format test. Maybe you don't have locale fr_FR.UTF-8 \
generated on the system, and then it falls back to C locale.
Anyway, then just make that change in literals (" 4.20" to " \
4,20"), and we call it good. (Even if then you should get failure on that test \
:)</pre> </blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Hm, \
wasn't the only thing missing here the modification of the unit test?</p></pre> <br \
/>
<p>- Chusslove</p>
<br />
<p>On фебруар 8th, 2017, 3:10 по п. CET, Kai Uwe Broulik wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: \
1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; \
-webkit-border-radius: 6px;"> <tr>
<td>
<div>Review request for KDE Frameworks.</div>
<div>By Kai Uwe Broulik.</div>
<p style="color: grey;"><i>Updated Феб. 8, 2017, 3:10 по п.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
ki18n
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">Use %L1 instead of %1 which tells it it's a \
number.</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">This way I get decimal indicator and thousands \
separators according to my locale in eg. KUnitConversion</p></pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">0,00094697205065430554 Meilen and 1.524 Millimeter \
instead of 0.00094697205065430554 Meilen and 1524 Millimeter</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/klocalizedstring.cpp <span style="color: grey">(fc80135)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/127800/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
--===============3220958510124418229==--
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic