D14397: Support libcanberra for audio notification

[Harald Sitter]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20180731/08c268a2/attachment-0001.html>