D14397: Support libcanberra for audio notification

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R289:f03443cfb2b9: Support libcanberra for audio notification 
(authored by broulik).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D14397?vs=38843&id=39289#toc

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14397?vs=38843&id=39289

REVISION DETAIL
  https://phabricator.kde.org/D14397

AFFECTED FILES
  CMakeLists.txt
  cmake/modules/FindCanberra.cmake
  src/CMakeLists.txt
  src/knotificationmanager.cpp
  src/notifybyaudio.cpp
  src/notifybyaudio.h
  src/notifybyaudio_canberra.cpp
  src/notifybyaudio_canberra.h
  src/notifybyaudio_phonon.cpp
  src/notifybyaudio_phonon.h

To: broulik, #frameworks, dfaure, davidedmundson, sitter, drosca, kfunk, rjvbb
Cc: apol, cfeck, alexeymin, ngraham, nicolasfella, kde-frameworks-devel, 
michaelh, bruns

D14397: Support libcanberra for audio notification

[René J . V . Bertin]

rjvbb added a comment.


  That was already mentioned :)
  
  I see no problem with those components depending on something that is 
probably inevitable in a "linuxy desktop" environment.
  
  Also FYI: I built the latest canberra (PA support disabled) and gstreamer1 on 
Mac and am not getting any sound out of canberra-gtk-play (a memory error 
instead).
  Not that QtMultimedia is such an evident alternative (if QMediaPlayer has too 
much overhead): QAudioDecoder only works when you activate Qt's gstreamer 
support (and apply a simple patch so that the plugins are actually built) ...

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D14397

To: broulik, #frameworks, dfaure, davidedmundson, sitter, drosca, kfunk, rjvbb
Cc: apol, cfeck, alexeymin, ngraham, nicolasfella, kde-frameworks-devel, 
michaelh, bruns

D14397: Support libcanberra for audio notification

[David Faure]

dfaure added a comment.


  Just FYI, canberra is already an optional dependency of kmix and plasma-pa. 
https://lxr.kde.org/search?_filestring=CMakeLists.txt&_string=canberra

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D14397

To: broulik, #frameworks, dfaure, davidedmundson, sitter, drosca, kfunk, rjvbb
Cc: apol, cfeck, alexeymin, ngraham, nicolasfella, kde-frameworks-devel, 
michaelh, bruns

D14397: Support libcanberra for audio notification

[René J . V . Bertin]

rjvbb added a comment.


  > I really don't see what the big fuss is all about.
  
  Did you read Harald's remark (about raising a neon fuss) to which I replied?!

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D14397

To: broulik, #frameworks, dfaure, davidedmundson, sitter, drosca, kfunk, rjvbb
Cc: apol, cfeck, alexeymin, ngraham, nicolasfella, kde-frameworks-devel, 
michaelh, bruns

D14397: Support libcanberra for audio notification

[Aleix Pol Gonzalez]

apol added a comment.


  :( please @broulik

INLINE COMMENTS

> rjvbb wrote in CMakeLists.txt:77
> 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.

I'd say this is perfectly fine, already not optional. If you want to disable 
it, pass `-DCMAKE_DISABLE_FIND_PACKAGE_Canberra=OFF`.

I really don't see what the big fuss is all about.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D14397

To: broulik, #frameworks, dfaure, davidedmundson, sitter, drosca, kfunk, rjvbb
Cc: apol, cfeck, alexeymin, ngraham, nicolasfella, kde-frameworks-devel, 
michaelh, bruns

D14397: Support libcanberra for audio notification

[René J . V . Bertin]

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

D14397: Support libcanberra for audio notification

[Harald Sitter]

sitter accepted this revision.
sitter added a comment.
This revision is now accepted and ready to land.


  Well, I still think the result handling is more complicated than it needs to 
be, but whatever. LGTM for landing after 5.49 tagging (expected August 4th 
IIRC).
  
  https://cgit.kde.org/plasma-phone-components.git/plain/sounds/sitter/ohits.ogg

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

D14397: Support libcanberra for audio notification

[Kai Uwe Broulik]

broulik updated this revision to Diff 38843.
broulik added a comment.


  - Require Phonon so that a sound system is preferred (it is only searched for 
if Canberra isn't)

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14397?vs=38842&id=38843

REVISION DETAIL
  https://phabricator.kde.org/D14397

AFFECTED FILES
  CMakeLists.txt
  cmake/modules/FindCanberra.cmake
  src/CMakeLists.txt
  src/knotificationmanager.cpp
  src/notifybyaudio.cpp
  src/notifybyaudio.h
  src/notifybyaudio_canberra.cpp
  src/notifybyaudio_canberra.h
  src/notifybyaudio_phonon.cpp
  src/notifybyaudio_phonon.h

To: broulik, #frameworks, dfaure, davidedmundson, sitter, drosca, kfunk, rjvbb
Cc: cfeck, alexeymin, ngraham, nicolasfella, kde-frameworks-devel, michaelh, 
bruns

D14397: Support libcanberra for audio notification

[Kai Uwe Broulik]

broulik updated this revision to Diff 38842.
broulik added a comment.


  MOAAAR error handling

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14397?vs=38841&id=38842

REVISION DETAIL
  https://phabricator.kde.org/D14397

AFFECTED FILES
  CMakeLists.txt
  cmake/modules/FindCanberra.cmake
  src/CMakeLists.txt
  src/knotificationmanager.cpp
  src/notifybyaudio.cpp
  src/notifybyaudio.h
  src/notifybyaudio_canberra.cpp
  src/notifybyaudio_canberra.h
  src/notifybyaudio_phonon.cpp
  src/notifybyaudio_phonon.h

To: broulik, #frameworks, dfaure, davidedmundson, sitter, drosca, kfunk, rjvbb
Cc: cfeck, alexeymin, ngraham, nicolasfella, kde-frameworks-devel, michaelh, 
bruns

D14397: Support libcanberra for audio notification

[Kai Uwe Broulik]

broulik updated this revision to Diff 38841.
broulik added a comment.


  - More error handling

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14397?vs=38512&id=38841

REVISION DETAIL
  https://phabricator.kde.org/D14397

AFFECTED FILES
  CMakeLists.txt
  cmake/modules/FindCanberra.cmake
  src/CMakeLists.txt
  src/knotificationmanager.cpp
  src/notifybyaudio.cpp
  src/notifybyaudio.h
  src/notifybyaudio_canberra.cpp
  src/notifybyaudio_canberra.h
  src/notifybyaudio_phonon.cpp
  src/notifybyaudio_phonon.h

To: broulik, #frameworks, dfaure, davidedmundson, sitter, drosca, kfunk, rjvbb
Cc: cfeck, alexeymin, ngraham, nicolasfella, kde-frameworks-devel, michaelh, 
bruns

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(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

D14397: Support libcanberra for audio notification

[René J . V . Bertin]

rjvbb added a comment.


  I have no other objections than the ones above, as long as there's a fallback 
to NOT using libcanberra which can be enforced without having to patch the code 
(= via `CMAKE_DISABLE_FIND_PACKAGE_FOO` or a dedicated option).

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

D14397: Support libcanberra for audio notification

[Kai Uwe Broulik]

broulik reclaimed this revision.

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

D14397: Support libcanberra for audio notification

[Christoph Feck]

cfeck added a comment.


  +1 for supporting libcanberra.

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

D14397: Support libcanberra for audio notification

[Nathaniel Graham]

ngraham added a comment.


  +1 too; we can always work on upstream Qt improvements later.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D14397

To: broulik, #frameworks, dfaure, davidedmundson, sitter, drosca, kfunk, rjvbb
Cc: alexeymin, ngraham, nicolasfella, kde-frameworks-devel, michaelh, bruns

D14397: Support libcanberra for audio notification

[Nicolas Fella]

nicolasfella added a comment.


  +1 for Canberra

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D14397

To: broulik, #frameworks, dfaure, davidedmundson, sitter, drosca, kfunk, rjvbb
Cc: alexeymin, ngraham, nicolasfella, kde-frameworks-devel, michaelh, bruns

D14397: Support libcanberra for audio notification

[Kai Uwe Broulik]

broulik added a comment.


  So, what shall we do with this? Either we take it or leve it, I won't be the 
one changing it to QtMultimedia as canberra is *the* way to go.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D14397

To: broulik, #frameworks, dfaure, davidedmundson, sitter, drosca, kfunk, rjvbb
Cc: alexeymin, ngraham, nicolasfella, kde-frameworks-devel, michaelh, bruns

D14397: Support libcanberra for audio notification

[René J . V . Bertin]

rjvbb added a comment.


  >   I don't really care about your artificially created problems. You *have* 
libcanberra but want to use it for one project and not the other. Sorry, but 
no. 
  
  Oh, and do you have to show ignorance? This is a very common problem in 
software packaging, part of what allows your distribution to warn you something 
you're trying to uninstall is a dependency for something you might want to keep 
installed.
  
  As I said, yes, there is a CMake mechanism to disable specific `find_package` 
calls but it's rather confidential, ugly and useless with custom findfoo.cmake 
implementations that bypass find_package.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D14397

To: broulik, #frameworks, dfaure, davidedmundson, sitter, drosca, kfunk, rjvbb
Cc: alexeymin, ngraham, nicolasfella, kde-frameworks-devel, michaelh, bruns

D14397: Support libcanberra for audio notification

[René J . V . Bertin]

rjvbb added a comment.


  >   (Oh btw it's not like I didn't try, I originally ported all of this to 
QtMultimedia but `QAudioEffect` which is for low-latency sound effects only 
supports WAV sounds and `QMediaPlayer` had significant overhead and 
initialization times as well which is why I eventually ended up using canberra)
  
  See, it would have been constructive to mention that earlier.
  
  FWIW, I think we just want to play a file here, not juggle with audio effects.
  
https://stackoverflow.com/questions/41197576/how-to-play-mp3-file-using-qaudiooutput-and-qaudiodecoder.
  
  Anyone else interested in the idea of having a QtMultiMedia Phonon backend 
that (a priori) uses the most low-level APIs from that Qt component?

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D14397

To: broulik, #frameworks, dfaure, davidedmundson, sitter, drosca, kfunk, rjvbb
Cc: alexeymin, ngraham, nicolasfella, kde-frameworks-devel, michaelh, bruns

D14397: Support libcanberra for audio notification

[Kai Uwe Broulik]

broulik abandoned this revision.
broulik added a comment.


  > I haven't checked, but I'd appreciate if that could also be done through a 
cmake option. In packaging systems like MacPorts and HB it's perfectly possible 
to have libcanberra and/or pulseaudio installed for Gnome apps that cannot do 
without them, but still not want to let other software use them (opportunistic 
dependencies)
  
  I don't really care about your artificially created problems. You *have* 
libcanberra but want to use it for one project and not the other. Sorry, but 
no. How about I cannot do without them like your Gnome apps? Then what. (Can't 
you override those variables externally using `-D...` anyway?)
  
  But whatever, I'm out. Was an attempt to fixup the subpar experience we had 
with notification sounds and I'll happily continue to use use this patch 
locally but I'm tired of this.
  
  (Oh btw it's not like I didn't try, I originally ported all of this to 
QtMultimedia but `QAudioEffect` which is for low-latency sound effects only 
supports WAV sounds and `QMediaPlayer` had significant overhead and 
initialization times as well which is why I eventually ended up using canberra)

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D14397

To: broulik, #frameworks, dfaure, davidedmundson, sitter, drosca, kfunk, rjvbb
Cc: alexeymin, ngraham, nicolasfella, kde-frameworks-devel, michaelh, bruns

D14397: Support libcanberra for audio notification

[René J . V . Bertin]

rjvbb added a comment.


  [I didn't get to send this yesterday]
  
  >   Not using something because it's from a "rival GUI" is not a valid 
argument.
  
  It is IMVHO if "it" introduces a dependency on another GUI middleware. 
Libcanberra does that to the best of my knowledge.
  
  > Canberra over GStreamer works there as well.
  
  Only if you accept dependencies that shouldn't be required if you want to 
keep things as native as possible. The fact that a plasma project already uses 
libcanberra shouldn't be used as an argument for using it in a *framework*. 
Plasma is exclusive to non-Apple Unix (and in fact almost to Linux), and it's 
probably irrealistic to think that one could construct a completely GTk-free 
distribution on such systems (= the library will probably be there anyway).
  KNotifications is a Tier1 framework with a cross-platform vocation. For me 
that means that a solution with QtMultimedia should be preferred if anyway 
possible. Regardless what that component depends on itself... (Alexey beat me 
to it I see :))
  
  NB: I'm not talking about glib. That's a platform-agnostic support library 
that Qt uses even on Mac (I don't know about MSWin).
  
  > You could shove coreaudio or quicktime behind it if you wanted.
  
  *That* would be reinventing the wheel. Why would you want to do that in a 
Qt-based universe if Qt already has a component where this has been done?
  Quicktime is dead, btw.
  
  > That's not true.
  
  It is if you leave my remark in its (generic) context... And sound latencies 
can hardly be avoided with a 100% guarantee with asynchronous playback over an 
independent daemon process.
  
  > I restored the Phonon option for when canberra isn't available on the 
platform.
  
  I haven't checked, but I'd appreciate if that could also be done through a 
cmake option. In packaging systems like MacPorts and HB it's perfectly possible 
to have libcanberra and/or pulseaudio installed for Gnome apps that cannot do 
without them, but still not want to let other software use them (opportunistic 
dependencies). Alternatively, CMake has a DISABLE* backdoor for the basic 
`find_package()`, not very elegant but maybe it can do the trick?

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D14397

To: broulik, #frameworks, dfaure, davidedmundson, sitter, drosca, kfunk, rjvbb
Cc: alexeymin, ngraham, nicolasfella, kde-frameworks-devel, michaelh, bruns

D14397: Support libcanberra for audio notification

[Kai Uwe Broulik]

broulik added a comment.


  > Currently, libcanberra is tested on Linux only.
  
  I restored the Phonon option for when canberra isn't available on the 
platform.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D14397

To: broulik, #frameworks, dfaure, davidedmundson, sitter, drosca, kfunk, rjvbb
Cc: alexeymin, ngraham, nicolasfella, kde-frameworks-devel, michaelh, bruns

D14397: Support libcanberra for audio notification

[Alexey Min]

alexeymin added a comment.


  But `Currently, libcanberra is tested on Linux only.` 😢 Plasma-pa and 
plasma-whatever can use anything and be Linux-centric, KNotifications is a 
framework, isn't it?
  ++vote for QtMultimedia option...

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D14397

To: broulik, #frameworks, dfaure, davidedmundson, sitter, drosca, kfunk, rjvbb
Cc: alexeymin, ngraham, nicolasfella, kde-frameworks-devel, michaelh, bruns

D14397: Support libcanberra for audio notification

[Kai Uwe Broulik]

broulik added a comment.


  > phonon works just fine
  
  That's not true. If it worked fine, this patch would not exist, as I would 
not have apps freeze for seconds on teardown of Phonon objects, or latencies 
beyond all comparison

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D14397

To: broulik, #frameworks, dfaure, davidedmundson, sitter, drosca, kfunk, rjvbb
Cc: ngraham, nicolasfella, kde-frameworks-devel, michaelh, bruns

D14397: Support libcanberra for audio notification

[Harald Sitter]

sitter added a comment.


  In D14397#299073 , @rjvbb wrote:
  
  > >   That's reinventing the wheel to a point.
  >
  > Creating Qt after Gtk or vice-versa was also reinventing the wheel to a 
much larger extent - so is adding more and more OS features to Qt as has been 
going on for years.
  >
  > libcanberra comes from "the other" GUI universe. My arguments against 
depending on it stand, I think, including the one about not increasing and 
probably reducing the number of dependencies.
  
  
  Your arguments were against depending on gstreamer. Canberra is an 
abstraction. You could shove coreaudio or quicktime behind it if you wanted.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D14397

To: broulik, #frameworks, dfaure, davidedmundson, sitter, drosca, kfunk, rjvbb
Cc: ngraham, nicolasfella, kde-frameworks-devel, michaelh, bruns

D14397: Support libcanberra for audio notification

[Nicolas Fella]

nicolasfella added a comment.


  Not using something because it's from a "rival GUI" is not a valid argument. 
FWIW plasma-pa already uses Glib and Canberra. QtMultimedia uses GStreamer as 
backend on Unix, so it pulls in glib anyway. The only advantage, if any, of 
using QtMultimedia is that it uses native APIs on Mac/Windows, but Canberra 
over GStreamer works there as well.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D14397

To: broulik, #frameworks, dfaure, davidedmundson, sitter, drosca, kfunk, rjvbb
Cc: ngraham, nicolasfella, kde-frameworks-devel, michaelh, bruns

D14397: Support libcanberra for audio notification

[René J . V . Bertin]

rjvbb added a comment.


  >   That's reinventing the wheel to a point.
  
  Creating Qt after Gtk or vice-versa was also reinventing the wheel to a much 
larger extent - so is adding more and more OS features to Qt as has been going 
on for years.
  
  libcanberra comes from "the other" GUI universe. My arguments against 
depending on it stand, I think, including the one about not increasing and 
probably reducing the number of dependencies.
  
  Improving QtMultimedia with useful features (as far as that's even 
necessary!) may be reinventing the wheel but will have benefits for much more 
than just KNotifications (because, you know, *useful*).
  
  I'd love to look into using QAudioOutput (esp. if someone a bit more familiar 
with audio programming is willing to provide guidance), but after summer.  
(Thinking beyond the issue at hand: phonon works just fine, but its backends 
have been bothering me for a while and a QtMultiMedia backend could be an 
interesting idea to investigate as an easier cross-platform option.)

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D14397

To: broulik, #frameworks, dfaure, davidedmundson, sitter, drosca, kfunk, rjvbb
Cc: ngraham, nicolasfella, kde-frameworks-devel, michaelh, bruns

D14397: Support libcanberra for audio notification

[Harald Sitter]

sitter added a comment.


  In D14397#298557 , @ngraham wrote:
  
  > Impressive work, Kai! However I rather like the idea of improving 
QTMultiMedia's sound handling features rather than working around the lack of 
them by using a different library.
  
  
  That's reinventing the wheel to a point. There is a perfectly reasonable 
abstraction layer already. It's libcanberra. A library specifically designed 
for notification sounds.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D14397

To: broulik, #frameworks, dfaure, davidedmundson, sitter, drosca, kfunk, rjvbb
Cc: ngraham, nicolasfella, kde-frameworks-devel, michaelh, bruns

D14397: Support libcanberra for audio notification

[René J . V . Bertin]

rjvbb added a comment.


  > However I rather like the idea of improving QTMultiMedia's sound handling 
features rather than working around the lack of them by using a different 
library.
  
  Thanks Nathan for rewording my argument better than I managed.
  
  > Is there a CMake way to say "one of those two is required"?
  
  I'm not sure if this is still an actual open question but I think the usual 
way would be to make both optionan and then to check and raise an error if 
neither is found.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D14397

To: broulik, #frameworks, dfaure, davidedmundson, sitter, drosca, kfunk, rjvbb
Cc: ngraham, nicolasfella, kde-frameworks-devel, michaelh, bruns

D14397: Support libcanberra for audio notification

[Kai Uwe Broulik]

broulik updated this revision to Diff 38512.
broulik retitled this revision from "Port audio notification to libcanberra" to 
"Support libcanberra for audio notification".
broulik edited the test plan for this revision.
broulik added a comment.


  - Keep Phonon as fallback when libcanberra isn't provided

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14397?vs=38484&id=38512

REVISION DETAIL
  https://phabricator.kde.org/D14397

AFFECTED FILES
  CMakeLists.txt
  cmake/modules/FindCanberra.cmake
  src/CMakeLists.txt
  src/knotificationmanager.cpp
  src/notifybyaudio.cpp
  src/notifybyaudio.h
  src/notifybyaudio_canberra.cpp
  src/notifybyaudio_canberra.h
  src/notifybyaudio_phonon.cpp
  src/notifybyaudio_phonon.h

To: broulik, #frameworks, dfaure, davidedmundson, sitter, drosca, kfunk, rjvbb
Cc: ngraham, nicolasfella, kde-frameworks-devel, michaelh, bruns