D21661: add snoretoast backend for KNotifications on Windows

[Pino Toscano]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20190615/e41da24c/attachment-0001.html>