pino added a comment.
In D21661#476607 <https://phabricator.kde.org/D21661#476607>, @sredman wrote: > In D21661#476571 <https://phabricator.kde.org/D21661#476571>, @pino wrote: > > > It seems the snoretoast library provides a `SnoreToasts` class to do this instead of spawning an helper tool, what about using it instead? > > > @vonreth can probably explain better, but basically the situation as I understand it is on Windows you need to be installed in a special place and registered with the OS in order to show notifications. Since KNotifications is a library, an app using it can't (feasibly) be properly registered with the OS. It is possible we could come up with some complicated solution which would require every KNotification-using app to do some special and probably difficult to understand change to support Windows. Or we can have SnoreNotify.exe take care of all that nonsense for us. Note that, up to this point, there have been no special KNotifications changes to the generic KDE Connect codebase to make this work, just some tweaks to the Windows installer to pull in SnoreToast. IMHO this, plus what @vonreth adds later on, is exactly the kind of information that is important enough to be explained at least either as comment in the sources, or as commit message. This way people know why this approach was used, and they will not try to change things later on without dealing with the same situation. Furthermore: - I still do think that is better to remove the cached image of a notification when removing it. Yes, they are stored in a QTemporaryDir directory, so they will be deleted when the application exists, although they will pile up and take more and more space. This is not a problem for short-living applications, but it is for long-running application, for example: - a media player showing the popup notification of a new song with its album cover, so every 3/4/5 minutes - a IM application showing messages from other users with their avatars when a message is received, so potentially even every few seconds - regarding the notification image: so far only the pixmap is set, although there are other ways to set the "artwork" to use for a notification: for example the `IconName` in the notifyrc configuration, or the iconName proeprty in `KNotifyConfig`; check what the NotifyByPopup plugin does, for example, to get them, and what the NotifyByAndroid plugin does to get an image from a theme icon name INLINE COMMENTS > pino wrote in CMakeLists.txt:112 > `Qt5::Core` is not needed here; if we really want to be pedantic, it ought to > be added unconditionally (however that would be a different patch than this > one) still there > pino wrote in CMakeLists.txt:48-49 > Qt5Core is already pretty much required to build anything using Qt5, so > looking for it in this place is a no-op (because it was already found) still there > notifybysnore.cpp:29 > +#include <QString> > +#include <QTemporaryDir> > +#include <QLoggingCategory> already included in the .h file > notifybysnore.cpp:31 > +#include <QLoggingCategory> > +#include <QLocalServer> > +#include <QLocalSocket> already included in the .h file > notifybysnore.cpp:58 > + const QByteArray rawData = sock->readAll(); > + sock->close(); > + const QString data = closing is good, although the socket object is still leaked > notifybysnore.cpp:63 > + QMap<QString, QString> map; > + for (const auto &str : data.split(QStringLiteral(";"))) { > + const auto index = str.indexOf(QStringLiteral("=")); - splitRef here to not allocate strings that are not used anyway (since they are split again later on) - you are using a C++11 for loop on a Qt container, and this will detach it -- you need to store the container as variable: const auto parts = data.splitRef(...); for (... : parts) - QLatin1Char is better than QStringLiteral here (since you have a single character as separator) > notifybysnore.cpp:64 > + for (const auto &str : data.split(QStringLiteral(";"))) { > + const auto index = str.indexOf(QStringLiteral("=")); > + map.insert(str.mid(0, index), str.mid(index + 1)); ditto, QStringLiteral -> QLatin1Char > notifybysnore.cpp:70 > + KNotification *notification = nullptr; > + const auto it = m_notifications.find(id); > + if (it != m_notifications.end()) { constFind > notifybysnore.cpp:71 > + const auto it = m_notifications.find(id); > + if (it != m_notifications.end()) { > + notification = it.value(); constEnd > notifybysnore.cpp:77 > + switch (snoreAction) { > + case SnoreToastActions::Actions::Clicked : > + qCDebug(LOG_KNOTIFICATIONS) << " User clicked on the toast."; no space between the enum and the colon (applies to the other lines too) > notifybysnore.cpp:115 > + server.close(); > + iconDir.remove(); > +} this is not needed, the destructor of QTemporaryDir will do it by default > notifybysnore.cpp:126 > + QStringList arguments; > + QString iconPath; > + move this variable directly in the code block where it is used > notifybysnore.cpp:131-132 > + arguments << notification->title(); > + } > + else { > + arguments << qApp->applicationDisplayName(); coding style: they go in the same line > notifybysnore.cpp:151 > + > + m_notifications.insert(notification->id(), notification); > + proc->start(program, arguments); better have this as very last operation done, so early returns will not have this notification in `m_notifications` > notifybysnore.cpp:152 > + m_notifications.insert(notification->id(), notification); > + proc->start(program, arguments); > + most probably check whether starting the process succeeded, and if not call `finish(notification)` and return earlier > notifybysnore.cpp:155 > + connect(proc, QOverload<int, > QProcess::ExitStatus>::of(&QProcess::finished), > + [=](int exitCode, QProcess::ExitStatus exitStatus){ > + proc->deleteLater(); IMHO it is better to check whether the process exited correctly, and its return status was a success, finish'ing the notification in case of failure > notifybysnore.cpp:169 > + > + QProcess *proc = new QProcess(); > + QStringList arguments; considering here the notification is being closed and thus checking what the tool does has almost no effect on the flow, then there is no need to manually create a QProcess object: - collect the arguments (like you already do) - use the static `QProcess::startDetached(program, args)` > brute4s99 wrote in notifybysnore.h:44 > I referred to this: https://woboq.com/blog/qmap_qhash_benchmark.html All the other `KNotificationPlugin`s in knotification use QHash for indexing the `KNotification` objects, FWIW, so at least using QHash would keep a bit of consistency. 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
