D26890: QXmlInputSource is deprecated in qt5.15. Port it to QXmlStreamReader
kossebau added inline comments. INLINE COMMENTS > kconfigloader.cpp:87 > + > +if (reader.isEndDocument()) > +return false; This logic is inverted., no? We should be at token document end when reader.atEnd() turns true. In any case on successful parse this condition is met here and false returned. No caller seems to check the return value of ConfigLoaderHandler::parse(), thus nobody noticed? REPOSITORY R237 KConfig REVISION DETAIL https://phabricator.kde.org/D26890 To: mlaurent, dfaure, apol Cc: kossebau, apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D26890: QXmlInputSource is deprecated in qt5.15. Port it to QXmlStreamReader
This revision was automatically updated to reflect the committed changes. Closed by commit R237:87471e14b370: QXmlInputSource is deprecated in qt5.15. Port it to QXmlStreamReader (authored by mlaurent). REPOSITORY R237 KConfig CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26890?vs=74428=74460 REVISION DETAIL https://phabricator.kde.org/D26890 AFFECTED FILES src/gui/kconfigloader.cpp src/gui/kconfigloaderhandler_p.h To: mlaurent, dfaure, apol Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26890: QXmlInputSource is deprecated in qt5.15. Port it to QXmlStreamReader
apol accepted this revision. This revision is now accepted and ready to land. REPOSITORY R237 KConfig BRANCH port_QXmlStreamReader (branched from master) REVISION DETAIL https://phabricator.kde.org/D26890 To: mlaurent, dfaure, apol Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26890: QXmlInputSource is deprecated in qt5.15. Port it to QXmlStreamReader
mlaurent updated this revision to Diff 74428. mlaurent added a comment. Use QStringRef here too REPOSITORY R237 KConfig CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26890?vs=74406=74428 BRANCH port_QXmlStreamReader (branched from master) REVISION DETAIL https://phabricator.kde.org/D26890 AFFECTED FILES src/gui/kconfigloader.cpp src/gui/kconfigloaderhandler_p.h To: mlaurent, dfaure Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26890: QXmlInputSource is deprecated in qt5.15. Port it to QXmlStreamReader
apol added inline comments.
INLINE COMMENTS
> kconfigloader.cpp:125
> for (int i = 0; i < numAttrs; ++i) {
> -QString name = attrs.localName(i).toLower();
> +const QString name = attrs.at(i).name().toString().toLower();
> if (name == QLatin1String("name")) {
You can use QStringRef::compare() in this if sequence over here too.
REPOSITORY
R237 KConfig
REVISION DETAIL
https://phabricator.kde.org/D26890
To: mlaurent, dfaure
Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26890: QXmlInputSource is deprecated in qt5.15. Port it to QXmlStreamReader
mlaurent updated this revision to Diff 74406. mlaurent added a comment. Use QStringRef::compare REPOSITORY R237 KConfig CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26890?vs=74349=74406 BRANCH port_QXmlStreamReader (branched from master) REVISION DETAIL https://phabricator.kde.org/D26890 AFFECTED FILES src/gui/kconfigloader.cpp src/gui/kconfigloaderhandler_p.h To: mlaurent, dfaure Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26890: QXmlInputSource is deprecated in qt5.15. Port it to QXmlStreamReader
mlaurent marked 4 inline comments as done. REPOSITORY R237 KConfig REVISION DETAIL https://phabricator.kde.org/D26890 To: mlaurent, dfaure Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26890: QXmlInputSource is deprecated in qt5.15. Port it to QXmlStreamReader
apol added inline comments.
INLINE COMMENTS
> kconfigloader.cpp:77
> +case QXmlStreamReader::Characters:
> +if (!reader.isWhitespace() &&
> !reader.text().toString().trimmed().isEmpty()) {
> +if (!characters(reader.text()))
No need to construct a QString if it's just to check if it's empty.
> kconfigloader.cpp:106
> +QString name = attrs.at(i).name().toString().toLower();
> if (name == QLatin1String("name")) {
> //qDebug() << "set group to" << attrs.value(i);
This comparison doesn't need to be onto a converted QString. You can compare
against the QStringRef and use QStringRef::compare to make it case insensitive.
> kconfigloader.cpp:140
> +const QString name = attrs.at(i).name().toString().toLower();
> if (name == QLatin1String("name")) {
> +m_choice.name = attrs.at(i).value().toString();
Use QStringRef::compare()
> kconfigloader.cpp:194
> +const QString tag = localName.toString().toLower();
> if (tag == QLatin1String("entry")) {
> addItem();
I'd also use QStringRef::compare here
REPOSITORY
R237 KConfig
REVISION DETAIL
https://phabricator.kde.org/D26890
To: mlaurent, dfaure
Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26890: QXmlInputSource is deprecated in qt5.15. Port it to QXmlStreamReader
mlaurent updated this revision to Diff 74349. mlaurent added a comment. We need to open QIODevice. Now all works correctly after rebuilding all REPOSITORY R237 KConfig CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26890?vs=74348=74349 BRANCH port_QXmlStreamReader (branched from master) REVISION DETAIL https://phabricator.kde.org/D26890 AFFECTED FILES src/gui/kconfigloader.cpp src/gui/kconfigloaderhandler_p.h To: mlaurent, dfaure Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26890: QXmlInputSource is deprecated in qt5.15. Port it to QXmlStreamReader
mlaurent updated this revision to Diff 74348. mlaurent added a comment. Trim better calling toString() REPOSITORY R237 KConfig CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26890?vs=74295=74348 BRANCH port_QXmlStreamReader (branched from master) REVISION DETAIL https://phabricator.kde.org/D26890 AFFECTED FILES autotests/kconfigloadertest.cpp src/gui/kconfigloader.cpp src/gui/kconfigloaderhandler_p.h To: mlaurent, dfaure Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26890: QXmlInputSource is deprecated in qt5.15. Port it to QXmlStreamReader
apol added a comment.
Looks good otherwise, this code is pretty well tested too, so I quite trust
we aren't breaking it horribly.
Trying to compile the rest of KDE Software against this patch would also be
useful.
INLINE COMMENTS
> kconfigloader.cpp:127
> } else if (name == QLatin1String("key")) {
> -m_key = attrs.value(i).trimmed();
> +m_key = attrs.at(i).value().toString().trimmed();
> }
You can trim before toString, this way we save some string allocations.
REPOSITORY
R237 KConfig
REVISION DETAIL
https://phabricator.kde.org/D26890
To: mlaurent, dfaure
Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26890: QXmlInputSource is deprecated in qt5.15. Port it to QXmlStreamReader
mlaurent edited the test plan for this revision. mlaurent added a reviewer: dfaure. REPOSITORY R237 KConfig REVISION DETAIL https://phabricator.kde.org/D26890 To: mlaurent, dfaure Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26890: QXmlInputSource is deprecated in qt5.15. Port it to QXmlStreamReader
mlaurent created this revision. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. mlaurent requested review of this revision. REVISION SUMMARY QXmlInputSource is deprecated in qt5.15 REPOSITORY R237 KConfig BRANCH port_QXmlStreamReader (branched from master) REVISION DETAIL https://phabricator.kde.org/D26890 AFFECTED FILES autotests/kconfigloadertest.cpp src/gui/kconfigloader.cpp src/gui/kconfigloaderhandler_p.h To: mlaurent Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
