pino added inline comments.
INLINE COMMENTS
> notifybysnore.cpp:115
> +{
> + if (m_notifications.find(notification->id()) != m_notifications.end()) {
> + qCDebug(LOG_KNOTIFICATIONS) << "Duplicate notification with ID:
> " << notification->id() << " ignored.";
since this method is not const, prefer using constFind + constEnd for the hash
lookup
> notifybysnore.cpp:119-121
> + if(notification->id() == -1){
> + qCDebug(LOG_KNOTIFICATIONS) << "Notification with ID : \"-1\"
> ignored.";
> + }
none of the other plugins seem to check for id == -1 here, so I'd say this
check can be dropped
> notifybysnore.cpp:132
> + arguments << QStringLiteral("-m") << notification->text();
> + arguments << QStringLiteral("-p") << file.fileName();
> + arguments << QStringLiteral("-appID") << app->applicationName();
if the notification has no pixmap (see `!notification->pixmap().isNull()` check
above), then I guess you do not need to pass this argument, as the image will
not exist
> notifybysnore.cpp:161-163
> + if (it.value()) {
> + finish(it.value());
> + }
`it` is used after erasing it from `m_notifications`, and that means that
iterator points to nothing; you cannot use iterators after you remove them from
their container
you already have `notification`, so just use it instead
REPOSITORY
R289 KNotifications
REVISION DETAIL
https://phabricator.kde.org/D21661
To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns
