Please review Snorenotify (Finish Incubating)

[Aleix Pol]

On Wed, Nov 18, 2015 at 11:48 PM, Hannah von Reth <vonreth at kde.org> wrote:
> Hi there,
>
> Mario guided me until now through the incubation process and we think it is
> time to move Snorenotify from playground to extragear.
> Snorenotify is a notification framework supporting Linux, Windows and Mac
> OSX.
> It is not meant to replace Knotifications, it is more targeted on Qt
> applications without dependency to the plasma desktop, so it might become a
> backend for Knotifications.
>
> I guess you can find the most important information here
> https://community.kde.org/Incubator/Projects/Snorenotify.
>
> Besides Snorenotify there is also Snoretoast, a sub project of Snorenotify,
> https://quickgit.kde.org/?p=snoretoast.git.
> Snoretoast  is a command line application and used within Snorenotify for
> the Windows Toast notifications.
> The application can only be build using the Microsoft compiler.
>
> So it would be great if Snorenotify could become a official KDE library and
> maybe even a framework someday.
> Currently it is used by Quassel and Tomahawk but hopefully more will start
> to use it soon.
>
> So please review Snorenotify.
>
> If you find the idea of Snorenotify useful or you fancy notifications, like
> I do, feel free to contribute ;)  or start to use Snorenotify.

Hi Hannah,
I'm happy that you're decided to push this forward, I think it's a
framework that could be definitely useful. In KDE Connect we'd like to
be able to use it instead of KNotifications because it's leaner and
more straightforward to our notifications use-case. As I said before,
I already ported KDE Connect to use snore, nevertheless there's some
issues like the API that I would suggest to review before committing
to API and ABI stability.

Here's some thoughts:
- It feels overly complex that the Notification object is passed
around by value rather than by reference. For starters, being able to
connect to the notification would be very handy. I work-arounded it by
setting a hint with the relevant information, but I have the feeling
the API would be slightly smoother.
- There's a hard dependency on QtWidgets from the very core of the
framework. This requires applications that would use it to use
QApplication. In the case of KDE Connect, for instance, the plan was
to make it possible to have it in a QCoreApplication (or
QGuiApplication at least). It's a daemon from which we consider that
it's acceptable to show notifications but we don't really plan to open
a dialog from there. Do you think it could/should be worked out?
- There's a daemon. We're kind of trying to get rid of daemons. When
is it needed?
- enum values are all-caps joined by underscore (e.g. GLOBAL_SETTING).
Camel-case would be more Qt-friendly.
- Some methods use wid as a windows id. Please use QWindow whenever
possible, it's being an issue in Plasma nowadays already.
- Maybe we can port the internal logging infrastructure to just use
QLoggingCategory?

Cheers! :)
Aleix