Really fix the Windows backend for KNotifications
AbandonedPublic

Authored by brute4s99 on Jan 21 2020, 3:08 AM.

Details

Reviewers
vonreth
broulik
Group Reviewers
KDE Connect
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.

Diff Detail

Repository
R289 KNotifications
Branch
arcpatch-D26801
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 21545
Build 21563: arc lint + arc unit
brute4s99 created this revision.Jan 21 2020, 3:08 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptJan 21 2020, 3:08 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
brute4s99 requested review of this revision.Jan 21 2020, 3:08 AM
brute4s99 added inline comments.Jan 21 2020, 3:17 AM
src/notifybysnore.cpp
147–149

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).

brute4s99 added inline comments.Jan 21 2020, 3:18 AM
src/notifybysnore.cpp
147–149

Weirdly though, pairingRequest notification is not updated by the base.

^ this refers to eventId of the notification for KDE Connect's pairing request.

anthonyfieroni added inline comments.
src/notifybysnore.cpp
147–149

So the problem isn't here. notification id should be always valid and not calculated here.

src/notifybysnore.h
50

You cannot overlap signed integer, it's undefined behaviour.

vonreth added inline comments.Jan 21 2020, 9:18 AM
src/notifybysnore.cpp
147–149

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

152

const newId = m_counter++;

204

At least warn?

206–207

const int newId

src/notifybysnore.h
49

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.

brute4s99 updated this revision to Diff 74005.Jan 21 2020, 11:05 AM
brute4s99 marked 3 inline comments as done.

update the diff.

brute4s99 marked an inline comment as done.Jan 21 2020, 11:06 AM
vonreth added inline comments.Jan 21 2020, 12:13 PM
src/notifybysnore.cpp
90

if (!notification) or at least compare with nullptr
And remove the commented KNotificaion*

src/notifybysnore.h
49

now add white space around the = :)

brute4s99 updated this revision to Diff 74190.Jan 23 2020, 6:43 AM
brute4s99 marked 2 inline comments as done.

updated

broulik added inline comments.Jan 23 2020, 8:24 AM
src/notifybysnore.cpp
89

if (!notification) {

147

Why this random eventId check?
Also, compare with QLatin1String

200

Does the notification still end up with an id eventually?

202

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.

brute4s99 marked 2 inline comments as done.Jan 25 2020, 1:01 PM

I will be closing this diff as I recently formed a simpler patch. Inviting your reviews on that one!

https://phabricator.kde.org/D26888

brute4s99 abandoned this revision.Jan 27 2020, 6:47 AM
brute4s99 added inline comments.
src/notifybysnore.cpp
200

yes. I've used the id in the new patch