D21661: add snoretoast backend for KNotifications on Windows

[Volker Krause]

vkrause added a comment.


  In D21661#511172 , @brute4s99 
wrote:
  
  > In D21661#510813 , @vkrause 
wrote:
  >
  > > 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!

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: vkrause, nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, 
ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 added a comment.


  In D21661#510813 , @vkrause wrote:
  
  > 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.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: vkrause, nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, 
ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

[Volker Krause]

vkrause added a comment.


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

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: vkrause, nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, 
ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

This revision was automatically updated to reflect the committed changes.
Closed by commit R289:d00612351778: add snoretoast backend for KNotifications 
on Windows (authored by brute4s99).

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21661?vs=63558=63569

REVISION DETAIL
  https://phabricator.kde.org/D21661

AFFECTED FILES
  CMakeLists.txt
  src/CMakeLists.txt
  src/knotification.cpp
  src/knotificationmanager.cpp
  src/knotifications.qrc
  src/notifybysnore.cpp
  src/notifybysnore.h

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Hannah von Reth]

vonreth accepted this revision.

REPOSITORY
  R289 KNotifications

BRANCH
  arcpatch-D21661

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 marked an inline comment as done.

REPOSITORY
  R289 KNotifications

BRANCH
  arcpatch-D21661

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 updated this revision to Diff 63558.
brute4s99 added a comment.


  removed duplication

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21661?vs=63551=63558

BRANCH
  arcpatch-D21661

REVISION DETAIL
  https://phabricator.kde.org/D21661

AFFECTED FILES
  CMakeLists.txt
  src/CMakeLists.txt
  src/knotification.cpp
  src/knotificationmanager.cpp
  src/knotifications.qrc
  src/notifybysnore.cpp
  src/notifybysnore.h

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Hannah von Reth]

vonreth added inline comments.

INLINE COMMENTS

> knotificationmanager.cpp:363
> +
> +#include "moc_knotificationmanager_p.cpp"

pls fix the duplication

REPOSITORY
  R289 KNotifications

BRANCH
  arcpatch-D21661

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 updated this revision to Diff 63551.
brute4s99 added a comment.


  rebased.

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21661?vs=63549=63551

BRANCH
  arcpatch-D21661

REVISION DETAIL
  https://phabricator.kde.org/D21661

AFFECTED FILES
  CMakeLists.txt
  src/CMakeLists.txt
  src/knotification.cpp
  src/knotificationmanager.cpp
  src/knotifications.qrc
  src/notifybysnore.cpp
  src/notifybysnore.h

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 updated this revision to Diff 63549.
brute4s99 added a comment.


  landing this one. 

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21661?vs=63538=63549

BRANCH
  arcpatch-D21661

REVISION DETAIL
  https://phabricator.kde.org/D21661

AFFECTED FILES
  CMakeLists.txt
  src/CMakeLists.txt
  src/knotification.cpp
  src/knotificationmanager.cpp
  src/knotifications.qrc
  src/notifybysnore.cpp
  src/notifybysnore.h

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 marked 2 inline comments as done.
brute4s99 added a comment.


  done.

REPOSITORY
  R289 KNotifications

BRANCH
  arcpatch-D21661

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Simon Redman]

sredman accepted this revision.
sredman added a comment.
This revision is now accepted and ready to land.


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

INLINE COMMENTS

> CMakeLists.txt:76
> +if (NOT WIN32)
> +if (ANDROID)
> +find_package(Qt5 ${REQUIRED_QT_VERSION} CONFIG REQUIRED 
> AndroidExtras)

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)

> CMakeLists.txt:98
> +endif()
> +else()
> +find_package(LibSnoreToast REQUIRED)

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?

REPOSITORY
  R289 KNotifications

BRANCH
  arcpatch-D21661

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 added a comment.


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

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 updated this revision to Diff 63538.
brute4s99 added a comment.


  - removed the duplicate remover.

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21661?vs=62374=63538

BRANCH
  arcpatch-D21661

REVISION DETAIL
  https://phabricator.kde.org/D21661

AFFECTED FILES
  CMakeLists.txt
  src/CMakeLists.txt
  src/knotification.cpp
  src/knotificationmanager.cpp
  src/knotifications.qrc
  src/notifybysnore.cpp
  src/notifybysnore.h

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 added a comment.


  landing it...

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 added inline comments.

INLINE COMMENTS

> win32_defaults.notifyrc:90
> +Comment[zh_TW]=程式發生了嚴重錯誤，必須離開
> +Action=Snore
> +

Can someone please confirm if this file is correctly used?

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, 
ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 added a comment.


  Please do a last minute review, I believe this patch is ready to merge now. 

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, 
ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 marked 2 inline comments as done.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, 
ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 updated this revision to Diff 62374.
brute4s99 marked an inline comment as done.
brute4s99 added a comment.


  updated and rebased.

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21661?vs=61084=62374

BRANCH
  arcpatch-D21661

REVISION DETAIL
  https://phabricator.kde.org/D21661

AFFECTED FILES
  CMakeLists.txt
  src/CMakeLists.txt
  src/knotification.cpp
  src/knotificationmanager.cpp
  src/knotifications.qrc
  src/notifybysnore.cpp
  src/notifybysnore.h
  src/win32_defaults.notifyrc

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, 
ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

[Nicolas Fella]

nicolasfella added inline comments.

INLINE COMMENTS

> CMakeLists.txt:48
> +   )
> +  find_package(Qt5Network REQUIRED)
> +  list(APPEND knotifications_SRCS notifybysnore.cpp)

find_package calls are usually in the toplevel CMakeLists.txt

> knotification.cpp:382
> +#elif defined(Q_OS_WIN)
> +return QStringLiteral("win32_defaults");
> +#else

where is the win32_defaults.notifyrc file?

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, 
ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 added a comment.


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

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 updated this revision to Diff 61084.
brute4s99 added a comment.


  renamed variables (start with `m_` now)

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21661?vs=60035=61084

BRANCH
  arcpatch-D21661

REVISION DETAIL
  https://phabricator.kde.org/D21661

AFFECTED FILES
  src/CMakeLists.txt
  src/knotification.cpp
  src/knotificationmanager.cpp
  src/notifybysnore.cpp
  src/notifybysnore.h

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 marked 10 inline comments as done.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 added inline comments.

INLINE COMMENTS

> brute4s99 wrote in notifybysnore.cpp:84
> well, I just found out the patch had broken functionality that I fixed just 
> after putting here an `else return`! 

I have fixed the issue now.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 updated this revision to Diff 60035.
brute4s99 marked 9 inline comments as done.

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21661?vs=59835=60035

BRANCH
  arcpatch-D21661

REVISION DETAIL
  https://phabricator.kde.org/D21661

AFFECTED FILES
  src/CMakeLists.txt
  src/knotification.cpp
  src/knotificationmanager.cpp
  src/notifybysnore.cpp
  src/notifybysnore.h

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 added inline comments.

INLINE COMMENTS

> pino wrote in notifybysnore.cpp:84
> 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

well, I just found out the patch had broken functionality that I fixed just 
after putting here an `else return`! 

> pino wrote in notifybysnore.cpp:168-171
> 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

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

> pino wrote in notifybysnore.cpp:44
> > if you could guide me on how to update the docs on the website, that'd be 
> > great!
> 
> which website?

https://api.kde.org/frameworks/knotifications/html/index.html

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Hannah von Reth]

vonreth added inline comments.

INLINE COMMENTS

> shubham wrote in notifybysnore.h:44
> I doubt this compiles...You can't initialise variables inside the class

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

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: shubham, nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, 
ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

[Shubham]

shubham added inline comments.

INLINE COMMENTS

> notifybysnore.h:44
> +QHash> m_notifications;
> +QString program = QStringLiteral("SnoreToast.exe");
> +QLocalServer server;

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

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: shubham, nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, 
ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

[Hannah von Reth]

vonreth added inline comments.

INLINE COMMENTS

> pino wrote in notifybysnore.cpp:92
> please document what is the issue, so in the future the workaround can be 
> dropped

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

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Hannah von Reth]

vonreth added inline comments.

INLINE COMMENTS

> notifybysnore.cpp:72
> +sock->deleteLater();
> +sock->close();
> +const QString data =

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

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Pino Toscano]

pino added a comment.


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

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Pino Toscano]

pino added a comment.


  In D21661#479366 , @brute4s99 
wrote:
  
  > 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...

INLINE COMMENTS

> CMakeLists.txt:49
> +  find_package(Qt5Network REQUIRED)
> +  find_package(Qt5XmlPatterns REQUIRED)
> +  list(APPEND knotifications_SRCS notifybysnore.cpp)

what do you need XmlPatterns for?

> CMakeLists.txt:51
> +  list(APPEND knotifications_SRCS notifybysnore.cpp)
> +  endif ()
> +

this is indented too much

> notifybysnore.cpp:80
> +const auto index = str.indexOf(QLatin1Char('='));
> +map.insert(str.toString().mid(0, index), str.mid(index + 1));
> +}

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

> notifybysnore.cpp:83
> +const auto action = map[QStringLiteral("action")].toString();
> +const auto id = 
> map[QStringLiteral("notificationId")].toString().toInt();
> +KNotification *notification;

QStringRef has toInt()

> notifybysnore.cpp:84
> +const auto id = 
> map[QStringLiteral("notificationId")].toString().toInt();
> +KNotification *notification;
> +const auto it = m_notifications.constFind(id);

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

> notifybysnore.cpp:92
> +const auto snoreAction = SnoreToastActions::getAction(waction);
> +// MSVC2019 has issues with QString::toStdWString()
> +

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

> notifybysnore.cpp:139-140
> +if (m_notifications.constFind(notification->id()) != 
> m_notifications.constEnd()) {
> +qCDebug(LOG_KNOTIFICATIONS) << "Duplicate notification with ID: 
> " << notification->id() << " ignored.";
> +return;
> +}

they are indented too much

> notifybysnore.cpp:168-171
> +if (!proc->waitForStarted()) {
> +} else {
> +finish(notification);
> +}

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

> notifybysnore.cpp:178-179
> +if (exitStatus == QProcess::NormalExit) {
> +QFile::remove(QString(iconDir.path() + QLatin1Char('/') 
> ++ QString::number(notification->id()) + 
> QStringLiteral(".png")));
> +} else {

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

> notifybysnore.cpp:200-202
> +if (it.value()) {
> +finish(it.value());
> +}

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

> brute4s99 wrote in notifybysnore.cpp:44
> 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! 

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

which website?

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 updated this revision to Diff 59835.
brute4s99 added a comment.


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

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21661?vs=59826=59835

BRANCH
  win32 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21661

AFFECTED FILES
  src/CMakeLists.txt
  src/knotification.cpp
  src/knotificationmanager.cpp
  src/notifybysnore.cpp
  src/notifybysnore.h

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 updated this revision to Diff 59826.
brute4s99 added a comment.


  rebased on upstream/master

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21661?vs=59774=59826

BRANCH
  win32 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21661

AFFECTED FILES
  src/CMakeLists.txt
  src/knotification.cpp
  src/knotificationmanager.cpp
  src/notifybysnore.cpp
  src/notifybysnore.h

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 updated this revision to Diff 59774.

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21661?vs=59766=59774

BRANCH
  win32 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21661

AFFECTED FILES
  src/CMakeLists.txt
  src/knotification.cpp
  src/knotificationmanager.cpp
  src/notifybysnore.cpp
  src/notifybysnore.h

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 updated this revision to Diff 59766.

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21661?vs=59765=59766

BRANCH
  win32 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21661

AFFECTED FILES
  src/CMakeLists.txt
  src/knotification.cpp
  src/knotificationmanager.cpp
  src/notifybysnore.cpp
  src/notifybysnore.h

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 updated this revision to Diff 59765.
brute4s99 marked an inline comment as done.

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21661?vs=59574=59765

BRANCH
  win32 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21661

AFFECTED FILES
  src/CMakeLists.txt
  src/knotification.cpp
  src/knotificationmanager.cpp
  src/notifybysnore.cpp
  src/notifybysnore.h

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 marked 67 inline comments as done.
brute4s99 added a comment.


  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! ✊

INLINE COMMENTS

> nicolasfella wrote in notifybysnore.cpp:44
> This should be documented somewhere else, too. Either the API dox or the KDE 
> wiki

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! 

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Hannah von Reth]

vonreth added inline comments.

INLINE COMMENTS

> pino wrote in notifybysnore.cpp:63
> - 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)

splitRef

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

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Pino Toscano]

pino added a comment.


  In D21661#476607 , @sredman wrote:
  
  > 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

INLINE COMMENTS

> pino wrote in CMakeLists.txt:112
> `Qt5::Core` is not needed here; if we really want to be pedantic, it ought to 
> be added unconditionally (however that would be a different patch than this 
> one)

still there

> pino wrote in CMakeLists.txt:48-49
> Qt5Core is already pretty much required to build anything using Qt5, so 
> looking for it in this place is a no-op (because it was already found)

still there

> notifybysnore.cpp:29
> +#include 
> +#include 
> +#include 

already included in the .h file

> notifybysnore.cpp:31
> +#include 
> +#include 
> +#include 

already included in the .h file

> notifybysnore.cpp:58
> +const QByteArray rawData = sock->readAll();
> +sock->close();
> +const QString data =

closing is good, although the socket object is still leaked

> notifybysnore.cpp:63
> +QMap map;
> +for (const auto  : data.split(QStringLiteral(";"))) {
> +const auto index = str.indexOf(QStringLiteral("="));

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

> notifybysnore.cpp:64
> +for (const auto  : data.split(QStringLiteral(";"))) {
> +const auto index = str.indexOf(QStringLiteral("="));
> +map.insert(str.mid(0, index), str.mid(index + 1));

ditto, QStringLiteral -> QLatin1Char

> notifybysnore.cpp:70
> +KNotification *notification = nullptr;
> +const auto it = m_notifications.find(id);
> +if (it != m_notifications.end()) {

constFind

> notifybysnore.cpp:71
> +const auto it = m_notifications.find(id);
> +if (it != m_notifications.end()) {
> +notification = it.value();

constEnd

> notifybysnore.cpp:77
> +switch (snoreAction) {
> +case SnoreToastActions::Actions::Clicked :
> +qCDebug(LOG_KNOTIFICATIONS) << " User clicked on the toast.";

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

> notifybysnore.cpp:115
> +server.close();
> +iconDir.remove();
> +}

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

> notifybysnore.cpp:126
> +QStringList arguments;
> 

D21661: add snoretoast backend for KNotifications on Windows

[Hannah von Reth]

vonreth added inline comments.

INLINE COMMENTS

> notifybysnore.cpp:23
> +
> +#include 
> +

Is anything from Windows needed?
The header is huge.

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

Only delete once 

> pino wrote in notifybysnore.cpp:49
> `iconDir` is leaked

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

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 updated this revision to Diff 59574.

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21661?vs=59573=59574

BRANCH
  win32 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21661

AFFECTED FILES
  src/CMakeLists.txt
  src/knotification.cpp
  src/knotificationmanager.cpp
  src/notifybysnore.cpp
  src/notifybysnore.h

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 updated this revision to Diff 59573.

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21661?vs=59572=59573

BRANCH
  win32 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21661

AFFECTED FILES
  src/CMakeLists.txt
  src/knotification.cpp
  src/knotificationmanager.cpp
  src/notifybyportal.h
  src/notifybysnore.cpp
  src/notifybysnore.h

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 added a comment.


  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.

INLINE COMMENTS

> pino wrote in notifybysnore.cpp:38
> `QCoreApplication` is enough (see below)

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)

> vonreth wrote in notifybysnore.cpp:47
> 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?

Ooh! Sounds fun! I can do that :D

> vonreth wrote in notifybysnore.h:44
> The amount of notifications should be small in which case a map has a better 
> performance.

I referred to this: https://woboq.com/blog/qmap_qhash_benchmark.html

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 updated this revision to Diff 59572.
brute4s99 added a comment.


  updated acc to new reviews by Pino and Hannah

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21661?vs=59425=59572

BRANCH
  win32 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21661

AFFECTED FILES
  src/CMakeLists.txt
  src/knotification.cpp
  src/knotificationmanager.cpp
  src/notifybyportal.h
  src/notifybysnore.cpp
  src/notifybysnore.h

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Nicolas Fella]

nicolasfella added inline comments.

INLINE COMMENTS

> knotificationmanager.cpp:140
>  if (action == QLatin1String("Popup")) {
> -#ifndef Q_OS_ANDROID
> -if (d->portalDBusServiceExists) {
> -plugin = new NotifyByPortal(this);
> -} else {
> -plugin = new NotifyByPopup(this);
> -}
> -#else
> -plugin = new NotifyByAndroid(this);
> -#endif
> -
> +#if defined(Q_OS_ANDROID)
> +plugin = new NotifyByAndroid(this);

No space before macro

> brute4s99 wrote in notifybysnore.cpp:44
> Yep! In 
> https://piyushagg.home.blog/2019/06/07/gsoc19-project-milestone-1/

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

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D21661: add snoretoast backend for KNotifications on Windows

[Hannah von Reth]

vonreth added inline comments.

INLINE COMMENTS

> albertvaka wrote in notifybysnore.h:45
> Isn't LibSnore abstracting this? Why do we need to call the exe instead of 
> using some method in LibSnore?

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

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

[Albert Vaca Cintora]

albertvaka added inline comments.

INLINE COMMENTS

> notifybysnore.h:45
> +QMap> m_notifications;
> +QString program = QStringLiteral("SnoreToast.exe");
> +QLocalServer *server;

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

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

[Hannah von Reth]

vonreth added inline comments.

INLINE COMMENTS

> notifybysnore.cpp:46
> +iconDir = new QTemporaryDir();
> +server = new QLocalServer();
> +server->listen(qApp->applicationName());

@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

> notifybysnore.cpp:47
> +server = new QLocalServer();
> +server->listen(qApp->applicationName());
> +

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?

> pino wrote in notifybysnore.cpp:56
> QMap -> QHash

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

> notifybysnore.cpp:153
> +[=](int exitCode, QProcess::ExitStatus exitStatus){ 
> +proc->close();
> +QFile file(iconPath);

deleteLater()

> notifybysnore.cpp:155
> +QFile file(iconPath);
> +file.remove();
> +});

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?

> notifybysnore.cpp:180
> +[=](int exitCode, QProcess::ExitStatus exitStatus){ 
> +proc->close();
> +});

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

> pino wrote in notifybysnore.h:44
> QMap -> QHash

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

> notifybysnore.h:46
> +QString program = QStringLiteral("SnoreToast.exe");
> +QLocalServer *server;
> +QTemporaryDir *iconDir;

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

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

[Hannah von Reth]

vonreth added a comment.


  In D21661#476607 , @sredman wrote:
  
  > 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

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

[Simon Redman]

sredman added a comment.


  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

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

[Pino Toscano]

pino added a comment.


  > This comment was removed by brute4s99.
  
  This comment was:
  
  > updated wrt review by toscanos
  
  Please do **NOT** remove your own comments. Other than being anti-social 
(imagine how easily you can twist conversations), in this case it's perfectly 
useless, as the default assignee for these reviews is a mailing list, so the 
original message is archived forever.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

[Pino Toscano]

pino added inline comments.

INLINE COMMENTS

> CMakeLists.txt:112
> +if (TARGET SnoreToast::SnoreToastActions)
> +  target_link_libraries(KF5Notifications PRIVATE Qt5::Core Qt5::Network 
> SnoreToast::SnoreToastActions)
> +endif ()

`Qt5::Core` is not needed here; if we really want to be pedantic, it ought to 
be added unconditionally (however that would be a different patch than this one)

> brute4s99 wrote in CMakeLists.txt:48-49
> the interfacing plugin (notifybysnore) requires QLocalServer and 
> QLocalSocket, which are from Qt5Network.
> Qt5Core has QProcess, QDir, QTemporaryDir and a bunch of other headers, which 
> are required for the plugin as well.

Qt5Core is already pretty much required to build anything using Qt5, so looking 
for it in this place is a no-op (because it was already found)

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

[Pino Toscano]

pino added a comment.


  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?
  
  > Also: please format all the new code according to the kdelibs coding style 
-- for example `notifybysnore.cpp`.
  
  Still needs to be done.  Please make sure to indent by 4 spaces only, no 
tabs; also mind the spacing around conditions of `if`s, etc:
  https://community.kde.org/Policies/Kdelibs_Coding_Style
  
  Furthermore: there is a big switch on `snoreAction` that reacts only on 
buttons clicked; some of the other actions hint that the notification was 
closed, so most probably the KNotification object ought to be closed too. Check 
what the `NotifyByPopup` plugins does, for example, as there is something 
similar done.

INLINE COMMENTS

> knotificationmanager.cpp:45
> +#if defined(Q_OS_ANDROID)
> +#include "notifybyandroid.h"
> +#elif defined(Q_OS_WIN)

do not add spaces before the `#` of C preprocessor tokens, as it is 
undefined/nonstandard behaviour

> knotificationmanager.cpp:145
> +#else
> +if (d->inSandbox && d->portalDBusServiceExists) {
> +plugin = new NotifyByPortal(this);

this change is still unrelated -- are you really sure you rebased your work on 
top of the current master branch?

> notifybysnore.cpp:26
> +#include 
> +#include 
> +#include 

not needed (`debug_p.h` is already included)

> notifybysnore.cpp:30
> +#include 
> +#include 
> +

not needed

> notifybysnore.cpp:31-32
> +#include 
> +
> +
> +#include 

extra empty lines

> notifybysnore.cpp:34
> +#include 
> +
> +#include 

extra empty line

> notifybysnore.cpp:38
> +#include 
> +#include 
> +

`QCoreApplication` is enough (see below)

> notifybysnore.cpp:47
> +server = new QLocalServer();
> +server->listen(qApp->applicationName());
> +

`QCoreApplication::applicationName()` is static, so use it directly as static 
(this applies to all the uses)

> notifybysnore.cpp:50
> +QObject::connect(server, ::newConnection, server, [this]() {
> +auto sock = server->nextPendingConnection();
> +sock->waitForReadyRead();

sock is not closed and leaked

> notifybysnore.cpp:56
> +rawData.size() / sizeof(wchar_t));
> +QMap map;
> +for (const auto  : data.split(QStringLiteral(";"))) {

QMap -> QHash

> notifybysnore.cpp:65
> +const auto snoreAction = 
> SnoreToastActions::getAction(action.toStdWString());
> +qCDebug(LOG_KNOTIFICATIONS) << "The notification ID is : " << 
> QString::number(id);
> +switch(snoreAction){

QDebug can print numbers directly, no need to convert it to string manually

> notifybysnore.cpp:119
> +QStringList arguments;
> +QString iconPath(QStringLiteral(""));
> +

no need to initialize it here -- it can go directly in the if block where it is 
used

> notifybysnore.cpp:120
> +QString iconPath(QStringLiteral(""));
> +
> +

extra empty line

> notifybysnore.cpp:133
> +if(!notification->pixmap().isNull()){
> +iconPath.append(iconDir->path() + QStringLiteral("/") 
> ++ QString::number(notification->id()) + 
> QStringLiteral(".png"));

no need to append, just assign directly (iconPath was empty before, anyway)

> notifybysnore.cpp:147
> +
> +qDebug() << arguments;
> +m_notifications.insert(notification->id(), notification);

either it uses the same debug category of the other message, or it is dropped

> notifybysnore.cpp:154-155
> +proc->close();
> +QFile file(iconPath);
> +file.remove();
> +});

There is a static `QFile::remove()`, so use it directly

> notifybysnore.cpp:154-155
> +proc->close();
> +QFile file(iconPath);
> +file.remove();
> +});

won't this remove the image too early? this will remove the image once the 
process ends, not when notification is closed; this fits better in `close()`

> notifybysnore.cpp:176
> +}
> +arguments.clear();
> +m_notifications.erase(it);

no need to clear this here, it will be deleted at the end of the function 
anyway...

> notifybysnore.cpp:190
> +}
> \ No newline at end of file


C/C++ source require an empty newline at the end

> pino wrote in notifybysnore.cpp:49
> `iconDir` is leaked

this is still not fixed

> pino wrote in notifybysnore.cpp:50
> `server` is leaked

this is still not fixed

> pino wrote in 

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 marked 4 inline comments as done.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 updated this revision to Diff 59425.
brute4s99 marked 2 inline comments as done.
brute4s99 added a comment.


  updated acc to review by Hannah

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21661?vs=59401=59425

BRANCH
  win32 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21661

AFFECTED FILES
  src/CMakeLists.txt
  src/knotification.cpp
  src/knotificationmanager.cpp
  src/notifybysnore.cpp
  src/notifybysnore.h

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

[Hannah von Reth]

vonreth added inline comments.

INLINE COMMENTS

> CMakeLists.txt:45
> +   set_package_properties(LibSnoreToast PROPERTIES TYPE REQUIRED
> +  PURPOSE "Enable support for the snorenotify framework"
> +  DESCRIPTION "a cross-platform notification framework"

It's snoretoast not Snorenotify  so you used the wrong description

> pino wrote in CMakeLists.txt:112
> this should be the `_FOUND` variable, just like done for the others

Hm I don't think the config sets a found variable. But

  if (TARGET Snoretoast::Snoretoast)

Actions should work.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 added a comment.


  updating acc to review by pino

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 marked an inline comment as done.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 updated this revision to Diff 59401.
brute4s99 marked an inline comment as done.
brute4s99 added a comment.


  updated wrt review by toscanos

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21661?vs=59379=59401

BRANCH
  win32 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21661

AFFECTED FILES
  src/CMakeLists.txt
  src/knotification.cpp
  src/knotificationmanager.cpp
  src/notifybysnore.cpp
  src/notifybysnore.h

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 marked 22 inline comments as done and an inline comment as not done.
brute4s99 added a comment.


  update incoming

INLINE COMMENTS

> pino wrote in CMakeLists.txt:48-49
> why are these two needed? if snoretoast require them, then its cmake config 
> file must require them, so that the above `find_package(LibSnoreToast)` is 
> enough

the interfacing plugin (notifybysnore) requires QLocalServer and QLocalSocket, 
which are from Qt5Network.
Qt5Core has QProcess, QDir, QTemporaryDir and a bunch of other headers, which 
are required for the plugin as well.

> pino wrote in knotificationmanager.cpp:76
> this does not seem related to snoretoast

yeah, looks like it picked up changes from commits on master. apologies for 
this.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

[Pino Toscano]

pino added inline comments.

INLINE COMMENTS

> notifybysnore.cpp:115
> +{
> +if (m_notifications.find(notification->id()) != m_notifications.end()) {
> +qCDebug(LOG_KNOTIFICATIONS) << "Duplicate notification with ID: 
> " << notification->id() << " ignored.";

since this method is not const, prefer using constFind + constEnd for the hash 
lookup

> notifybysnore.cpp:119-121
> +if(notification->id() == -1){
> +qCDebug(LOG_KNOTIFICATIONS) << "Notification with ID : \"-1\" 
> ignored.";
> +}

none of the other plugins seem to check for id == -1 here, so I'd say this 
check can be dropped

> notifybysnore.cpp:132
> +arguments << QStringLiteral("-m") << notification->text();
> +arguments << QStringLiteral("-p") <<  file.fileName();
> +arguments << QStringLiteral("-appID") << app->applicationName();

if the notification has no pixmap (see `!notification->pixmap().isNull()` check 
above), then I guess you do not need to pass this argument, as the image will 
not exist

> notifybysnore.cpp:161-163
> +if (it.value()) {
> +finish(it.value());
> +}

`it` is used after erasing it from `m_notifications`, and that means that 
iterator points to nothing; you cannot use iterators after you remove them from 
their container

you already have `notification`, so just use it instead

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

[Pino Toscano]

pino added a comment.


  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...
  
  Also: please format all the new code according to the kdelibs coding style -- 
for example `notifybysnore.cpp`.

INLINE COMMENTS

> CMakeLists.txt:44
> +  find_package(LibSnoreToast REQUIRED)
> +   set_package_properties(LibSnoreToast PROPERTIES TYPE OPTIONAL
> +  PURPOSE "Enable support for the snorenotify framework"

the find_package above is REQUIRED, while here it is marked as OPTIONAL

> CMakeLists.txt:48-49
> +
> +  find_package(Qt5Core REQUIRED)
> +  find_package(Qt5Network REQUIRED)
> +  list(APPEND knotifications_SRCS notifybysnore.cpp)

why are these two needed? if snoretoast require them, then its cmake config 
file must require them, so that the above `find_package(LibSnoreToast)` is 
enough

> CMakeLists.txt:112
>  )
> +if (MSVC)
> +  target_link_libraries(KF5Notifications PRIVATE Qt5::Core Qt5::Network 
> SnoreToast::SnoreToastActions)

this should be the `_FOUND` variable, just like done for the others

> knotification.cpp:381-382
>  return QStringLiteral("android_defaults");
> +#else
> +#ifdef Q_OS_WIN
> +return QStringLiteral("win32_defaults");

#elif defined(Q_OS_WIN)

> knotificationmanager.cpp:46-47
> +#include "notifybyandroid.h"
>  #else
> -#include "notifybyandroid.h"
> +#ifdef Q_OS_WIN
> +#include "notifybysnore.h"

#elif defined(Q_OS_WIN)

> knotificationmanager.cpp:76
>  QStringList dirtyConfigCache;
> +bool inSandbox = false;
>  bool portalDBusServiceExists = false;

this does not seem related to snoretoast

> knotificationmanager.cpp:100-110
> +if (!qEnvironmentVariableIsEmpty("XDG_RUNTIME_DIR")) {
> +const QByteArray runtimeDir = qgetenv("XDG_RUNTIME_DIR");
> +if (!runtimeDir.isEmpty()) {
> +d->inSandbox = QFileInfo::exists(QFile::decodeName(runtimeDir) + 
> QLatin1String("/flatpak-info"));
> +}
> +} else if (qEnvironmentVariableIsSet("SNAP")) {
> +d->inSandbox = true;

this does not seem related to snoretoast

> knotificationmanager.cpp:152-153
> +plugin = new NotifyByAndroid(this);
> +#else
> +#ifdef Q_OS_WIN
> +plugin = new NotifyBySnore(this);

#elif defined(Q_OS_WIN)

> notifybysnore.cpp:42
> +
> +static NotifyBySnore *s_instance = nullptr;
> +

this does not seem used -- just drop

> notifybysnore.cpp:49
> +app = QCoreApplication::instance();
> +iconDir = new QTemporaryDir();
> +server = new QLocalServer();

`iconDir` is leaked

> notifybysnore.cpp:50
> +iconDir = new QTemporaryDir();
> +server = new QLocalServer();
> +server->listen(app->applicationName());

`server` is leaked

> notifybysnore.cpp:63
> +const auto index = str.indexOf(QStringLiteral("="));
> +map[str.mid(0, index)] = str.mid(index + 1);
> +}

map.insert() is slightly better here

> notifybysnore.cpp:93
> +int action_no = s.indexOf(button) + 1; 
> // QStringList starts with index 0 but not actions
> +
> NotifyBySnore::notificationActionInvoked(ID, action_no);
> +break;

notificationActionInvoked is not a static method, so do not call it as such

> notifybysnore.cpp:122
> +}
> +QProcess *proc = new QProcess();
> +

`proc` is leaked

> notifybysnore.cpp:125
> +QStringList arguments;
> +QFile file(iconDir->path() + QString::number(notification->id()));
> +if (!notification->pixmap().isNull()) {

does `QTemporaryDir::path()` return a path with a trailing path separator? if 
not, this path is not within the temporary directory

also, maybe add a `.png` extension to the file (mostly to ease its handling)

> notifybysnore.cpp:127
> +if (!notification->pixmap().isNull()) {
> +notification->pixmap().save(, "PNG");
> +}

the QFile is not open, so I'm not sure whether this will work at all; OTOH, you 
do not need to create a QFile to save a QPixmap, just pass the file path to 
save()

also, assuming these images are created within a temporary directory, they will 
be deleted only at the application exit: this means that long-running 
applications that have notifications with images (for example media players, IM 
apps, etc) will consume more and more disk space; please delete each image once 
its notification is done

> notifybysnore.cpp:153
> +
> +QProcess *proc = new QProcess();
> +QStringList arguments;

`proc` is leaked

> notifybysnore.cpp:172-175
> +void NotifyBySnore::notificationActionInvoked(int id, int action)
> +{
> + 

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 updated this revision to Diff 59379.
brute4s99 marked 2 inline comments as done.
brute4s99 added a comment.


  updated wrt review.

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21661?vs=59377=59379

BRANCH
  win32 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21661

AFFECTED FILES
  src/CMakeLists.txt
  src/knotification.cpp
  src/knotificationmanager.cpp
  src/notifybysnore.cpp
  src/notifybysnore.h
  src/snoretoastactions.h

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 marked 5 inline comments as done.
brute4s99 added inline comments.

INLINE COMMENTS

> sredman wrote in CMakeLists.txt:43
> Do you know whether this requires MSVC, or can SnoreToast.exe be built with 
> MSVC and KNotifications be built with an unspecified compiler?

AFAIU it *should work fine with any compiler*, once SnoreToast is installed 
within the system one way or the other. (:

> sredman wrote in notifybysnore.cpp:44
> Did this end up being documented?

Yep! In 
https://piyushagg.home.blog/2019/06/07/gsoc19-project-milestone-1/

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

[Simon Redman]

sredman added inline comments.

INLINE COMMENTS

> CMakeLists.txt:43
> +if (WIN32)
> +  if (MSVC)
> +  find_package(LibSnoreToast REQUIRED)

Do you know whether this requires MSVC, or can SnoreToast.exe be built with 
MSVC and KNotifications be built with an unspecified compiler?

> notifybysnore.cpp:44
> +
> +// !DOCUMENT THIS! apps must have shortcut appID same as 
> app->applicationName()
> +NotifyBySnore::NotifyBySnore(QObject* parent) :

Did this end up being documented?

> notifybysnore.cpp:73
> +case SnoreToastActions::Actions::Clicked :{
> +qDebug() << " User clicked on the 
> toast.";
> +break;

Add a category to qDebug, like `qCDebug(LOG_KNOTIFICATIONS) << "Message";`. I 
can't figure out where `LOG_KNOTIFICATIONS` is coming from, but other backends 
seem to use it :)

> notifybysnore.cpp:117
> +if (m_notifications.find(notification->id()) != m_notifications.end() || 
> notification->id() == -1) {
> +qDebug() << "AHAA ! Duplicate for ID: " << notification->id() << 
> " caught!";
> +return;

Maybe this log line should be different? :)

> notifybysnore.cpp:155
> +  << QStringLiteral("-appID") << app->applicationName();
> +;
> +proc->start(program, arguments);

Does this `;` belong to something?

> notifybysnore.h:48
> +QString program = QStringLiteral("SnoreToast.exe");
> +// QProcess *proc;
> +QLocalServer *server;

Delete commented code

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 edited the summary of this revision.
brute4s99 added reviewers: broulik, sredman, vonreth, albertvaka.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 added a dependency: D21602: Add NSIS Script for KDE Connect.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21661

To: brute4s99
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

[Piyush Aggarwal]

brute4s99 created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
brute4s99 requested review of this revision.

REPOSITORY
  R289 KNotifications

BRANCH
  win32 (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21661

AFFECTED FILES
  src/CMakeLists.txt
  src/knotification.cpp
  src/knotificationmanager.cpp
  src/notifybysnore.cpp
  src/notifybysnore.h
  src/snoretoastactions.h

To: brute4s99
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns