add snoretoast backend for KNotifications on Windows
ClosedPublic

Authored by brute4s99 on Jun 7 2019, 11:45 PM.

Details

Summary

This is the main patch that adds the interfacing between KNotifications and SnoreToast.

Diff Detail

Repository
R289 KNotifications
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
pino added inline comments.Jun 11 2019, 5:00 AM
src/notifybysnore.cpp
64
  • 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)
65

ditto, QStringLiteral -> QLatin1Char

72

constEnd

78

no space between the enum and the colon (applies to the other lines too)

116

this is not needed, the destructor of QTemporaryDir will do it by default

127

move this variable directly in the code block where it is used

132–133

coding style: they go in the same line

152

better have this as very last operation done, so early returns will not have this notification in m_notifications

153

most probably check whether starting the process succeeded, and if not call finish(notification) and return earlier

156

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

170

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)
vonreth added inline comments.Jun 11 2019, 7:21 AM
src/notifybysnore.cpp
64
splitRef

is interesting, everyone learns in a code review.
Wouldn't a qAsConst also prevent detaching?

brute4s99 marked 67 inline comments as done.Jun 13 2019, 10:44 PM

updated code incoming. I think I should make a new diff for further discussions, as this one is quite riddled with suggestions now. Are there any more issues with this patch or should I continue with a new one instead? I'm willing to fix it further if you have some suggestions! ✊

src/notifybysnore.cpp
45

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 updated this revision to Diff 59765.Jun 13 2019, 10:46 PM
brute4s99 marked an inline comment as done.
brute4s99 updated this revision to Diff 59766.Jun 13 2019, 11:10 PM
brute4s99 updated this revision to Diff 59774.Jun 14 2019, 1:02 AM
brute4s99 updated this revision to Diff 59826.Jun 14 2019, 8:36 PM

rebased on upstream/master

brute4s99 updated this revision to Diff 59835.Jun 14 2019, 11:06 PM

updated acc to workaround for MSVC2019 suggested by Hannah
added inline comment for it at L92 -- notifybysnore.cpp

pino added a comment.Jun 15 2019, 8:24 AM

I think I should make a new diff for further discussions, as this one is quite riddled with suggestions now. Are there any more issues with this patch or should I continue with a new one instead?

Please do not drop this opening a new one, otherwise the whole discussion is basically killed...

src/CMakeLists.txt
49

what do you need XmlPatterns for?

51

this is indented too much

src/notifybysnore.cpp
45

if you could guide me on how to update the docs on the website, that'd be great!

which website?

81

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

84

QStringRef has toInt()

85

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

93

please document what is the issue, so in the future the workaround can be dropped

140–141

they are indented too much

169–172

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

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

201–203

no need to use it here, you already have the notification parameter...

pino added a comment.Jun 15 2019, 8:26 AM

Ah, one last thing I apparently forgot to mention so far: the names of class variables must start with m_.

vonreth added inline comments.Jun 15 2019, 10:15 AM
src/notifybysnore.cpp
73

deleteLater should also close the socket.
Also don't use a pointer after you deleted it, even with a deleteLater.

vonreth added inline comments.Jun 15 2019, 2:00 PM
src/notifybysnore.cpp
93

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.
Runs fine with msvc2017.
I tried to create a minimal example https://invent.kde.org/snippets/253 but this one works fine..

shubham added inline comments.
src/notifybysnore.h
45

I doubt this compiles...You can't initialise variables inside the class

vonreth added inline comments.Jun 15 2019, 8:03 PM
src/notifybysnore.h
45

Have you heard of this new c++ standard? C++11?

shubham removed a subscriber: shubham.Jun 16 2019, 5:58 AM
brute4s99 updated this revision to Diff 60035.Jun 19 2019, 5:09 AM
brute4s99 marked 9 inline comments as done.
brute4s99 added inline comments.Jun 19 2019, 5:09 AM
src/notifybysnore.cpp
45
85

well, I just found out the patch had broken functionality that I fixed just after putting here an else return! πŸ˜†

169–172

I'm removing this code chunk because we already check for successful show of notif in the following connect.

brute4s99 added inline comments.Jun 19 2019, 6:58 AM
src/notifybysnore.cpp
85

I have fixed the issue now.

brute4s99 marked 10 inline comments as done.Jun 26 2019, 8:20 AM
brute4s99 updated this revision to Diff 61084.Jul 3 2019, 3:59 PM

renamed variables (start with m_ now)

I believe I have covered all the issues with the patch now. (:

nicolasfella added inline comments.Jul 17 2019, 11:03 PM
src/CMakeLists.txt
48

find_package calls are usually in the toplevel CMakeLists.txt

src/knotification.cpp
378

where is the win32_defaults.notifyrc file?

brute4s99 updated this revision to Diff 62374.Jul 23 2019, 9:03 AM
brute4s99 marked an inline comment as done.

updated and rebased.

brute4s99 marked 2 inline comments as done.Jul 23 2019, 9:03 AM

Please do a last minute review, I believe this patch is ready to merge now. πŸŽ‰

brute4s99 added inline comments.Jul 27 2019, 9:41 AM
src/win32_defaults.notifyrc
90 β†—(On Diff #62374)

Can someone please confirm if this file is correctly used?

landing it...

brute4s99 updated this revision to Diff 63538.Aug 11 2019, 1:23 PM
  • removed the duplicate remover.

waiting for some green light(s) for now...

sredman accepted this revision.Aug 11 2019, 2:43 PM

I noticed a couple of places where the cmake file could be simplified so make those changes and then let's land this monster

CMakeLists.txt
104

Move this if(Android) block up to where the other *Extras are, lines 63-68 (then remove the connected else and then fix the indenting)

126

Logically this else is pretty separate from the if that it is connected to. Could you change it to if(NOT WIN32) to make it easier to read?

This revision is now accepted and ready to land.Aug 11 2019, 2:43 PM
brute4s99 marked 2 inline comments as done.Aug 11 2019, 3:14 PM

done.

brute4s99 updated this revision to Diff 63549.Aug 11 2019, 3:14 PM

landing this one. πŸŽ‰

vonreth added inline comments.Aug 11 2019, 5:53 PM
src/knotificationmanager.cpp
363

pls fix the duplication

brute4s99 updated this revision to Diff 63558.Aug 11 2019, 5:56 PM

removed duplication

brute4s99 marked an inline comment as done.Aug 11 2019, 5:56 PM
vonreth accepted this revision.Aug 11 2019, 6:07 PM
This revision was automatically updated to reflect the committed changes.

It looks like this made D-Bus and Phonon hard required dependencies on Android again!?

It looks like this made D-Bus and Phonon hard required dependencies on Android again!?

https://cgit.kde.org/knotifications.git/commit/?id=a56a379097d5159f1efa247a109c82e02872dfd2

this should fix that.

It looks like this made D-Bus and Phonon hard required dependencies on Android again!?

https://cgit.kde.org/knotifications.git/commit/?id=a56a379097d5159f1efa247a109c82e02872dfd2

this should fix that.

It does, thanks!