D14397: Support libcanberra for audio notification

[René J.V. Bertin]

rjvbb added inline comments.

INLINE COMMENTS

> sitter wrote in CMakeLists.txt:77
> I'd prefer if this was moved up, before finding Phonon, and then find phonon in the else branch of CANBERRA_FOUND.
> 
> The rationale is that (e.g.) all of neon's tech which auto detects missing cmake dependencies can only do it's thing automatically if only actually missing dependencies are reported as such. With the current structure both Phonon and Canberra would always be in the cmake summary, even though Phonon being missing is irrelevant if canberra was found. OTOH if only phonon is found we'd still want to raise a stink because canberra is our preferred choice.

Preferred choice on Linux and other non-Mac Unix variants (or even only Linux because as mentioned earlier, libcanberra is only tested there). It can't be the preferred choice when no native backend is available, IMHO.

Wouldn't the cleanest way to achieve that be the use of a `USE_CANBERRA` CMake option that defaults to `ON` on platforms where the dependency can be expected? Then you can make both backends hard dependencies.

> sitter wrote in notifybyaudio_canberra.cpp:118
> This may benefit from an explicit `Qt::QueuedConnection`. I am not sure foreign-thread detection is 100% reliable with C call chains. Specifying it certainly is more explicit about the intent when reading the code though.

That also corresponds to a Qt guideline.

REPOSITORY
  R289 KNotifications

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

To: broulik, #frameworks, dfaure, davidedmundson, sitter, drosca, kfunk, rjvbb
Cc: cfeck, alexeymin, ngraham, nicolasfella, kde-frameworks-devel, michaelh, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20180731/b31d174c/attachment.html>