This is the main patch that adds the interfacing between KNotifications and SnoreToast.
const auto parts = data.splitRef(...); for (... : parts)
ditto, QStringLiteral -> QLatin1Char
no space between the enum and the colon (applies to the other lines too)
this is not needed, the destructor of QTemporaryDir will do it by default
move this variable directly in the code block where it is used
coding style: they go in the same line
better have this as very last operation done, so early returns will not have this notification in m_notifications
most probably check whether starting the process succeeded, and if not call finish(notification) and return earlier
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
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:
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! ✊
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! 😃
Please do not drop this opening a new one, otherwise the whole discussion is basically killed...
what do you need XmlPatterns for?
this is indented too much
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
QStringRef has toInt()
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
please document what is the issue, so in the future the workaround can be dropped
they are indented too much
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
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
no need to use it here, you already have the notification parameter...
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.
well, I just found out the patch had broken functionality that I fixed just after putting here an else return! 😆
I'm removing this code chunk because we already check for successful show of notif in the following connect.
I noticed a couple of places where the cmake file could be simplified so make those changes and then let's land this monster
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)
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?