sitter requested changes to this revision.
sitter added a comment.
This revision now requires changes to proceed.
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.
suggestion:
bool validResult(int result)
{
if (result == 0) {
return true;
}
qCWarning(CATEGORYY) << ca_strerror(result));
return false;
}
INLINE COMMENTS
> CMakeLists.txt:77
>
> +find_package(Canberra)
> +set_package_properties(Canberra PROPERTIES DESCRIPTION "Library for
> generating event sounds"
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.
> notifybyaudio_canberra.cpp:112
> + ca_proplist_destroy(props);
> + props = nullptr;
> +}
Looks superfluous?
> notifybyaudio_canberra.cpp:118
> + Q_UNUSED(c);
> + QMetaObject::invokeMethod(static_cast<NotifyByAudio*>(userdata),
> + "finishCallback",
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.
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
