> On Feb. 26, 2014, 10:57 p.m., Albert Astals Cid wrote:
> > have you tried removing the include?
>
> Albert Astals Cid wrote:
> Ignore me, there's i18n calls in there :D
>
> Alex Merry wrote:
> 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.
>
> Aurélien Gâteau wrote:
> 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());
>
> /**
>
>
> Matthieu Gallien wrote:
> Aurélien, do you want me to update the review request or do you do it
> directly yourself ?
>
> Albert Astals Cid wrote:
> 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.
>
> Aurélien Gâteau wrote:
> @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").
>
> Alex Merry wrote:
> 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().
>
> Aurélien Gâteau wrote:
> 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.
>
>
> Aurélien Gâteau wrote:
> 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.
>
> Alex Merry wrote:
> 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.
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.
- Aurélien
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/116103/#review50988
-----------------------------------------------------------
On Feb. 26, 2014, 10:44 p.m., Matthieu Gallien wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/116103/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2014, 10:44 p.m.)
>
>
> Review request for KDE Frameworks.
>
>
> Repository: kio
>
>
> Description
> -------
>
> include/KF5/KIOCore/kio/slavebase.h is including headers from KI18N and is
> publically installed.
>
>
> Diffs
> -----
>
> KF5KIOConfig.cmake.in 3a947b7
> src/core/CMakeLists.txt d897e37
>
> Diff: https://git.reviewboard.kde.org/r/116103/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Matthieu Gallien
>
>
_______________________________________________
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
