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
