A11y Sound Alert for new person and new chat message #2640

chigkim · 2022-05-22T20:41:40Z

Short description of changes
Added sound Alert for new person and new chat message

CHANGELOG: Added sound Alert for new person and new chat message

Context: Fixes an issue?
Screen reader users has no idea when someone joins or when receiving a new chat message. This plays a short sound alert for those events.

Does this change need documentation? What needs to be documented and how?
The sound alerts play through OS sound output. for this to work on Windows, your system sound output may need to be different than what Jamulus uses depending on the audio device/driver being used.
Please include the new GUI setting in the documentation.
The original sound files are from Freesound, and they were edited to fit the context in Jamulus.
new_message.wav: https://freesound.org/people/zrrion_the_insect/sounds/469014/
new_user.wav: https://freesound.org/people/deleted_user_4772965/sounds/256513/
The licenses are CC0.

Status of this Pull Request

What is missing until this pull request can be merged?

Checklist

I've verified that this Pull Request follows the general code principles
I tested my code and it does what I want
My code follows the style guide
I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
I've filled all the content above

chigkim · 2022-05-22T21:02:24Z

It compiled without a problem on both Windows and Mac machines, but it failed on autobuild on Github. Thoughts?

hoffie

It compiled without a problem on both Windows and Mac machines, but it failed on autobuild on Github. Thoughts?

Project ERROR: Unknown module(s) in QT: multimedia

We are only downloading the required modules in the build scripts, so those lists need to be adjusted. Those might be the proper places:

$ grep archive.*qtbase .github/ -rn
.github/autobuild/android.sh:73:            --archives qtbase qttools qttranslations qtandroidextras
.github/autobuild/ios.sh:23:        python3 -m aqt install-qt --outputdir "${QT_DIR}" mac ios "${QT_VERSION}" --archives qtbase qttools qttranslations
.github/autobuild/ios.sh:28:            python3 -m aqt install-qt --outputdir "${QT_DIR}" mac desktop "${QT_VERSION}" --archives qtbase
.github/autobuild/windows.ps1:45:        "--archives", "qtbase", "qttools", "qttranslations"
.github/autobuild/mac.sh:22:        python3 -m aqt install-qt --outputdir "${QT_DIR}" mac desktop "${QT_VERSION}" --archives qtbase qttools qttranslations

Minor general request: Your changelog suggestions sound good. Can you put them on the same line as the CHANGELOG: prefix in your PR descriptions? That way our script can pick the text up directly.

It seems like this PR introduces those sound effects unconditionally for all users. While I understand that some people might want or even need those, I think those need to have a way to be turned off (probably even by default in order to avoid changing the behavior for the existing user base?).

chigkim · 2022-05-22T22:41:01Z

1. Thanks! I changed the auto build scripts and pushed a new commit to my ranch with the PR. Do I need to make another PR? 2. I updated the CHANGELOG: line. 3. I agree. If someone could implement this in the setting to toggle the future might be a good idea.

chigkim · 2022-05-22T23:15:33Z

@hoffie I just added qtmultimedia on the suggested lines, but all the builds except iOS failed.
I'd appreciate any help.

pljones · 2022-05-23T07:41:23Z

src/clientdlg.cpp


 void CClientDlg::OnChatTextReceived ( QString strChatText )
 {
+    QSoundEffect* sf = new QSoundEffect();


How do I turn this off?

pljones · 2022-05-23T07:41:39Z

src/clientdlg.cpp


 void CClientDlg::OnNumClientsChanged ( int iNewNumClients )
 {
+    if ( iNewNumClients > iClients )


How do I turn this off?

(It should be off by default.)

chigkim · 2022-05-23T10:55:47Z

Is there someone who could help with implementing a toggle in the setting dialog? I'm blind, and unfortunately qt designer is not accessible.
Thanks!

ann0see · 2022-05-24T18:51:39Z

@henkdegroot @mulyaj I think you worked with the GUI?

henkdegroot · 2022-05-24T19:53:02Z

a toggle in the setting dialog?

@chigkim, perhaps I can help out with the setting. Are you sure a setting in a GUI dialog is the best/most convenient option?
Thinking that perhaps a command line option might be easier to use. What do you think?
If it should be in the GUI, what would be the best place for it? Perhaps the My Profile tab? Which contains settings to control the user interface.

henkdegroot · 2022-05-24T19:54:58Z

@hoffie I just added qtmultimedia on the suggested lines, but all the builds except iOS failed.

@hoffie, could it be that the build fails due to the fact it is using cached versions (i.e. cached version without qtmultimedia)?

hoffie · 2022-05-24T21:04:22Z

@hoffie, could it be that the build fails due to the fact it is using cached versions (i.e. cached version without qtmultimedia)?

Umm, my first thought was "this should be covered by the hash key calculation", but the log and the code say otherwise. The code still refers to the old, no longer existing paths. Seems like I've missed that in autobuild refactoring cleanups.
@chigkim Sorry about that, will submit a PR shortly.

The Linux build failures are not related to that part though (no caching there). In that case I suspect that we need to install qtmultimedia explicitly as well (didn't expect that). It's slightly different there as we use the official Debian packages and no aqt.
@chigkim I suggest trying to add qtmultimedia5-dev to sudo apt-get -qq --no-install-recommends -y install devscripts build-essential debhelper fakeroot libjack-jackd2-dev qtbase5-dev qttools5-dev-tools in .github/autobuild/linux_deb.sh.

henkdegroot · 2022-05-24T21:10:23Z

@chigkim, I have cloned your branch and locally compiled without issues. However I don't hear any sound when a chat message arrives or when a new client joins the session. Any thoughts on this?

hoffie · 2022-05-24T21:12:37Z

@chigkim, I have cloned your branch and locally compiled without issues. However I don't hear any sound when a chat message arrives or when a new client joins the session. Any thoughts on this?

I suspect that by using Qt's audio code a totally different routing can apply. In other words, Jamulus may output to ASIO or Jack, but Qt will likely use the OS default instead, which might be a different device.
Edit: That might even be desirable? I guess other screen reader output may go there as well?

Another update regarding the build failures: Android seems to have a non-cache related cause as well (Manifest/permissions). Although the mssages seem logical, I can't really tell why they appear right now. Seems related to #2403.

pljones · 2022-05-24T21:13:13Z

a toggle in the setting dialog?

@chigkim, perhaps I can help out with the setting. Are you sure a setting in a GUI dialog is the best/most convenient option? Thinking that perhaps a command line option might be easier to use. What do you think? If it should be in the GUI, what would be the best place for it? Perhaps the My Profile tab? Which contains settings to control the user interface.

CClient (in this case) should own the setting
CClient should have a socket to change the value of the setting
CClient should have a signal for when the value of the setting is changed
The setting should get read from and written to the inifile
Both the Client RPC code and Client GUI code should expose the setting and respond to updates (i.e. both could be active)
Any command line option given should overwrite a value from the inifile (but then get written back to the inifile)

The above is how all Client (or Server) settings should work.

hoffie · 2022-05-24T21:32:36Z

I've filed #2642 for the cache key fix and I've started a CI run for this PR (#2642) plus the fix in my fork. That should hopefully show that the qtmultimedia-based failure is gone on Mac/Windows.

Edit: Looks good at first glance -- thank you @henkdegroot for the heads up!

chigkim · 2022-05-24T21:38:33Z

@henkdegroot, thanks for your help! I think toggle in the setting dialog would be a better option than command line.
Hmm that's interesting. This feature has been in my accessible build for a long time, and it was one of the favorite features for screen reader users. I know it worked for other people. Only thing I could think of is that I changed from QSound to QSoundEffect for qt6 compatibility recently.
I assume git can handle binary wav files right?

hoffie · 2022-05-24T21:44:52Z

I assume git can handle binary wav files right?

It should. The files in your PR play properly using mplayer for me.

I'm wondering whether it is possible and maybe even better to leave such possible notifications to the screen reader and just try to instruct the screen reader accordingly? Maybe QAccessibleEvent is of any help?
That might be more standardized for screen reader users and might keep the Jamulus changes simpler (no need for a setting and no need for qtmultimedia, for example).

chigkim · 2022-05-24T21:57:56Z

@hoffie, as far as I know unfortunately QT doesn't have a way to send a generic announcement with custom message to screen readers other than sending new value changed event for a specific UI element.
Also, would non-screen reader users find this useful/interesting at all? Maybe while playing and looking at chart, you can find out when someone sends you a message?

henkdegroot · 2022-05-26T18:48:43Z

@chigkim, still can't seem to get the sounds alerts working. I have also tried updating the code to use QSound. I am using a windows 10 device. I am testing my local build which I create using the powershell script provided (deploy_windows.ps1) and it uses Qt 5.15.2.

chigkim · 2022-05-27T12:53:53Z

@henkdegroot, have you checked your Windows default sound card? As hoffie mentioned, the sound alert should play through whatever your Windows default sound output is set to, not necessarily what Jamulus uses.

chigkim · 2022-05-27T14:16:59Z

@henkdegroot, also, you could try my build. https://www.dropbox.com/s/1cevtv50e9077o9/Jamulus-a11y-sound-alert.zip?dl=0

henkdegroot · 2022-05-27T16:30:53Z

@chigkim I will give your build a try and will also test with another windows laptop. I do get audio alerts from other programs so that is not the issue. I thought that perhaps the ::fromfile was trying to load a local file instead of the file from the resources. Hope to have some time in the weekend for this.

henkdegroot · 2022-05-28T06:42:14Z

@chigkim, tested your build and tested with another windows laptop. Both cases I do not hear any notifications on new client and chat messages.

chigkim · 2022-05-29T11:48:50Z

I asked some people to test the same build a couple of days ago, and they said it all worked fine for them. Are you using an external audio interface with Asio driver, or built-in sound card with asio4all?

henkdegroot · 2022-05-30T05:55:16Z

Are you using an external audio interface with Asio driver, or built-in sound card with asio4all?

I am using a built-on sound card with asio4all. I even tested with all other sound drivers disabled, and not difference. Not hearing sounds when a New Client joins or when a Chat Message is received. So not sure why I don't get these sound notifications.

ann0see · 2022-06-05T12:11:10Z

@chigkim do you think this will make it up in the next few days to be able to get included into the next release? If not, I think it's worth to de-tag.

henkdegroot · 2022-06-05T13:25:52Z

Just adding my view, as much I would like to assist creating a gui setting to be able to turn this on/off, I can only do so when I am able to get it working in the first place. If it fails for me, it probably fails for more people and we end up getting issues reported in that. I don't believe this not working is caused by my setup but don't know what is causing this.
@ann0see can you test this feature on a windows system using the link provided by @chigkim?

pljones · 2022-06-30T19:27:35Z

Sorry to be annoying... Can the build script changes be split out, at least into a separate (initial) commit?

What I look for is that each commit builds on its own, with no dependency on later commits. Now that may be the case here but

I thought the build script changes were needed first to get all the platforms working
Being able to have the sound off needs to be available at the point the commit that turns the sounds on happens

hoffie · 2022-06-30T20:14:47Z

I thought the build script changes were needed first to get all the platforms working

The build scripts are only touched to add the newly required dependency. Yes, this has to be done first or at the same time as introducing the code which uses it. Not sure if you saw my question in the chat: I could split that part out into its own commit, but it wouldn't be meaningful on its own. We wouldn't add the qtmultimedia dependency if we weren't to add code which uses it. Therefore, I made that part a single commit. Let me know what you think.

Being able to have the sound off needs to be available at the point the commit that turns the sounds on happens

Usability-wise I agree (i.e. it should be part of one PR and this is the case here). Technically, both states are buildable/usable on its own. If that's not acceptable, I would have to squash the GUI setting addition into the other commit as well. Do you want me to do that?

(Not sure when you've started reviewing -- your comment was posted only a few minutes after my rebasing/squash push; no code has changed, only git history did).

pljones · 2022-07-01T18:38:39Z

Yes, I was checking the individual commits for sanity as a squash/fixup rebase had been done. Based on what you've said, I think a single commit would make most sense.

pljones · 2022-07-01T18:58:40Z

If I install this new version, does it require access to hardware that I might have dedicated to Jamulus and thus cause Jamulus to fail to load?

Say I have a single audio system with a sound drive that only allows exclusive access to the sound card and that's what I expect Jamulus to use. Does simply having this feature cause a problem under those circumstances (even when it's not switched on)?

One other fundmental issue I have with the approach is that it's adding non-visual code to ClientDlg. That shouldn't happen. Why would I not want the audio alerts if I were using the JSON RPC client without the graphic UI?

pljones

With comments as per conversation / chat.

ann0see · 2022-07-01T19:11:35Z

Say I have a single audio system with a sound drive that only allows exclusive access to the sound card and that's what I expect Jamulus to use. Does simply having this feature cause a problem under those circumstances (even when it's not switched on)?

That's exactly an/the issue which needs to be avoided.

android/AndroidManifest.xml

src/clientdlg.cpp

ann0see · 2022-07-01T19:22:53Z

src/clientsettingsdlgbase.ui

+                  <item>
+                   <widget class="QLabel" name="lblAudioAlerts">
+                    <property name="text">
+                     <string>Audio Alerts</string>


--> Audio Alerts (might need secondary sound hardware)

Hrm, not sure if there's space for so much text. Have you checked?

Maybe we could add a tooltip later - and something clear in the documentation?

The What's this text does have a hint regarding that already, btw.

hoffie · 2022-07-01T21:14:59Z

Yes, I was checking the individual commits for sanity as a squash/fixup rebase had been done. Based on what you've said, I think a single commit would make most sense.

Ok. Squashed to a single commit now (with author information preserved as Co-authored-by).

If I install this new version, does it require access to hardware that I might have dedicated to Jamulus and thus cause Jamulus to fail to load?
Say I have a single audio system with a sound drive that only allows exclusive access to the sound card and that's what I expect Jamulus to use. Does simply having this feature cause a problem under those circumstances (even when it's not switched on)?

Based on my understanding, I would not expect such issues. Solely linking to qtmultimedia should not cause any audio devices to be considered "in use". All playback happens on demand, which means it happens after initializing Jamulus' main audio stack. So if exclusive access is a problem, I'd assume that the new feature breaks and not Jamulus' core functionality.

As far as I understand, this feature was part of @chigkim's a11y build which had already been in use by some people?

And as a last "Let's try it" argument: We are preparing a new minor release, which is not supposed to cause breakage, but is known to carry larger changes. We still have some time during betas to find possible issues.

One other fundmental issue I have with the approach is that it's adding non-visual code to ClientDlg. That shouldn't happen. Why would I not want the audio alerts if I were using the JSON RPC client without the graphic UI?

Understandable, especially regarding future plans. I'll open that as a separate issue.

mirabilos · 2022-07-01T21:39:26Z

Christian Hoffmann dixit:

Based on my understanding, I would not expect such issues. Solely linking to qtmultimedia should not cause any audio devices to be considered "in use". All playback happens on demand, which means it happens after initializing Jamulus' main audio stack. So if exclusive access is a problem, I'd assume that the new feature breaks and not Jamulus' core functionality.

AIUI / in my experience (pure ALSA-based system, no Poettering stuff) it is so that you have to start jackd (usually via qjackctl) before starting Jamulus, and that fails if anything uses the sound device, and afterwards any audio has to go via JACK anyway, so if anything is playing once jackd is started, it either goes through JACK (and thus is multiplexed) or is not audible (because jackd has exclusive access to the soundcard) or goes through a different soundcard which jackd does not use. bye, //mirabilos -- 21:12⎜<Vutral> sogar bei opensolaris haben die von der community so ziemlich jeden mist eingebaut │ man sollte unices nich so machen das desktopuser zuviel intresse kriegen │ das macht die code base kaputt 21:13⎜<Vutral:#MirBSD> linux war früher auch mal besser :D

src/clientdlg.cpp

Plays sound when someone joins/leaves. Plays sound when receiving a new message. Uses QtMultimedia for playback. Sounds are from Freesound (licensed CC0): - new_message.wav from https://freesound.org/people/zrrion_the_insect/sounds/469014/ - new_user.wav from https://freesound.org/people/deleted_user_4772965/sounds/256513/ Co-authored-by: henkdegroot <13550012+henkdegroot@users.noreply.github.com> Co-authored-by: Christian Hoffmann <mail@hoffmann-christian.info> Co-authored-by: Peter L Jones <pljones@users.noreply.github.com>

ann0see · 2022-07-03T13:12:15Z

@hoffie this PR created a wrong CHANGELOG entry.

pljones · 2022-07-04T06:15:54Z

Also, we need to get COMPILING.md updated to mention that qtmultimedia5-dev needs installing as a build time dependency (at least on Ubuntu 20 LTS).

henkdegroot · 2022-07-04T18:55:57Z

Also, we need to get COMPILING.md updated to mention that qtmultimedia5-dev needs installing

I believe I did this already as part of this PR....can you check please?

pljones · 2022-07-04T19:51:32Z

Ah sorry, I meant to come back and say it was user error on my part. I've noted elsewhere it needs highlighting as part of the release.

gilgongo · 2022-07-10T12:21:21Z

Screenshot for website. Ignore.

chigkim · 2022-07-17T13:46:46Z

r3_9_0beta2 didn't pick up:
CHANGELOG: Added sound Alert for new person and new chat message.
How should I format the line to be included?

ann0see · 2022-07-17T14:09:46Z

I think @pljones forgot updating the changelog for this beta

chigkim · 2022-10-11T09:17:09Z

Ah ok. Last time I checked, asio4all locks up and prevent other apps to play sound through the card it uses. I suspect asio4all is blocking QT to play sound with QSoundEffect. If I use asio4all with built-in sound card, it even blocks screen reader. That's why screen reader users would use an external audio interface with its own Asio driver for music apps like Jamulus and use built-in sound card for OS system output and screen readers. Do you have an external audio interface with Asio driver that you could test with? Skip asio4all and use that audio interface with Jamulus. Then set your built-in soundcard as your OS system output. Then hopefully you'll hear it.

chigkim force-pushed the a11y-sound-alert branch from 933a8fa to 4f9cffb Compare May 22, 2022 20:45

hoffie reviewed May 22, 2022

View reviewed changes

hoffie added this to the Release 3.9.0 milestone May 22, 2022

pljones reviewed May 23, 2022

View reviewed changes

hoffie mentioned this pull request May 24, 2022

Autobuild: Fix Mac/Windows cache key #2642

Merged

5 tasks

hoffie requested a review from ann0see June 30, 2022 19:15

pljones approved these changes Jul 1, 2022

View reviewed changes

ann0see reviewed Jul 1, 2022

View reviewed changes

hoffie force-pushed the a11y-sound-alert branch from e395c9d to 79c04cd Compare July 1, 2022 21:13

hoffie mentioned this pull request Jul 1, 2022

Audio alerts: Consider JSON-RPC use case #2686

Open

pljones reviewed Jul 1, 2022

View reviewed changes

src/clientdlg.cpp Show resolved Hide resolved

pljones reviewed Jul 2, 2022

View reviewed changes

src/clientdlg.cpp Show resolved Hide resolved

hoffie force-pushed the a11y-sound-alert branch from 79c04cd to feea2bf Compare July 2, 2022 21:50

pljones merged commit e7bad01 into jamulussoftware:master Jul 3, 2022

gilgongo mentioned this pull request Jul 10, 2022

Screenshots and updates jamulussoftware/jamuluswebsite#772

Merged

gilgongo removed the needs documentation PRs requiring documentation changes or additions label Jul 10, 2022

gilgongo mentioned this pull request Jul 13, 2022

Event/Growl notifications on client desktop machines #425

Open

chigkim deleted the a11y-sound-alert branch July 31, 2022 04:29

hoffie mentioned this pull request Aug 20, 2022

Android: Duplicate permissions in Manifest #2403

Open

A11y Sound Alert for new person and new chat message #2640

A11y Sound Alert for new person and new chat message #2640

Uh oh!

Conversation

chigkim commented May 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

chigkim commented May 22, 2022

Uh oh!

hoffie left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chigkim commented May 22, 2022 via email

Uh oh!

chigkim commented May 22, 2022

Uh oh!

pljones May 23, 2022

Choose a reason for hiding this comment

Uh oh!

pljones May 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chigkim commented May 23, 2022

Uh oh!

ann0see commented May 24, 2022

Uh oh!

henkdegroot commented May 24, 2022

Uh oh!

henkdegroot commented May 24, 2022

Uh oh!

hoffie commented May 24, 2022

Uh oh!

henkdegroot commented May 24, 2022

Uh oh!

hoffie commented May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pljones commented May 24, 2022

Uh oh!

hoffie commented May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chigkim commented May 24, 2022

Uh oh!

hoffie commented May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chigkim commented May 24, 2022

Uh oh!

henkdegroot commented May 26, 2022

Uh oh!

chigkim commented May 27, 2022

Uh oh!

chigkim commented May 27, 2022

Uh oh!

henkdegroot commented May 27, 2022

Uh oh!

henkdegroot commented May 28, 2022

Uh oh!

chigkim commented May 29, 2022 via email

Uh oh!

henkdegroot commented May 30, 2022

Uh oh!

ann0see commented Jun 5, 2022

Uh oh!

henkdegroot commented Jun 5, 2022

Uh oh!

pljones commented Jun 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hoffie commented Jun 30, 2022

Uh oh!

pljones commented Jul 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pljones commented Jul 1, 2022

Uh oh!

chigkim commented May 22, 2022 •

edited

Loading

hoffie left a comment •

edited

Loading

pljones May 23, 2022 •

edited

Loading

hoffie commented May 24, 2022 •

edited

Loading

hoffie commented May 24, 2022 •

edited

Loading

hoffie commented May 24, 2022 •

edited

Loading

pljones commented Jun 30, 2022 •

edited

Loading

pljones commented Jul 1, 2022 •

edited

Loading