Re: Review Request 116103: Make KI18N a public dependency of KIO since public installed headers of KIO requires it
> On March 4, 2014, 1:06 a.m., Aleix Pol Gonzalez wrote: > > Ok, I just realized this was being dealt with and I did a different patch: > > https://git.reviewboard.kde.org/r/116573/ > > > > I think that having UI strings on a header file is quite bad TBH, but since > > I see there's consensus I'll discard it. > > Alex Merry wrote: > Somewhat confusingly, the diff in this RR was not what was committed, but > instead the diff in the comments. See > http://commits.kde.org/kio/bf41a9aa94881db1d7f85c41aa35277c6e118574 My fault. I suggested Matthieu to do this, but that may not have been the best idea after all. - Aurélien --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116103/#review51844 --- On March 3, 2014, 9:59 p.m., Matthieu Gallien wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/116103/ > --- > > (Updated March 3, 2014, 9:59 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
Re: Review Request 116103: Make KI18N a public dependency of KIO since public installed headers of KIO requires it
> On March 4, 2014, 12:06 a.m., Aleix Pol Gonzalez wrote: > > Ok, I just realized this was being dealt with and I did a different patch: > > https://git.reviewboard.kde.org/r/116573/ > > > > I think that having UI strings on a header file is quite bad TBH, but since > > I see there's consensus I'll discard it. Somewhat confusingly, the diff in this RR was not what was committed, but instead the diff in the comments. See http://commits.kde.org/kio/bf41a9aa94881db1d7f85c41aa35277c6e118574 - Alex --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116103/#review51844 --- On March 3, 2014, 8:59 p.m., Matthieu Gallien wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/116103/ > --- > > (Updated March 3, 2014, 8:59 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
Re: Review Request 116103: Make KI18N a public dependency of KIO since public installed headers of KIO requires it
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116103/#review51844 --- Ok, I just realized this was being dealt with and I did a different patch: https://git.reviewboard.kde.org/r/116573/ I think that having UI strings on a header file is quite bad TBH, but since I see there's consensus I'll discard it. - Aleix Pol Gonzalez On March 3, 2014, 8:59 p.m., Matthieu Gallien wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/116103/ > --- > > (Updated March 3, 2014, 8:59 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
Re: Review Request 116103: Make KI18N a public dependency of KIO since public installed headers of KIO requires it
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116103/ --- (Updated March 3, 2014, 8:59 p.m.) Status -- This change has been marked as submitted. 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
Re: Review Request 116103: Make KI18N a public dependency of KIO since public installed headers of KIO requires it
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116103/#review51821 --- This review has been submitted with commit bf41a9aa94881db1d7f85c41aa35277c6e118574 by Matthieu Gallien to branch master. - Commit Hook 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 > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116103: Make KI18N a public dependency of KIO since public installed headers of KIO requires it
> 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 > #include > #include "job_base.h" // for KIO::JobFlags > -#include > > #include > #include > @@ -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
Re: Review Request 116103: Make KI18N a public dependency of KIO since public installed headers of KIO requires it
> 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 > #include > #include "job_base.h" // for KIO::JobFlags > -#include > > #include > #include > @@ -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)
Re: Review Request 116103: Make KI18N a public dependency of KIO since public installed headers of KIO requires it
> 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 > #include > #include "job_base.h" // for KIO::JobFlags > -#include > > #include > #include > @@ -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. > Actually, my patch was not complete, here is an updated one (using isNull() as suggested): http://paste.kde.org/punhyzdsd
Re: Review Request 116103: Make KI18N a public dependency of KIO since public installed headers of KIO requires it
> 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 > #include > #include "job_base.h" // for KIO::JobFlags > -#include > > #include > #include > @@ -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/#review5
Re: Review Request 116103: Make KI18N a public dependency of KIO since public installed headers of KIO requires it
> 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 > #include > #include "job_base.h" // for KIO::JobFlags > -#include > > #include > #include > @@ -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"). 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(). - 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
Re: Review Request 116103: Make KI18N a public dependency of KIO since public installed headers of KIO requires it
> 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 > #include > #include "job_base.h" // for KIO::JobFlags > -#include > > #include > #include > @@ -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. @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"). - 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
Re: Review Request 116103: Make KI18N a public dependency of KIO since public installed headers of KIO requires it
> 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 > #include > #include "job_base.h" // for KIO::JobFlags > -#include > > #include > #include > @@ -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 ? 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. - Albert --- 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 > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116103: Make KI18N a public dependency of KIO since public installed headers of KIO requires it
> 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 > #include > #include "job_base.h" // for KIO::JobFlags > -#include > > #include > #include > @@ -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()); > > /** > Aurélien, do you want me to update the review request or do you do it directly yourself ? - Matthieu --- 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
Re: Review Request 116103: Make KI18N a public dependency of KIO since public installed headers of KIO requires it
> 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. 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 #include #include "job_base.h" // for KIO::JobFlags -#include #include #include @@ -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()); /** - 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
Re: Review Request 116103: Make KI18N a public dependency of KIO since public installed headers of KIO requires it
> 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 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. - 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 > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116103: Make KI18N a public dependency of KIO since public installed headers of KIO requires it
> On Feb. 26, 2014, 9:57 p.m., Albert Astals Cid wrote: > > have you tried removing the include? Ignore me, there's i18n calls in there :D - Albert --- 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 > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116103: Make KI18N a public dependency of KIO since public installed headers of KIO requires it
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116103/#review50988 --- have you tried removing the include? - Albert Astals Cid 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 > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 116103: Make KI18N a public dependency of KIO since public installed headers of KIO requires it
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116103/ --- 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