<table><tr><td style="">rjvbb added inline comments.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D14397">View Revision</a></tr></table><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D14397#inline-76589">View Inline</a><span style="color: #4b4d51; font-weight: bold;">sitter</span> wrote in <span style="color: #4b4d51; font-weight: bold;">CMakeLists.txt:77</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">I'd prefer if this was moved up, before finding Phonon, and then find phonon in the else branch of CANBERRA_FOUND.</p>

<p style="padding: 0; margin: 8px;">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.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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.</p>

<p style="padding: 0; margin: 8px;">Wouldn't the cleanest way to achieve that be the use of a <tt style="background: #ebebeb; font-size: 13px;">USE_CANBERRA</tt> CMake option that defaults to <tt style="background: #ebebeb; font-size: 13px;">ON</tt> on platforms where the dependency can be expected? Then you can make both backends hard dependencies.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D14397#inline-76595">View Inline</a><span style="color: #4b4d51; font-weight: bold;">sitter</span> wrote in <span style="color: #4b4d51; font-weight: bold;">notifybyaudio_canberra.cpp:118</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">This may benefit from an explicit <tt style="background: #ebebeb; font-size: 13px;">Qt::QueuedConnection</tt>. 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.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">That also corresponds to a Qt guideline.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R289 KNotifications</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D14397">https://phabricator.kde.org/D14397</a></div></div><br /><div><strong>To: </strong>broulik, Frameworks, dfaure, davidedmundson, sitter, drosca, kfunk, rjvbb<br /><strong>Cc: </strong>cfeck, alexeymin, ngraham, nicolasfella, kde-frameworks-devel, michaelh, bruns<br /></div>