> 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().
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
-----------------------------------------------------------
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
[email protected]
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
