D21661: add snoretoast backend for KNotifications on Windows

[Pino Toscano]

pino added a comment.


  In D21661#476156 <https://phabricator.kde.org/D21661#476156>, @pino wrote:
  
  > how is snoretoast actually used here? you are requiring the library for building and linking, but then:
  >
  > - `snoretoastactions.h`, which is part of the headers of snoretoast, is copied here
  > - the snoretoast library is never used, as the utilities of it are invoked instead If the library does all the work already, then I'd prefer to use it directly instead of spawning executables all the time...
  
  
  Piyush, what about this? It seems the snoretoast library provides a `SnoreToasts` class to do this instead of spawning an helper tool, what about using it instead?
  
  > Also: please format all the new code according to the kdelibs coding style -- for example `notifybysnore.cpp`.
  
  Still needs to be done.  Please make sure to indent by 4 spaces only, no tabs; also mind the spacing around conditions of `if`s, etc:
  https://community.kde.org/Policies/Kdelibs_Coding_Style
  
  Furthermore: there is a big switch on `snoreAction` that reacts only on buttons clicked; some of the other actions hint that the notification was closed, so most probably the KNotification object ought to be closed too. Check what the `NotifyByPopup` plugins does, for example, as there is something similar done.

INLINE COMMENTS

> knotificationmanager.cpp:45
> +#if defined(Q_OS_ANDROID)
> +    #include "notifybyandroid.h"
> +#elif defined(Q_OS_WIN)

do not add spaces before the `#` of C preprocessor tokens, as it is undefined/nonstandard behaviour

> knotificationmanager.cpp:145
> +        #else
> +        if (d->inSandbox && d->portalDBusServiceExists) {
> +            plugin = new NotifyByPortal(this);

this change is still unrelated -- are you really sure you rebased your work on top of the current master branch?

> notifybysnore.cpp:26
> +#include <QBuffer>
> +#include <QDebug>
> +#include <QIcon>

not needed (`debug_p.h` is already included)

> notifybysnore.cpp:30
> +#include <QString>
> +#include <QDir>
> +

not needed

> notifybysnore.cpp:31-32
> +#include <QDir>
> +
> +
> +#include <QTemporaryDir>

extra empty lines

> notifybysnore.cpp:34
> +#include <QTemporaryDir>
> +
> +#include <QLoggingCategory>

extra empty line

> notifybysnore.cpp:38
> +#include <QLocalSocket>
> +#include <QGuiApplication>
> +

`QCoreApplication` is enough (see below)

> notifybysnore.cpp:47
> +    server = new QLocalServer();
> +    server->listen(qApp->applicationName());
> +

`QCoreApplication::applicationName()` is static, so use it directly as static (this applies to all the uses)

> notifybysnore.cpp:50
> +    QObject::connect(server, &QLocalServer::newConnection, server, [this]() {
> +        auto sock = server->nextPendingConnection();
> +        sock->waitForReadyRead();

sock is not closed and leaked

> notifybysnore.cpp:56
> +                                        rawData.size() / sizeof(wchar_t));
> +        QMap<QString, QString> map;
> +        for (const auto &str : data.split(QStringLiteral(";"))) {

QMap -> QHash

> notifybysnore.cpp:65
> +        const auto snoreAction = SnoreToastActions::getAction(action.toStdWString());
> +        qCDebug(LOG_KNOTIFICATIONS) << "The notification ID is : " << QString::number(id);
> +        switch(snoreAction){

QDebug can print numbers directly, no need to convert it to string manually

> notifybysnore.cpp:119
> +    QStringList arguments;
> +    QString iconPath(QStringLiteral(""));
> +

no need to initialize it here -- it can go directly in the if block where it is used

> notifybysnore.cpp:120
> +    QString iconPath(QStringLiteral(""));
> +
> +

extra empty line

> notifybysnore.cpp:133
> +    if(!notification->pixmap().isNull()){
> +        iconPath.append(iconDir->path() + QStringLiteral("/") 
> +                        + QString::number(notification->id()) + QStringLiteral(".png"));

no need to append, just assign directly (iconPath was empty before, anyway)

> notifybysnore.cpp:147
> +    
> +    qDebug() << arguments;
> +    m_notifications.insert(notification->id(), notification);

either it uses the same debug category of the other message, or it is dropped

> notifybysnore.cpp:154-155
> +        proc->close();
> +        QFile file(iconPath);
> +        file.remove();
> +    });

There is a static `QFile::remove()`, so use it directly

> notifybysnore.cpp:154-155
> +        proc->close();
> +        QFile file(iconPath);
> +        file.remove();
> +    });

won't this remove the image too early? this will remove the image once the process ends, not when notification is closed; this fits better in `close()`

> notifybysnore.cpp:176
> +    }
> +    arguments.clear();
> +    m_notifications.erase(it);

no need to clear this here, it will be deleted at the end of the function anyway...

> notifybysnore.cpp:190
> +}
> \ No newline at end of file


C/C++ source require an empty newline at the end

> pino wrote in notifybysnore.cpp:49
> `iconDir` is leaked

this is still not fixed

> pino wrote in notifybysnore.cpp:50
> `server` is leaked

this is still not fixed

> pino wrote in notifybysnore.cpp:122
> `proc` is leaked

this is still not fixed

> pino wrote in notifybysnore.cpp:153
> `proc` is leaked

this still not fixed

> pino wrote in notifybysnore.cpp:161-163
> `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

this is partially still not fixed:

> you already have notification, so just use it instead

> notifybysnore.h:28
> +#include <QTemporaryDir>
> +
> +

extra empty line

> notifybysnore.h:44
> +private:
> +    QMap<int, QPointer<KNotification>> m_notifications;
> +    QString program = QStringLiteral("SnoreToast.exe");

QMap -> QHash

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