[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-core-devel
Subject: Re: Review Request 114889: Fix KIO::convertSize(...) returning string with "(I18N_ARGUMENT_MISSING)"
From: "Chusslove Illich" <caslav.ilic () gmx ! net>
Date: 2014-01-07 9:30:18
Message-ID: 20140107093018.7621.50977 () probe ! kde ! org
[Download RAW message or body]
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/114889/#review46964
-----------------------------------------------------------
This code was originally like this (more or less), but someone complained
about performance of unit formatting. Then the code was switched away from
normal i18n calls, to use an internal low-level method of KLocale that \
would fetch translation only once and cache it, later only substituting the
argument. When splitting into frameworks, this internal method was no \
longer available, so the quick switch back to normal i18n left it in this \
semi- correct state. The question now is whether to use correct i18n, as in \
your patch, or to go back to a caching solution.
Without that low-level method, the caching solution would go by \
substituting right away the literal "%1" as the argument (e.g.
i18nc("size in 10^6 bytes", "%1 MB", QString::fromLatin1("%1"));), caching
such translation, and subsequently using .arg on the cached translations
(similarly to that in klocale_kde.cpp in KDE 4).
- Chusslove Illich
On Jan. 6, 2014, 8 p.m., Friedrich W. H. Kossebau wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/114889/
> -----------------------------------------------------------
>
> (Updated Jan. 6, 2014, 8 p.m.)
>
>
> Review request for kdelibs.
>
>
> Repository: kio
>
>
> Description
> -------
>
> Seems the code behind i18n() in kf5 does not like %-placeholders without \
> any arguments passed in the i18n call. And thus inserts the warning. \
> (Effect can be seen e.g. in Okteta's File Info tool).
> Attached patch fixes that, by delaying the argument substitution as \
> proposed in http://api.kde.org/frameworks-5.x-api/frameworks-apidocs/ki18n/html/prg_guide.html#spec_usage
>
>
> Diffs
> -----
>
> src/core/global.cpp 42f453b
>
> Diff: https://git.reviewboard.kde.org/r/114889/diff/
>
>
> Testing
> -------
>
> Results of KIO::convertSize(...) displays fine in Okteta with the patch.
>
>
> Thanks,
>
> Friedrich W. H. Kossebau
>
>
[Attachment #3 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px \
#c9c399 solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/114889/">https://git.reviewboard.kde.org/r/114889/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">This code was originally like this (more or less), but someone \
complained about performance of unit formatting. Then the code was switched \
away from normal i18n calls, to use an internal low-level method of KLocale \
that would fetch translation only once and cache it, later only \
substituting the argument. When splitting into frameworks, this internal \
method was no longer available, so the quick switch back to normal i18n \
left it in this semi- correct state. The question now is whether to use \
correct i18n, as in your patch, or to go back to a caching solution.
Without that low-level method, the caching solution would go by \
substituting right away the literal "%1" as the argument (e.g.
i18nc("size in 10^6 bytes", "%1 MB", \
QString::fromLatin1("%1"));), caching such translation, and \
subsequently using .arg on the cached translations (similarly to that in \
klocale_kde.cpp in KDE 4). </pre>
<br />
<p>- Chusslove Illich</p>
<br />
<p>On January 6th, 2014, 8 p.m. CET, Friedrich W. H. Kossebau wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px \
black solid;"> <tr>
<td>
<div>Review request for kdelibs.</div>
<div>By Friedrich W. H. Kossebau.</div>
<p style="color: grey;"><i>Updated Jan. 6, 2014, 8 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kio
</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;">Seems the code behind i18n() in kf5 does not like \
%-placeholders without any arguments passed in the i18n call. And thus \
inserts the warning. (Effect can be seen e.g. in Okteta's File Info \
tool).
Attached patch fixes that, by delaying the argument substitution as \
proposed in http://api.kde.org/frameworks-5.x-api/frameworks-apidocs/ki18n/html/prg_guide.html#spec_usage</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;">Results of KIO::convertSize(...) displays fine in Okteta with \
the patch.</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/core/global.cpp <span style="color: grey">(42f453b)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/114889/diff/" \
style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic