D21661: add snoretoast backend for KNotifications on Windows

[Hannah von Reth]

vonreth added inline comments.

INLINE COMMENTS

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

@brute4s99 Have you heard of the how Qt manages the life time of QObjects?
 If you pass this class as the parent object, the server will automatically get deleted once this class gets deleted

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

I think the server should be a bit more unique,  what if two process of that name exist?
How about the full application path as a qHash?

> pino wrote in notifybysnore.cpp:56
> QMap -> QHash

same here, for this small amount of pairs a map is ok

> notifybysnore.cpp:153
> +        [=](int exitCode, QProcess::ExitStatus exitStatus){ 
> +        proc->close();
> +        QFile file(iconPath);

deleteLater()

> notifybysnore.cpp:155
> +        QFile file(iconPath);
> +        file.remove();
> +    });

No need to remove the icon at all, we use a tmp dir so we could just keep them around until the application exits and they get deleted automatically?

> notifybysnore.cpp:180
> +    [=](int exitCode, QProcess::ExitStatus exitStatus){ 
> +        proc->close();
> +    });

Use proc->deleteLater() which will call the destructor https://doc.qt.io/qt-5/qprocess.html#dtor.QProcess

> pino wrote in notifybysnore.h:44
> QMap -> QHash

The amount of notifications should be small in which case a map has a better performance.

> notifybysnore.h:46
> +    QString program = QStringLiteral("SnoreToast.exe");
> +    QLocalServer *server;
> +    QTemporaryDir *iconDir;

I guess you could use both, server and icon dir as non pointer members, that way they would exist as long as NotifyBySnore  exists.

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/20190609/090088f7/attachment.html>