add snoretoast backend for KNotifications on Windows
Needs ReviewPublic

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
Branch
arcpatch-D21661
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13571
Build 13589: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
In D21661#476571, @pino wrote:
In D21661#476156, @pino wrote:

how is snoretoast actually used here? you are requiring the library for building and linking, but then:

  • snoretoastactions.h, which is part of the headers of snoretoast, is copied here
  • the snoretoast library is never used, as the utilities of it are invoked instead If the library does all the work already, then I'd prefer to use it directly instead of spawning executables all the time...

Piyush, what about this? It seems the snoretoast library provides a SnoreToasts class to do this instead of spawning an helper tool, what about using it instead?

@vonreth can probably explain better, but basically the situation as I understand it is on Windows you need to be installed in a special place and registered with the OS in order to show notifications. Since KNotifications is a library, an app using it can't (feasibly) be properly registered with the OS. It is possible we could come up with some complicated solution which would require every KNotification-using app to do some special and probably difficult to understand change to support Windows. Or we can have SnoreNotify.exe take care of all that nonsense for us. Note that, up to this point, there have been no special KNotifications changes to the generic KDE Connect codebase to make this work, just some tweaks to the Windows installer to pull in SnoreToast.

Spawning the process of a small, helper .exe is what other big cross-platform apps, like say Chromium/Google Chrome, do.

@pino Thank you very much for your reviews up to this point. It is very appreciated :). Supporting notifications on Windows is part of a GSoC project to release KDE Connect on Windows. We have a weekly VoIP meeting to talk about KDE Connect GSoC progress. If you would like to join those meetings, I can give you the information

In D21661#476571, @pino wrote:
In D21661#476156, @pino wrote:

how is snoretoast actually used here? you are requiring the library for building and linking, but then:

  • snoretoastactions.h, which is part of the headers of snoretoast, is copied here
  • the snoretoast library is never used, as the utilities of it are invoked instead If the library does all the work already, then I'd prefer to use it directly instead of spawning executables all the time...

Piyush, what about this? It seems the snoretoast library provides a SnoreToasts class to do this instead of spawning an helper tool, what about using it instead?

@vonreth can probably explain better, but basically the situation as I understand it is on Windows you need to be installed in a special place and registered with the OS in order to show notifications. Since KNotifications is a library, an app using it can't (feasibly) be properly registered with the OS. It is possible we could come up with some complicated solution which would require every KNotification-using app to do some special and probably difficult to understand change to support Windows. Or we can have SnoreNotify.exe take care of all that nonsense for us. Note that, up to this point, there have been no special KNotifications changes to the generic KDE Connect codebase to make this work, just some tweaks to the Windows installer to pull in SnoreToast.

So the location doesn't matter, but as far as I know its only possible to register the internal com server in an executable. We could make it a static lib and link it in all KDE applications, but to make the action center integration work, we would need to to also compile a class into the executable using a compile time uuid.

The solution with a external process isn't optimal, but the only I was able to come up with.

The used header is meant to help with parsing the response, the cmake target is a INTERFACE lib, it only provides the include path..

Spawning the process of a small, helper .exe is what other big cross-platform apps, like say Chromium/Google Chrome, do.

@pino Thank you very much for your reviews up to this point. It is very appreciated :). Supporting notifications on Windows is part of a GSoC project to release KDE Connect on Windows. We have a weekly VoIP meeting to talk about KDE Connect GSoC progress. If you would like to join those meetings, I can give you the information

vonreth added inline comments.Jun 9 2019, 10:59 AM
src/notifybysnore.cpp
47

@brute4s99 Have you heard of the how Qt manages the life time of QObjects?
If you pass this class as the parent object, the server will automatically get deleted once this class gets deleted

48

I think the server should be a bit more unique, what if two process of that name exist?
How about the full application path as a qHash?

57

same here, for this small amount of pairs a map is ok

154

deleteLater()

156

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?

181

Use proc->deleteLater() which will call the destructor https://doc.qt.io/qt-5/qprocess.html#dtor.QProcess

src/notifybysnore.h
45

The amount of notifications should be small in which case a map has a better performance.

47

I guess you could use both, server and icon dir as non pointer members, that way they would exist as long as NotifyBySnore exists.

albertvaka added inline comments.Jun 9 2019, 4:24 PM
src/notifybysnore.h
46

Isn't LibSnore abstracting this? Why do we need to call the exe instead of using some method in LibSnore?

vonreth added inline comments.Jun 9 2019, 4:47 PM
src/notifybysnore.h
46

Libsnore is something different, yes its an abstract "notification framework" but ... its basically unmaintained and the windows 8+ notifications use exactly this executable....

nicolasfella added inline comments.
src/knotificationmanager.cpp
150–151

No space before macro

src/notifybysnore.cpp
45

This should be documented somewhere else, too. Either the API dox or the KDE wiki

brute4s99 updated this revision to Diff 59572.Jun 11 2019, 2:55 AM

updated acc to new reviews by Pino and Hannah

Oh! Apologies, Pino. Actually I accidentally referred to you as toscanos from IRC, so I deleted that comment. I'll avoid deleting them from now on.

src/notifybysnore.cpp
39

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)

48

Ooh! Sounds fun! I can do that :D

src/notifybysnore.h
45
brute4s99 updated this revision to Diff 59573.Jun 11 2019, 3:02 AM
brute4s99 updated this revision to Diff 59574.Jun 11 2019, 3:04 AM
vonreth added inline comments.Jun 11 2019, 3:57 AM
src/notifybysnore.cpp
24

Is anything from Windows needed?
The header is huge.

50

FromStdString is wrong, use fromLatin1.
What about qHash, we don't ned cryptography algorithm here

181

Only delete once πŸ™ˆ

pino added a comment.Jun 11 2019, 5:00 AM
In D21661#476571, @pino wrote:

It seems the snoretoast library provides a SnoreToasts class to do this instead of spawning an helper tool, what about using it instead?

@vonreth can probably explain better, but basically the situation as I understand it is on Windows you need to be installed in a special place and registered with the OS in order to show notifications. Since KNotifications is a library, an app using it can't (feasibly) be properly registered with the OS. It is possible we could come up with some complicated solution which would require every KNotification-using app to do some special and probably difficult to understand change to support Windows. Or we can have SnoreNotify.exe take care of all that nonsense for us. Note that, up to this point, there have been no special KNotifications changes to the generic KDE Connect codebase to make this work, just some tweaks to the Windows installer to pull in SnoreToast.

IMHO this, plus what @vonreth adds later on, is exactly the kind of information that is important enough to be explained at least either as comment in the sources, or as commit message. This way people know why this approach was used, and they will not try to change things later on without dealing with the same situation.

Furthermore:

  • I still do think that is better to remove the cached image of a notification when removing it. Yes, they are stored in a QTemporaryDir directory, so they will be deleted when the application exists, although they will pile up and take more and more space. This is not a problem for short-living applications, but it is for long-running application, for example:
    • a media player showing the popup notification of a new song with its album cover, so every 3/4/5 minutes
    • a IM application showing messages from other users with their avatars when a message is received, so potentially even every few seconds
  • regarding the notification image: so far only the pixmap is set, although there are other ways to set the "artwork" to use for a notification: for example the IconName in the notifyrc configuration, or the iconName proeprty in KNotifyConfig; check what the NotifyByPopup plugin does, for example, to get them, and what the NotifyByAndroid plugin does to get an image from a theme icon name
src/CMakeLists.txt
48–49

still there

111

still there

src/notifybysnore.cpp
30

already included in the .h file

32

already included in the .h file

59

closing is good, although the socket object is still leaked

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

71

constFind

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)
src/notifybysnore.h
45

All the other KNotificationPlugins in knotification use QHash for indexing the KNotification objects, FWIW, so at least using QHash would keep a bit of consistency.

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.Wed, Jun 26, 8:20 AM
brute4s99 updated this revision to Diff 61084.Wed, Jul 3, 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.Wed, Jul 17, 11:03 PM
src/CMakeLists.txt
48

find_package calls are usually in the toplevel CMakeLists.txt

src/knotification.cpp
382

where is the win32_defaults.notifyrc file?