<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/116103/">https://git.reviewboard.kde.org/r/116103/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 26th, 2014, 10:57 p.m. CET, <b>Albert Astals Cid</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;">have you tried removing the include?</pre>
</blockquote>
<p>On February 26th, 2014, 11:53 p.m. CET, <b>Albert Astals Cid</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;">Ignore me, there's i18n calls in there :D</pre>
</blockquote>
<p>On February 27th, 2014, 12:32 a.m. CET, <b>Alex Merry</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;">However, I wonder if those calls should really be in the header. I have no idea what catalogue they will use at runtime; I suspect it will depend on whether code that includes the header defined TRANSLATION_DOMAIN and where, exactly, they do so.
A better solution (SC but not BC) is probably to create overloaded methods and put those calls in the cpp file.</pre>
</blockquote>
<p>On February 27th, 2014, 2:57 p.m. CET, <b>Aurélien Gâteau</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;">May I suggest this instead:
diff --git a/src/core/slavebase.cpp b/src/core/slavebase.cpp
index 1236ad5..2002cdf 100644
--- a/src/core/slavebase.cpp
+++ b/src/core/slavebase.cpp
@@ -968,9 +968,11 @@ int SlaveBase::messageBox(MessageBoxType type, const QString &text, const QStrin
}
int SlaveBase::messageBox(const QString &text, MessageBoxType type, const QString &caption,
- const QString &buttonYes, const QString &buttonNo,
+ const QString &_buttonYes, const QString &_buttonNo,
const QString &dontAskAgainName)
{
+ QString buttonYes = _buttonYes.isEmpty() ? i18n("&Yes") : _buttonYes;
+ QString buttonNo = _buttonNo.isEmpty() ? i18n("&No") : _buttonNo;
//qDebug() << "messageBox " << type << " " << text << " - " << caption << buttonYes << buttonNo;
KIO_DATA << (qint32)type << text << caption << buttonYes << buttonNo << dontAskAgainName;
send(INF_MESSAGEBOX, data);
diff --git a/src/core/slavebase.h b/src/core/slavebase.h
index 86f1506..0922893 100644
--- a/src/core/slavebase.h
+++ b/src/core/slavebase.h
@@ -24,7 +24,6 @@
#include <kio/udsentry.h>
#include <kio/authinfo.h>
#include "job_base.h" // for KIO::JobFlags
-#include <klocalizedstring.h>
#include <QtCore/QByteArray>
#include <QtNetwork/QHostInfo>
@@ -277,8 +276,8 @@ public:
*/
int messageBox(MessageBoxType type, const QString &text,
const QString &caption = QString(),
- const QString &buttonYes = i18n("&Yes"),
- const QString &buttonNo = i18n("&No"));
+ const QString &buttonYes = QString(),
+ const QString &buttonNo = QString());
/**
* Call this to show a message box from the slave
@@ -297,8 +296,8 @@ public:
*/
int messageBox(const QString &text, MessageBoxType type,
const QString &caption = QString(),
- const QString &buttonYes = i18n("&Yes"),
- const QString &buttonNo = i18n("&No"),
+ const QString &buttonYes = QString(),
+ const QString &buttonNo = QString(),
const QString &dontAskAgainName = QString());
/**
</pre>
</blockquote>
<p>On February 27th, 2014, 8:57 p.m. CET, <b>Matthieu Gallien</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;">Aurélien, do you want me to update the review request or do you do it directly yourself ?</pre>
</blockquote>
<p>On February 27th, 2014, 11:32 p.m. CET, <b>Albert Astals Cid</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;">If we go Aurélien's way i'd document that QString() means Yes/No and not an empty string in there (which is actually be runtime incompatible with kde4.x but i think that's fair and not much people would be passing "" there to get a button with no text)
Moreover we could go with isNull instead of isEmpty so that actually passing "" does give you an empty button and QString() gives you yes/no.</pre>
</blockquote>
<p>On February 28th, 2014, 12:28 a.m. CET, <b>Aurélien Gâteau</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;">@Matthieu: Feel free to update the review request, that's less work.
@Albert: The doc for the methods actually already says it the default values is i18n("&Yes") and i18n("&No").</pre>
</blockquote>
<p>On February 28th, 2014, 2:01 a.m. CET, <b>Alex Merry</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;">Albert is right: the "default value" is not the same as "an empty string". You'll be changing the result of
messageBox(type, "blah", "some caption", "", "");
Not that this is a sensible call to make, but it should be documented.
I also agree with Albert that isNull() is a better check than isEmpty().</pre>
</blockquote>
<p>On February 28th, 2014, 8:34 a.m. CET, <b>Aurélien Gâteau</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;">I don't mind if the code uses isNull() instead of isEmpty(), but documenting this sounds wrong.
Academically that make sense, but it really is not pragmatic. What would the documentation look like? Something like this: "If you want to create empty buttons, pass "" to buttonYes and buttonNo."? The best API are the ones which are impossible to use wrong. As such I strongly advice against documenting how to achieve such a broken result, unless you can find a valid reason why one would want to create empty buttons.
</pre>
</blockquote>
<p>On February 28th, 2014, 10:55 a.m. CET, <b>Aurélien Gâteau</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;">Actually, my patch was not complete, here is an updated one (using isNull() as suggested): http://paste.kde.org/punhyzdsd
That brings the question of SC: some code may fail to build because it did not include klocalizedstring.h, instead relying on slavebase.h bringing it in. I think such code was wrong and needs to be fixed anyway, but others may disagree. I am rebuilding the rest of Plasma Workspace 2 with the patch in to evaluate the importance of the breakage.</pre>
</blockquote>
<p>On February 28th, 2014, 11:58 a.m. CET, <b>Alex Merry</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;">The documentation I was thinking of was something like "An argument of QString() for @p buttonYes or @p buttonNo (the default) will cause i18n("&Yes") and i18n("&No") to be used, respectively.".
Having thought about it, I don't think allowing empty strings for the buttons is helpful, so isEmpty() would be fine as well.</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;">Rebuild is done (all frameworks + kde-runtime, kde-workspace, libnm-qt, plasma-nm, konsole, kate). I only had to add includes to two files in kde-runtime, which I already committed. I think it is safe to commit.
Regarding documentation, it says this:
* @param buttonYes The text for the first button.
* The default is i18n("&Yes").
* @param buttonNo The text for the second button.
* The default is i18n("&No").
Which I think is fine.
@Matthieu: feel free to commit my changes. If you are not comfortable with this, discard this request and I'll file a new one.</pre>
<br />
<p>- Aurélien</p>
<br />
<p>On February 26th, 2014, 10:44 p.m. CET, Matthieu Gallien 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 KDE Frameworks.</div>
<div>By Matthieu Gallien.</div>
<p style="color: grey;"><i>Updated Feb. 26, 2014, 10:44 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;">include/KF5/KIOCore/kio/slavebase.h is including headers from KI18N and is publically installed.</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>KF5KIOConfig.cmake.in <span style="color: grey">(3a947b7)</span></li>
<li>src/core/CMakeLists.txt <span style="color: grey">(d897e37)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/116103/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>