D21661: add snoretoast backend for KNotifications on Windows

[Pino Toscano]

pino added a comment.


  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...
  
  Also: please format all the new code according to the kdelibs coding style -- for example `notifybysnore.cpp`.

INLINE COMMENTS

> CMakeLists.txt:44
> +  find_package(LibSnoreToast REQUIRED)
> +   set_package_properties(LibSnoreToast PROPERTIES TYPE OPTIONAL
> +      PURPOSE     "Enable support for the snorenotify framework"

the find_package above is REQUIRED, while here it is marked as OPTIONAL

> CMakeLists.txt:48-49
> +
> +  find_package(Qt5Core REQUIRED)
> +  find_package(Qt5Network REQUIRED)
> +  list(APPEND knotifications_SRCS notifybysnore.cpp)

why are these two needed? if snoretoast require them, then its cmake config file must require them, so that the above `find_package(LibSnoreToast)` is enough

> CMakeLists.txt:112
>  )
> +if (MSVC)
> +  target_link_libraries(KF5Notifications PRIVATE Qt5::Core Qt5::Network SnoreToast::SnoreToastActions)

this should be the `_FOUND` variable, just like done for the others

> knotification.cpp:381-382
>      return QStringLiteral("android_defaults");
> +#else
> +#ifdef Q_OS_WIN
> +    return QStringLiteral("win32_defaults");    

#elif defined(Q_OS_WIN)

> knotificationmanager.cpp:46-47
> +    #include "notifybyandroid.h"
>  #else
> -#include "notifybyandroid.h"
> +    #ifdef Q_OS_WIN
> +        #include "notifybysnore.h"

#elif defined(Q_OS_WIN)

> knotificationmanager.cpp:76
>      QStringList dirtyConfigCache;
> +    bool inSandbox = false;
>      bool portalDBusServiceExists = false;

this does not seem related to snoretoast

> knotificationmanager.cpp:100-110
> +    if (!qEnvironmentVariableIsEmpty("XDG_RUNTIME_DIR")) {
> +        const QByteArray runtimeDir = qgetenv("XDG_RUNTIME_DIR");
> +        if (!runtimeDir.isEmpty()) {
> +            d->inSandbox = QFileInfo::exists(QFile::decodeName(runtimeDir) + QLatin1String("/flatpak-info"));
> +        }
> +    } else if (qEnvironmentVariableIsSet("SNAP")) {
> +        d->inSandbox = true;

this does not seem related to snoretoast

> knotificationmanager.cpp:152-153
> +            plugin = new NotifyByAndroid(this);
> +        #else
> +            #ifdef Q_OS_WIN
> +                plugin = new NotifyBySnore(this);

#elif defined(Q_OS_WIN)

> notifybysnore.cpp:42
> +
> +static NotifyBySnore *s_instance = nullptr;
> +

this does not seem used -- just drop

> notifybysnore.cpp:49
> +    app = QCoreApplication::instance();
> +    iconDir = new QTemporaryDir();
> +    server = new QLocalServer();

`iconDir` is leaked

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

`server` is leaked

> notifybysnore.cpp:63
> +            const auto index = str.indexOf(QStringLiteral("="));
> +            map[str.mid(0, index)] = str.mid(index + 1);
> +        }

map.insert() is slightly better here

> notifybysnore.cpp:93
> +                                    int action_no = s.indexOf(button) + 1; // QStringList starts with index 0 but not actions
> +                                    NotifyBySnore::notificationActionInvoked(ID, action_no);
> +                                    break;

notificationActionInvoked is not a static method, so do not call it as such

> notifybysnore.cpp:122
> +        }
> +    QProcess *proc = new QProcess();
> +

`proc` is leaked

> notifybysnore.cpp:125
> +    QStringList arguments;
> +    QFile file(iconDir->path() + QString::number(notification->id()));
> +    if (!notification->pixmap().isNull()) {

does `QTemporaryDir::path()` return a path with a trailing path separator? if not, this path is not within the temporary directory

also, maybe add a `.png` extension to the file (mostly to ease its handling)

> notifybysnore.cpp:127
> +    if (!notification->pixmap().isNull()) {
> +            notification->pixmap().save(&file, "PNG");
> +}

the QFile is not open, so I'm not sure whether this will work at all; OTOH, you do not need to create a QFile to save a QPixmap, just pass the file path to save()

also, assuming these images are created within a temporary directory, they will be deleted only at the application exit: this means that long-running applications that have notifications with images (for example media players, IM apps, etc) will consume more and more disk space; please delete each image once its notification is done

> notifybysnore.cpp:153
> +
> +    QProcess *proc = new QProcess();
> +    QStringList arguments;

`proc` is leaked

> notifybysnore.cpp:172-175
> +void NotifyBySnore::notificationActionInvoked(int id, int action)
> +{
> +    emit actionInvoked(id, action);
> +}

this method does not do anything more than emitting a signal... just emit the signal in the only place where it is done

> notifybysnore.h:50
> +    QTemporaryDir *iconDir;
> +    QCoreApplication *app;
> +};

there is no need to keep a pointer to a Q(Core)Application, just use `qApp` instead (already provided by QCoreApplication)

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/83a4660f/attachment-0001.html>