D26801: Really fix the Windows backend for KNotifications
brute4s99 added inline comments. INLINE COMMENTS > broulik wrote in notifybysnore.cpp:200 > Does the notification still end up with an `id` eventually? yes. I've used the id in the new patch 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
D26801: Really fix the Windows backend for KNotifications
brute4s99 abandoned this revision. 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
D26801: Really fix the Windows backend for KNotifications
brute4s99 marked 2 inline comments as done. brute4s99 added a comment. I will be closing this diff as I recently formed a simpler patch. Inviting your reviews on that one! https://phabricator.kde.org/D26888 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
D26801: Really fix the Windows backend for KNotifications
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
D26801: Really fix the Windows backend for KNotifications
brute4s99 updated this revision to Diff 74190. brute4s99 marked 2 inline comments as done. brute4s99 added a comment. updated REPOSITORY R289 KNotifications CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26801?vs=74005=74190 BRANCH arcpatch-D26801 REVISION DETAIL https://phabricator.kde.org/D26801 AFFECTED FILES src/notifybysnore.cpp src/notifybysnore.h 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
D26801: Really fix the Windows backend for KNotifications
vonreth added inline comments.
INLINE COMMENTS
> notifybysnore.cpp:90
> +KNotification *notification = m_notifications.value(id);
> +if (notification == 0) {
> qCDebug(LOG_KNOTIFICATIONS) << "Notification not found!";
if (!notification) or at least compare with nullptr
And remove the commented KNotificaion*
> vonreth wrote in notifybysnore.h:48
> m_notificationsRev
> m_counter
>
> KDE and Qt uses cammelCase and member vars are prefixed with an "m_".
>
> m_notifications it only used to see which notification was closed by a user
> interaction to snoretoast (this part i probably broken right now).
> There is no need to keep two maps.
now add white space around the = :)
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
D26801: Really fix the Windows backend for KNotifications
brute4s99 marked an inline comment as done. 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
D26801: Really fix the Windows backend for KNotifications
brute4s99 updated this revision to Diff 74005. brute4s99 marked 3 inline comments as done. brute4s99 added a comment. update the diff. REPOSITORY R289 KNotifications CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26801?vs=73983=74005 BRANCH master REVISION DETAIL https://phabricator.kde.org/D26801 AFFECTED FILES src/notifybysnore.cpp src/notifybysnore.h 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
D26801: Really fix the Windows backend for KNotifications
vonreth added inline comments.
INLINE COMMENTS
> anthonyfieroni wrote in notifybysnore.cpp:143
> So the problem isn't here. notification id should be always valid and not
> calculated here.
I'm not sure what would be correct here but just ignoring those notifications
sounds like a bad idea.
This is really a question for @broulik
> notifybysnore.cpp:148
> +Q_UNUSED(config);
> +int new_ID = counter;
> +counter++;
const newId = m_counter++;
> notifybysnore.cpp:201
> +if (rev_m_notifications.constFind(notification) ==
> rev_m_notifications.constEnd()) {
> return;
> }
At least warn?
> notifybysnore.cpp:203
> }
> -qCDebug(LOG_KNOTIFICATIONS) << "SnoreToast closing notification with ID:
> " << notification->id();
> +int new_ID = rev_m_notifications.value(notification);
> +qCDebug(LOG_KNOTIFICATIONS) << "SnoreToast closing notification with ID:
> " << new_ID;
const int newId
> notifybysnore.h:48
> QTemporaryDir m_iconDir;
> +QHash rev_m_notifications;
> +int counter=0;
m_notificationsRev
m_counter
KDE and Qt uses cammelCase and member vars are prefixed with an "m_".
m_notifications it only used to see which notification was closed by a user
interaction to snoretoast (this part i probably broken right now).
There is no need to keep two maps.
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
D26801: Really fix the Windows backend for KNotifications
anthonyfieroni added inline comments. INLINE COMMENTS > brute4s99 wrote in notifybysnore.cpp:143 > KNotif objects by default have `d->id = -1` > > `notify()` is invoked by the base with -1 ID and later updated with the > correct ID and the actions. > > Weirdly though, `pairingRequest` notification is not updated by the base, > (that is, the notification has all the information in the first invocation > itself) but still it has ID=-1 (because it's not updated later by the base, > hence the default ID). So the problem isn't here. notification id should be always valid and not calculated here. > notifybysnore.h:49 > +QHash rev_m_notifications; > +int counter=0; > }; You cannot overlap signed integer, it's undefined behaviour. 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
D26801: Really fix the Windows backend for KNotifications
brute4s99 added inline comments. INLINE COMMENTS > brute4s99 wrote in notifybysnore.cpp:143 > KNotif objects by default have `d->id = -1` > > `notify()` is invoked by the base with -1 ID and later updated with the > correct ID and the actions. > > Weirdly though, `pairingRequest` notification is not updated by the base, > (that is, the notification has all the information in the first invocation > itself) but still it has ID=-1 (because it's not updated later by the base, > hence the default ID). > Weirdly though, `pairingRequest` notification is not updated by the base. ^ this refers to `eventId` of the notification for KDE Connect's pairing request. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D26801 To: brute4s99, vonreth, broulik, #kde_connect Cc: 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
D26801: Really fix the Windows backend for KNotifications
brute4s99 added inline comments.
INLINE COMMENTS
> notifybysnore.cpp:143
> {
> -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;
KNotif objects by default have `d->id = -1`
`notify()` is invoked by the base with -1 ID and later updated with the correct
ID and the actions.
Weirdly though, `pairingRequest` notification is not updated by the base, (that
is, the notification has all the information in the first invocation itself)
but still it has ID=-1 (because it's not updated later by the base, hence the
default ID).
REPOSITORY
R289 KNotifications
REVISION DETAIL
https://phabricator.kde.org/D26801
To: brute4s99, vonreth, broulik, #kde_connect
Cc: 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
D26801: Really fix the Windows backend for KNotifications
brute4s99 created this revision. brute4s99 added reviewers: vonreth, broulik, KDE Connect. brute4s99 added a project: KDE Connect. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. brute4s99 requested review of this revision. REVISION SUMMARY This patch fixes the Windows backend. The workaround was implemented by completing ditching the notification->id() thing, because it relies on updating the notification afterwards. Updating was not a viable option because WIndows 8 and 8.1 does not support updating a notification. TEST PLAN Now the backend actually works and does not crash at every other notification like it used to. REPOSITORY R289 KNotifications BRANCH master REVISION DETAIL https://phabricator.kde.org/D26801 AFFECTED FILES src/notifybysnore.cpp src/notifybysnore.h To: brute4s99, vonreth, broulik, #kde_connect Cc: 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
