Review Request 116103: Make KI18N a public dependency of KIO since public installed headers of KIO requires it

[Aurélien Gâteau]

> 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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20140228/426c3da5/attachment.html>