Changeset View
Standalone View
src/notifybysnore.cpp
- This file was added.
1 | /* | ||||
---|---|---|---|---|---|
2 | Copyright (C) 2019 Piyush Aggarwal <piyushaggarwal002@gmail.com> | ||||
3 | | ||||
4 | This program is free software; you can redistribute it and/or modify it | ||||
5 | under the terms of the GNU Library General Public License as published by | ||||
6 | the Free Software Foundation; either version 2 of the License, or (at your | ||||
7 | option) any later version. | ||||
8 | | ||||
9 | This program is distributed in the hope that it will be useful, but WITHOUT | ||||
10 | ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or | ||||
11 | FITNESS FOR A PARTICULAR PURPOSE. See the GNU Library General Public | ||||
12 | License for more details. | ||||
13 | | ||||
14 | You should have received a copy of the GNU General Public License | ||||
15 | along with this program. If not, see <https://www.gnu.org/licenses/>. | ||||
16 | */ | ||||
17 | | ||||
18 | #include "notifybysnore.h" | ||||
19 | #include "knotification.h" | ||||
20 | #include "knotifyconfig.h" | ||||
21 | #include "debug_p.h" | ||||
22 | | ||||
23 | #include <QBuffer> | ||||
24 | #include <QIcon> | ||||
vonreth: Is anything from Windows needed?
The header is huge. | |||||
25 | #include <QLoggingCategory> | ||||
26 | #include <QLocalSocket> | ||||
27 | #include <QGuiApplication> | ||||
pino: not needed (`debug_p.h` is already included) | |||||
28 | | ||||
29 | #include <snoretoastactions.h> | ||||
30 | | ||||
pino: already included in the .h file | |||||
31 | /* | ||||
pino: not needed | |||||
32 | * On Windows a shortcut to your app is needed to be installed in the Start Menu | ||||
pino: already included in the .h file | |||||
33 | * (and subsequently, registered with the OS) in order to show notifications. | ||||
pino: extra empty lines | |||||
34 | * Since KNotifications is a library, an app using it can't (feasibly) be properly | ||||
35 | * registered with the OS. It is possible we could come up with some complicated solution | ||||
pino: extra empty line | |||||
36 | * which would require every KNotification-using app to do some special and probably | ||||
37 | * difficult to understand change to support Windows. Or we can have SnoreToast.exe | ||||
38 | * take care of all that nonsense for us. | ||||
39 | * Note that, up to this point, there have been no special | ||||
pino: `QCoreApplication` is enough (see below) | |||||
Actually, I also use applicationDisplayName() as a fallback in case the notification does not have a title() set. One of the use cases was in KDE Connect itself (the pairing notification) brute4s99: Actually, I also use `applicationDisplayName()` as a fallback in case the notification does not… | |||||
40 | * KNotifications changes to the generic application codebase to make this work, | ||||
41 | * just some tweaks to the Craft blueprint and packaging script | ||||
42 | * to pull in SnoreToast and trigger shortcut building respectively. | ||||
43 | * Be sure to have a shortcut installed in Windows Start Menu by SnoreToast. | ||||
pino: this does not seem used -- just drop | |||||
44 | * | ||||
45 | * So the location doesn't matter, but it's only possible to register the internal COM server in an executable. | ||||
sredman: Did this end up being documented? | |||||
brute4s99: Yep! In
https://piyushagg.home.blog/2019/06/07/gsoc19-project-milestone-1/ | |||||
This should be documented somewhere else, too. Either the API dox or the KDE wiki nicolasfella: This should be documented somewhere else, too. Either the API dox or the KDE wiki | |||||
I've added more inline docs for now. @pino if you could guide me on how to update the docs on the website, that'd be great! 😃 brute4s99: I've added more inline docs for now. @pino if you could guide me on how to update the docs on… | |||||
which website? pino: > if you could guide me on how to update the docs on the website, that'd be great!
which… | |||||
brute4s99: https://api.kde.org/frameworks/knotifications/html/index.html | |||||
46 | * We could make it a static lib and link it in all KDE applications, | ||||
47 | * but to make the action center integration work, we would need to also compile a class | ||||
@brute4s99 Have you heard of the how Qt manages the life time of QObjects? vonreth: @brute4s99 Have you heard of the how Qt manages the life time of QObjects?
If you pass this… | |||||
48 | * into the executable using a compile time uuid. | ||||
QCoreApplication::applicationName() is static, so use it directly as static (this applies to all the uses) pino: `QCoreApplication::applicationName()` is static, so use it directly as static (this applies to… | |||||
I think the server should be a bit more unique, what if two process of that name exist? vonreth: I think the server should be a bit more unique, what if two process of that name exist?
How… | |||||
brute4s99: Ooh! Sounds fun! I can do that :D | |||||
49 | * | ||||
50 | * The used header is meant to help with parsing the response. | ||||
pino: `iconDir` is leaked | |||||
pino: this is still not fixed | |||||
FromStdString is wrong, use fromLatin1. vonreth: FromStdString is wrong, use fromLatin1.
What about qHash, we don't ned cryptography algorithm… | |||||
51 | * The cmake target for LibSnoreToast is a INTERFACE lib, it only provides the include path. | ||||
pino: `server` is leaked | |||||
pino: this is still not fixed | |||||
pino: sock is not closed and leaked | |||||
52 | * | ||||
53 | * | ||||
54 | * Trigger the shortcut installation during the installation of your app; syntax for shortcut installation is - | ||||
55 | * ./SnoreToast.exe -install <absolute\address\of\shortcut> <absolute\address\to\app.exe> <appID> | ||||
56 | * | ||||
57 | * appID: use as-is from your app's QCoreApplication::applicationName() when installing the shortcut. | ||||
pino: QMap -> QHash | |||||
vonreth: same here, for this small amount of pairs a map is ok | |||||
58 | * NOTE: Install the shortcut in Windows Start Menu folder. | ||||
59 | * For example, check out Craft Blueprint for Quassel-IRC or KDE Connect. | ||||
pino: closing is good, although the socket object is still leaked | |||||
60 | */ | ||||
61 | | ||||
62 | NotifyBySnore::NotifyBySnore(QObject* parent) : | ||||
63 | KNotificationPlugin(parent) | ||||
64 | { | ||||
pino: map.insert() is slightly better here | |||||
const auto parts = data.splitRef(...); for (... : parts)
pino: - splitRef here to not allocate strings that are not used anyway (since they are split again… | |||||
splitRef is interesting, everyone learns in a code review. vonreth: splitRef
is interesting, everyone learns in a code review.
Wouldn't a `qAsConst` also… | |||||
65 | m_server.listen(QString::number(qHash(qApp->applicationDirPath()))); | ||||
pino: ditto, QStringLiteral -> QLatin1Char | |||||
66 | connect(&m_server, &QLocalServer::newConnection, &m_server, [this]() { | ||||
pino: QDebug can print numbers directly, no need to convert it to string manually | |||||
67 | auto sock = m_server.nextPendingConnection(); | ||||
68 | sock->waitForReadyRead(); | ||||
69 | const QByteArray rawData = sock->readAll(); | ||||
70 | sock->deleteLater(); | ||||
71 | const QString data = | ||||
pino: constFind | |||||
72 | QString::fromWCharArray(reinterpret_cast<const wchar_t *>(rawData.constData()), | ||||
pino: constEnd | |||||
73 | rawData.size() / sizeof(wchar_t)); | ||||
deleteLater should also close the socket. vonreth: deleteLater should also close the socket.
Also don't use a pointer after you deleted it, even… | |||||
74 | QMap<QString, QStringRef> map; | ||||
Add a category to qDebug, like qCDebug(LOG_KNOTIFICATIONS) << "Message";. I can't figure out where LOG_KNOTIFICATIONS is coming from, but other backends seem to use it :) sredman: Add a category to qDebug, like `qCDebug(LOG_KNOTIFICATIONS) << "Message";`. I can't figure out… | |||||
75 | const auto parts = data.splitRef(QLatin1Char(';')); | ||||
76 | for (auto &str : parts) { | ||||
77 | const auto index = str.indexOf(QLatin1Char('=')); | ||||
78 | map.insert(str.mid(0, index).toString(), str.mid(index + 1)); | ||||
pino: no space between the enum and the colon (applies to the other lines too) | |||||
79 | } | ||||
80 | const auto action = map[QStringLiteral("action")].toString(); | ||||
81 | const auto id = map[QStringLiteral("notificationId")].toInt(); | ||||
instead of getting the real QString of str and then get a substring of it, first get the substring(ref) and then get the real QString of it pino: instead of getting the real QString of `str` and then get a substring of it, first get the… | |||||
82 | KNotification *notification; | ||||
83 | const auto it = m_notifications.constFind(id); | ||||
84 | if (it != m_notifications.constEnd()) { | ||||
pino: QStringRef has toInt() | |||||
85 | notification = it.value(); | ||||
if the notification is not found, this will be an uninitialized pointer; TBH if the search for the notification with the specified id fails, then it should be better to return earlier, as it means the notification is unknown pino: if the notification is not found, this will be an uninitialized pointer; TBH if the search for… | |||||
well, I just found out the patch had broken functionality that I fixed just after putting here an else return! 😆 brute4s99: well, I just found out the patch had broken functionality that I fixed just after putting here… | |||||
brute4s99: I have fixed the issue now. | |||||
86 | } | ||||
87 | else { | ||||
88 | qCDebug(LOG_KNOTIFICATIONS) << "Notification not found!"; | ||||
89 | return; | ||||
90 | } | ||||
91 | | ||||
92 | // MSVC2019 has issues with QString::toStdWString() | ||||
93 | // Qstring::toStdWString() doesn't work with MSVC2019 yet. If it gets fixed | ||||
please document what is the issue, so in the future the workaround can be dropped pino: please document what is the issue, so in the future the workaround can be dropped | |||||
I can't put my finger on it I can only tell that https://github.com/KDE/snoretoast/blob/a53a6b733624ada78ef0f852407cad4be1c00b16/examples/qt/main.cpp#L58 produces a corrupted string. vonreth: I can't put my finger on it I can only tell that https://github. | |||||
94 | // in future, feel free to change the implementation below for lesser LOC. | ||||
pino: notificationActionInvoked is not a static method, so do not call it as such | |||||
95 | std::wstring waction(action.size(), 0); | ||||
96 | action.toWCharArray(const_cast<wchar_t *>(waction.data())); | ||||
97 | const auto snoreAction = SnoreToastActions::getAction(waction); | ||||
98 | | ||||
99 | qCDebug(LOG_KNOTIFICATIONS) << "The notification ID is : " << id; | ||||
100 | switch (snoreAction) { | ||||
101 | case SnoreToastActions::Actions::Clicked: | ||||
102 | qCDebug(LOG_KNOTIFICATIONS) << " User clicked on the toast."; | ||||
103 | if (notification) { | ||||
104 | close(notification); | ||||
105 | } | ||||
106 | break; | ||||
107 | case SnoreToastActions::Actions::Hidden: | ||||
108 | qCDebug(LOG_KNOTIFICATIONS) << "The toast got hidden."; | ||||
109 | break; | ||||
110 | case SnoreToastActions::Actions::Dismissed: | ||||
111 | qCDebug(LOG_KNOTIFICATIONS) << "User dismissed the toast."; | ||||
112 | break; | ||||
113 | case SnoreToastActions::Actions::Timedout: | ||||
114 | qCDebug(LOG_KNOTIFICATIONS) << "The toast timed out."; | ||||
115 | break; | ||||
116 | case SnoreToastActions::Actions::ButtonClicked:{ | ||||
since this method is not const, prefer using constFind + constEnd for the hash lookup pino: since this method is not const, prefer using constFind + constEnd for the hash lookup | |||||
pino: this is not needed, the destructor of QTemporaryDir will do it by default | |||||
117 | qCDebug(LOG_KNOTIFICATIONS) << " User clicked a button on the toast."; | ||||
118 | const auto button = map[QStringLiteral("button")].toString(); | ||||
sredman: Maybe this log line should be different? :) | |||||
119 | QStringList s = m_notifications.value(id)->actions(); | ||||
120 | int actionNum = s.indexOf(button) + 1; // QStringList starts with index 0 but not actions | ||||
no need to initialize it here -- it can go directly in the if block where it is used pino: no need to initialize it here -- it can go directly in the if block where it is used | |||||
121 | emit actionInvoked(id, actionNum); | ||||
pino: extra empty line | |||||
122 | break;} | ||||
none of the other plugins seem to check for id == -1 here, so I'd say this check can be dropped pino: none of the other plugins seem to check for id == -1 here, so I'd say this check can be dropped | |||||
123 | case SnoreToastActions::Actions::TextEntered: | ||||
pino: `proc` is leaked | |||||
pino: this is still not fixed | |||||
124 | qCDebug(LOG_KNOTIFICATIONS) << " User entered some text in the toast."; | ||||
125 | break; | ||||
126 | default: | ||||
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) pino: does `QTemporaryDir::path()` return a path with a trailing path separator? if not, this path is… | |||||
127 | qCDebug(LOG_KNOTIFICATIONS) << "Unexpected behaviour with the toast."; | ||||
pino: move this variable directly in the code block where it is used | |||||
128 | if (notification) { | ||||
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 pino: the QFile is not open, so I'm not sure whether this will work at all; OTOH, you do not need to… | |||||
129 | close(notification); | ||||
130 | } | ||||
131 | break; | ||||
132 | } | ||||
133 | }); | ||||
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 pino: if the notification has no pixmap (see `!notification->pixmap().isNull()` check above), then I… | |||||
pino: coding style: they go in the same line | |||||
134 | } | ||||
pino: no need to append, just assign directly (iconPath was empty before, anyway) | |||||
135 | | ||||
136 | NotifyBySnore::~NotifyBySnore() | ||||
137 | { | ||||
138 | m_server.close(); | ||||
139 | } | ||||
140 | | ||||
141 | void NotifyBySnore::notify(KNotification *notification, KNotifyConfig *config) | ||||
pino: they are indented too much | |||||
142 | { | ||||
143 | QProcess *proc = new QProcess(); | ||||
144 | QStringList arguments; | ||||
145 | | ||||
146 | arguments << QStringLiteral("-t"); | ||||
147 | if (!notification->title().isEmpty()) { | ||||
148 | arguments << notification->title(); | ||||
pino: either it uses the same debug category of the other message, or it is dropped | |||||
149 | } else { | ||||
150 | arguments << qApp->applicationDisplayName(); | ||||
151 | } | ||||
152 | arguments << QStringLiteral("-m") << notification->text(); | ||||
better have this as very last operation done, so early returns will not have this notification in m_notifications pino: better have this as very last operation done, so early returns will not have this notification… | |||||
153 | if (!notification->pixmap().isNull()) { | ||||
most probably check whether starting the process succeeded, and if not call finish(notification) and return earlier pino: most probably check whether starting the process succeeded, and if not call `finish… | |||||
154 | auto iconPath = QString(m_iconDir.path() + QLatin1Char('/') | ||||
pino: `proc` is leaked | |||||
pino: this still not fixed | |||||
vonreth: deleteLater() | |||||
155 | + QString::number(notification->id()) + QStringLiteral(".png")); | ||||
156 | notification->pixmap().save(iconPath, "PNG"); | ||||
sredman: Does this `;` belong to something? | |||||
pino: There is a static `QFile::remove()`, so use it directly | |||||
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() pino: won't this remove the image too early? this will remove the image once the process ends, not… | |||||
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? vonreth: No need to remove the icon at all, we use a tmp dir so we could just keep them around until the… | |||||
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 pino: IMHO it is better to check whether the process exited correctly, and its return status was a… | |||||
157 | arguments << QStringLiteral("-p") << iconPath; | ||||
158 | } | ||||
159 | arguments << QStringLiteral("-appID") << qApp->applicationName() | ||||
160 | << QStringLiteral("-id") << QString::number(notification->id()) | ||||
161 | << QStringLiteral("-pipename") << m_server.fullServerName(); | ||||
162 | | ||||
163 | if (!notification->actions().isEmpty()) { | ||||
164 | arguments << QStringLiteral("-b") << notification->actions().join(QLatin1Char(';')); | ||||
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 pino: `it` is used after erasing it from `m_notifications`, and that means that iterator points to… | |||||
this is partially still not fixed:
pino: this is partially still not fixed:
> you already have notification, so just use it instead
| |||||
165 | } | ||||
166 | qCDebug(LOG_KNOTIFICATIONS) << arguments; | ||||
167 | proc->start(m_program, arguments); | ||||
168 | m_notifications.insert(notification->id(), notification); | ||||
169 | connect(proc, QOverload<int, QProcess::ExitStatus>::of(&QProcess::finished), | ||||
170 | [=](int exitCode, QProcess::ExitStatus exitStatus){ | ||||
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:
pino: considering here the notification is being closed and thus checking what the tool does has… | |||||
171 | proc->deleteLater(); | ||||
172 | if (exitStatus != QProcess::NormalExit) { | ||||
the logic here is swapped: if waitForStarted() returns false, that means the process did not start successfully; also, after finish() you must return earlier (do not forget to delete the process), otherwise the rest of the code does things as if the process run fine pino: the logic here is swapped: if `waitForStarted()` returns false, that means the process did not… | |||||
I'm removing this code chunk because we already check for successful show of notif in the following connect. brute4s99: I'm removing this code chunk because we already check for successful show of notif in the… | |||||
173 | qCDebug(LOG_KNOTIFICATIONS) << "SnoreToast crashed while trying to show a notification."; | ||||
174 | close(notification); | ||||
175 | } | ||||
176 | QFile::remove(QString(m_iconDir.path() + QLatin1Char('/') | ||||
this method does not do anything more than emitting a signal... just emit the signal in the only place where it is done pino: this method does not do anything more than emitting a signal... just emit the signal in the… | |||||
177 | + QString::number(notification->id()) + QStringLiteral(".png"))); | ||||
no need to clear this here, it will be deleted at the end of the function anyway... pino: no need to clear this here, it will be deleted at the end of the function anyway... | |||||
178 | }); | ||||
179 | } | ||||
180 | | ||||
the removal ought to be done no matter the exit status of the process: the process exited, so image for it is no more needed pino: the removal ought to be done no matter the exit status of the process: the process exited, so… | |||||
181 | void NotifyBySnore::close(KNotification *notification) | ||||
Use proc->deleteLater() which will call the destructor https://doc.qt.io/qt-5/qprocess.html#dtor.QProcess vonreth: Use proc->deleteLater() which will call the destructor https://doc.qt.io/qt-5/qprocess. | |||||
vonreth: Only delete once 🙈 | |||||
182 | { | ||||
183 | if (m_notifications.constFind(notification->id()) == m_notifications.constEnd()) { | ||||
184 | return; | ||||
185 | } | ||||
186 | qCDebug(LOG_KNOTIFICATIONS) << "SnoreToast closing notification with ID: " << notification->id(); | ||||
187 | QStringList arguments; | ||||
188 | arguments << QStringLiteral("-close") << QString::number(notification->id()) | ||||
189 | << QStringLiteral("-appID") << qApp->applicationName(); | ||||
190 | QProcess::startDetached(m_program, arguments); | ||||
191 | if (notification) { | ||||
192 | finish(notification); | ||||
193 | } | ||||
194 | m_notifications.remove(notification->id()); | ||||
195 | } | ||||
196 | | ||||
197 | void NotifyBySnore::update(KNotification *notification, KNotifyConfig *config) | ||||
198 | { | ||||
199 | close(notification); | ||||
200 | notify(notification, config); | ||||
201 | } | ||||
pino: C/C++ source require an empty newline at the end |
Is anything from Windows needed?
The header is huge.