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

Alex Merry alex.merry at kde.org
Fri Feb 28 10:58:52 UTC 2014



> On Feb. 26, 2014, 9: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.

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.


- Alex


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/116103/#review50988
-----------------------------------------------------------


On Feb. 26, 2014, 9: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, 9: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/f849b214/attachment.html>


More information about the Kde-frameworks-devel mailing list