broulik added inline comments.
INLINE COMMENTS
> notifybysnore.cpp:89
> + KNotification *notification = m_notifications.value(id);
> + if (notification == nullptr) {
> qCDebug(LOG_KNOTIFICATIONS) << "Notification not found!";
`if (!notification) {`
> notifybysnore.cpp:147
> {
> - Q_UNUSED(config);
> - // HACK work around that notification->id() is only populated after
> returning from here
> - // note that config will be invalid at that point, so we can't pass that
> along
> - QMetaObject::invokeMethod(this, [this, notification](){
> notifyDeferred(notification); }, Qt::QueuedConnection);
> -}
> + if(notification->id() == -1 && notification->eventId() ==
> QStringLiteral("notification")) {
> + return;
Why this random `eventId` check?
Also, compare with `QLatin1String`
> notifybysnore.cpp:200
>
> void NotifyBySnore::close(KNotification *notification)
> {
Does the notification still end up with an `id` eventually?
> notifybysnore.cpp:202
> {
> - if (m_notifications.constFind(notification->id()) ==
> m_notifications.constEnd()) {
> + if (m_notifications.key(notification) == -1) {
> + qCWarning(LOG_KNOTIFICATIONS) << "Notification was already closed.";
Where do you get this `-1`?
> If the hash contains no item with the value, the function returns a
> default-constructed key.
You'll get `0` back unless you specify a `defaultValue`.
Also cache this as you do another `key` look up again below.
REPOSITORY
R289 KNotifications
REVISION DETAIL
https://phabricator.kde.org/D26801
To: brute4s99, vonreth, broulik, #kde_connect
Cc: anthonyfieroni, kde-frameworks-devel, nalvarez, KunalRaghav, ankitbaluni,
ankit, aliencode, Orage, ritwizsinha, LeGast00n, ewentzel, dshelley,
pawelkwiecinski, ctakano, vporvaznik, mschroeder, varunp, shivanshukantprasad,
skymoore, fbampaloukas, GB_2, brute4s99, wistak, anoopv, dvalencia, rmenezes,
julioc, Leptopoda, timothyc, Danial0_0, johnq, Pitel, domson, adeen-s,
michaelh, SemperPeritus, daniel.z.tg, jeanv, seebauer, ngraham, bruns, bugzy,
MayeulC, lemuel, menasshock, mikesomov, tctara, apol
