D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

brute4s99 added a comment. In D21661#510813 , @vkrause wrote: > It looks like this made D-Bus and Phonon hard required dependencies on Android again!?

D21661: add snoretoast backend for KNotifications on Windows

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,

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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

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

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

D21661: add snoretoast backend for KNotifications on Windows

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,

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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,

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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

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

D21661: add snoretoast backend for KNotifications on Windows

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

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,

D21661: add snoretoast backend for KNotifications on Windows

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,

D21661: add snoretoast backend for KNotifications on Windows

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

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

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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,

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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

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

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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,

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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?

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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 >

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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?

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

vonreth added a comment. In D21661#476607 , @sredman wrote: > In D21661#476571 , @pino wrote: > > > In D21661#476156 , @pino wrote: > > > > > how is

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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

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

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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

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

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

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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)

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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

D21661: add snoretoast backend for KNotifications on Windows

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,

D21661: add snoretoast backend for KNotifications on Windows

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

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