pino added a comment.
In D21661#479366 <https://phabricator.kde.org/D21661#479366>, @brute4s99 wrote: > I think I should make a new diff for further discussions, as this one is quite riddled with suggestions now. Are there any more issues with this patch or should I continue with a new one instead? Please do not drop this opening a new one, otherwise the whole discussion is basically killed... INLINE COMMENTS > CMakeLists.txt:49 > + find_package(Qt5Network REQUIRED) > + find_package(Qt5XmlPatterns REQUIRED) > + list(APPEND knotifications_SRCS notifybysnore.cpp) what do you need XmlPatterns for? > CMakeLists.txt:51 > + list(APPEND knotifications_SRCS notifybysnore.cpp) > + endif () > + this is indented too much > notifybysnore.cpp:80 > + const auto index = str.indexOf(QLatin1Char('=')); > + map.insert(str.toString().mid(0, index), str.mid(index + 1)); > + } instead of getting the real QString of `str` and then get a substring of it, first get the substring(ref) and then get the real QString of it > notifybysnore.cpp:83 > + const auto action = map[QStringLiteral("action")].toString(); > + const auto id = > map[QStringLiteral("notificationId")].toString().toInt(); > + KNotification *notification; QStringRef has toInt() > notifybysnore.cpp:84 > + const auto id = > map[QStringLiteral("notificationId")].toString().toInt(); > + KNotification *notification; > + const auto it = m_notifications.constFind(id); if the notification is not found, this will be an uninitialized pointer; TBH if the search for the notification with the specified id fails, then it should be better to return earlier, as it means the notification is unknown > notifybysnore.cpp:92 > + const auto snoreAction = SnoreToastActions::getAction(waction); > + // MSVC2019 has issues with QString::toStdWString() > + please document what is the issue, so in the future the workaround can be dropped > notifybysnore.cpp:139-140 > + if (m_notifications.constFind(notification->id()) != > m_notifications.constEnd()) { > + qCDebug(LOG_KNOTIFICATIONS) << "Duplicate notification with ID: > " << notification->id() << " ignored."; > + return; > + } they are indented too much > notifybysnore.cpp:168-171 > + if (!proc->waitForStarted()) { > + } else { > + finish(notification); > + } the logic here is swapped: if `waitForStarted()` returns false, that means the process did not start successfully; also, after `finish()` you must return earlier (do not forget to delete the process), otherwise the rest of the code does things as if the process run fine > notifybysnore.cpp:178-179 > + if (exitStatus == QProcess::NormalExit) { > + QFile::remove(QString(iconDir.path() + QLatin1Char('/') > + + QString::number(notification->id()) + > QStringLiteral(".png"))); > + } else { the removal ought to be done no matter the exit status of the process: the process exited, so image for it is no more needed > notifybysnore.cpp:200-202 > + if (it.value()) { > + finish(it.value()); > + } no need to use `it` here, you already have the `notification` parameter... > brute4s99 wrote in notifybysnore.cpp:44 > I've added more inline docs for now. @pino if you could guide me on how to > update the docs on the website, that'd be great! 😃 > if you could guide me on how to update the docs on the website, that'd be > great! which website? REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D21661 To: brute4s99, broulik, sredman, vonreth, albertvaka Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns
