D14397: Support libcanberra for audio notification

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=39289#toc REPOSITORY R289 KNotifications

D14397: Support libcanberra for audio notification

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

D14397: Support libcanberra for audio notification

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,

D14397: Support libcanberra for audio notification

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,

D14397: Support libcanberra for audio notification

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

D14397: Support libcanberra for audio notification

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

D14397: Support libcanberra for audio notification

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).

D14397: Support libcanberra for audio notification

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=38843 REVISION

D14397: Support libcanberra for audio notification

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=38842 REVISION DETAIL https://phabricator.kde.org/D14397 AFFECTED FILES CMakeLists.txt

D14397: Support libcanberra for audio notification

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=38841 REVISION DETAIL https://phabricator.kde.org/D14397 AFFECTED FILES CMakeLists.txt

D14397: Support libcanberra for audio notification

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

D14397: Support libcanberra for audio notification

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

D14397: Support libcanberra for audio notification

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

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,

D14397: Support libcanberra for audio notification

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,

D14397: Support libcanberra for audio notification

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

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,

D14397: Support libcanberra for audio notification

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

D14397: Support libcanberra for audio notification

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

D14397: Support libcanberra for audio notification

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

D14397: Support libcanberra for audio notification

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

D14397: Support libcanberra for audio notification

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,

D14397: Support libcanberra for audio notification

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

D14397: Support libcanberra for audio notification

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

D14397: Support libcanberra for audio notification

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

D14397: Support libcanberra for audio notification

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

D14397: Support libcanberra for audio notification

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

D14397: Support libcanberra for audio notification

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.

D14397: Support libcanberra for audio notification

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

D14397: Support libcanberra for audio notification

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