<table><tr><td style="">sitter requested changes to this revision.<br />sitter added a comment.<br />This revision now requires changes to proceed.
</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><div><p>Code looks mostly ok. As discussed on IRC my main concern is that the current code ignores the return values of just about every ca_* method. This is a bit problematic from a diagnostics POV as we'll have no way to figure out why things went wrong if they go wrong. I'd like to at least have some logging added with a helper method. Bonus points obviously for not exploding if e.g. ca_context_create failed.</p>
<p>suggestion:</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">bool validResult(int result)
{
if (result == 0) {
return true;
}
qCWarning(CATEGORYY) << ca_strerror(result));
return false;
}</pre></div></div></div><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;">CMakeLists.txt:77</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">find_package(Canberra)
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">set_package_properties(Canberra PROPERTIES DESCRIPTION "Library for generating event sounds"
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><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><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-76594">View Inline</a><span style="color: #4b4d51; font-weight: bold;">notifybyaudio_canberra.cpp:112</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">ca_proplist_destroy</span><span class="p">(</span><span class="n">props</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">props</span> <span style="color: #aa2211">=</span> <span style="color: #aa4000">nullptr</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Looks superfluous?</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;">notifybyaudio_canberra.cpp:118</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">Q_UNUSED</span><span class="p">(</span><span class="n">c</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QMetaObject</span><span style="color: #aa2211">::</span><span class="n">invokeMethod</span><span class="p">(</span><span style="color: #aa4000">static_cast</span><span style="color: #aa2211"><</span><span class="n">NotifyByAudio</span><span style="color: #aa2211">*></span><span class="p">(</span><span class="n">userdata</span><span class="p">),</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #766510">"finishCallback"</span><span class="p">,</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><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></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>