This is the main patch that adds the interfacing between KNotifications and SnoreToast.
Details
Diff Detail
- Repository
- R289 KNotifications
- Branch
- win32 (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 12556 Build 12574: arc lint + arc unit
src/notifybysnore.cpp | ||
---|---|---|
64 ↗ | (On Diff #59377) |
const auto parts = data.splitRef(...); for (... : parts)
|
65 ↗ | (On Diff #59377) | ditto, QStringLiteral -> QLatin1Char |
78 ↗ | (On Diff #59377) | no space between the enum and the colon (applies to the other lines too) |
116 ↗ | (On Diff #59377) | this is not needed, the destructor of QTemporaryDir will do it by default |
127 ↗ | (On Diff #59377) | move this variable directly in the code block where it is used |
132–133 ↗ | (On Diff #59377) | coding style: they go in the same line |
152 ↗ | (On Diff #59377) | better have this as very last operation done, so early returns will not have this notification in m_notifications |
153 ↗ | (On Diff #59377) | most probably check whether starting the process succeeded, and if not call finish(notification) and return earlier |
156 ↗ | (On Diff #59377) | 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 ↗ | (On Diff #59377) | 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:
|
src/notifybysnore.cpp | ||
---|---|---|
64 ↗ | (On Diff #59377) | splitRef is interesting, everyone learns in a code review. |
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 | ||
---|---|---|
44 ↗ | (On Diff #59377) | 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! 😃 |
updated acc to workaround for MSVC2019 suggested by Hannah
added inline comment for it at L92 -- notifybysnore.cpp
Please do not drop this opening a new one, otherwise the whole discussion is basically killed...
src/CMakeLists.txt | ||
---|---|---|
49 ↗ | (On Diff #59377) | what do you need XmlPatterns for? |
51 ↗ | (On Diff #59377) | this is indented too much |
src/notifybysnore.cpp | ||
44 ↗ | (On Diff #59377) |
which website? |
81 ↗ | (On Diff #59377) | 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 ↗ | (On Diff #59377) | QStringRef has toInt() |
85 ↗ | (On Diff #59377) | 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 ↗ | (On Diff #59377) | please document what is the issue, so in the future the workaround can be dropped |
140–141 ↗ | (On Diff #59377) | they are indented too much |
169–172 ↗ | (On Diff #59377) | 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 ↗ | (On Diff #59377) | 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 ↗ | (On Diff #59377) | no need to use it here, you already have the notification parameter... |
Ah, one last thing I apparently forgot to mention so far: the names of class variables must start with m_.
src/notifybysnore.cpp | ||
---|---|---|
73 ↗ | (On Diff #59377) | deleteLater should also close the socket. |
src/notifybysnore.cpp | ||
---|---|---|
93 ↗ | (On Diff #59377) | 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. |
src/notifybysnore.h | ||
---|---|---|
45 ↗ | (On Diff #59377) | I doubt this compiles...You can't initialise variables inside the class |
src/notifybysnore.h | ||
---|---|---|
45 ↗ | (On Diff #59377) | Have you heard of this new c++ standard? C++11? |
src/notifybysnore.cpp | ||
---|---|---|
44 ↗ | (On Diff #59377) | |
85 ↗ | (On Diff #59377) | well, I just found out the patch had broken functionality that I fixed just after putting here an else return! 😆 |
169–172 ↗ | (On Diff #59377) | I'm removing this code chunk because we already check for successful show of notif in the following connect. |
src/notifybysnore.cpp | ||
---|---|---|
85 ↗ | (On Diff #59377) | I have fixed the issue now. |
src/win32_defaults.notifyrc | ||
---|---|---|
90 ↗ | (On Diff #62374) | Can someone please confirm if this file is correctly used? |
I noticed a couple of places where the cmake file could be simplified so make those changes and then let's land this monster
CMakeLists.txt | ||
---|---|---|
76 ↗ | (On Diff #63538) | 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) |
98 ↗ | (On Diff #63538) | 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? |
src/knotificationmanager.cpp | ||
---|---|---|
380 ↗ | (On Diff #59377) | pls fix the duplication |
It looks like this made D-Bus and Phonon hard required dependencies on Android again!?