Don't imply that multiple drivers have errors #2168

ann0see · 2021-12-10T20:32:53Z

Short description of changes

If there's only one driver with an error message, there's no need to imply that multiple devices have errors. This change is done to simplify the error message.

The following is not exactly what my (fixed) code looks like.

Context: Fixes an issue?

Idea from #2163 (reply in thread)
Does this change need documentation? What needs to be documented and how?
No

Status of this Pull Request

Draft. It remains to test what happens if lNumDevs > 1.
What is missing until this pull request can be merged?
Untested since it's just a quick fix. Will test it by checking the artifacts.

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

gilgongo · 2021-12-11T14:50:27Z

Do you think we can remove the ambiguity in the first part of the message? The driver(s) in question my in fact be perfectly usable, it's just that there is a problem with them that may or may not be temporary.

So if only one driver is erroring, just present this error message with an option to open the settings:

If multiple drivers are erroring, try to let the errors speak for themselves:

In general, long dialogue messages tend not to get read though. I find that had to accept, and don't really know why it is, but it's a very observable and common thing. So the other approach might be not to show the errors themselves and instead just have a call to action to open the settings? Depends on how helpful the messages are I guess:

pljones · 2021-12-11T15:40:02Z

I like the first two. In the second, I'd add "If needed" at the start of the guidance - as it lists where the errors were, it may be that nothing needs be done. If the pop up does need attention - i.e. no audio device is available - that needs to be clear,

I don't like the third one because it's far too vague to decide what to do. "Maybe something is wrong with one of your sound devices but I'm not going to tell you which one or what the problem was." Thanks....

And is it "sound" or "audio"?

gilgongo · 2021-12-11T15:43:27Z

@pljones Ah yes the last one doesn't indicate which card has the fail. OK that's no good then :-)

BTW why "if needed"? Seems redundant. Or implies you may not want to fix the fact that Jamulus doesn't work for you?

~~I can never remember what we said about sound vs audio. I think it's audio as that's maybe more specific to computer things.~~

We now have https://jamulus.io/contribute/Style-and-Tone ! Which says... "Sound hardware" I think.

pljones · 2021-12-11T16:58:55Z

BTW why "if needed"? Seems redundant. Or implies you may not want to fix the fact that Jamulus doesn't work for you?

Because you list drivers that failed. If I'm not using those drivers, why would I want to correct them? The

You may be able to correct these in the driver settings

makes it sound like it should be looked at and may be fixable. It's better to make it clear the user needs to decide what steps to take: we're not telling them to fix it - just that something went wrong that could need fixing.

You could add "if applicable", if you prefer :).

Oh yes, meant to add: if you want a shorter pop-up, add a "Details..." to pop up another dialog with the list of cards and errors (if there's more than one).

And finally... The first line and last line usually get read -- to answer "What is this?" (first line) and "What do I need to do about it?" (last line). The bit in between is noise... (Hence the "Details..." button when needed.)

gilgongo · 2021-12-11T17:15:00Z

Oh I see - in "plural" errors case, "If needed... " would be clearer there but it still seems odd in the "singular" error case to me.

Agree about the first and last line prominence. Generally though, popups that spawn popups are seen as a bad thing as you can lose track or the parent or child if you accidentally click on something (and I think it's a bit of a mare for those using assistive tech). Also don't forget the ASIO settings is yet another "popop" in this context. But I guess if there's more than about three error messages it might be OK (the best solution would be an expandable panel within the dialogue but I assume that's not going to be possible).

makes it sound like it should be looked at and may be fixable.

I disagree. The sense is that there may or may not be issues to fix, and our lack of specificity clues this as well. Any other wording will be unacceptably long and complicated and we'll be back where we started.

src/soundbase.cpp

pljones · 2021-12-16T21:39:24Z

src/soundbase.cpp

@@ -166,30 +166,38 @@ QString CSoundBase::SetDev ( const QString strDevName )
        if ( !vsErrorList.isEmpty() )
        {
            // create error message with all details
-            QString sErrorMessage = "<b>" + QString ( tr ( "No usable %1 audio device found." ) ).arg ( strSystemDriverTechniqueName ) + "</b><br>";
+            QString sErrorMessage = "<b>";


Just declare it here - the floating "" makes the code hard to follow.

Ok. Still struggeling with the syntax though.

pljones · 2021-12-16T21:47:10Z

src/soundbase.cpp

            }
            else
            {
-                 sErrorMessage += "<b>" + GetDeviceName ( 0 ) + "</b>: " + vsErrorList[0] ;
+                sErrorMessage += tr ( "Couldn't initialize audio driver (using %1)</b><br>"


Line 173:

"Sound driver errors (using %1) found: "

Line 184:

"Couldn't initialize audio driver (using %1) "

Use the same string in both places. Ideally the less technical one. The first doesn't need to end with ":" as it no longer introduces a list.

Maybe

"Could not find a sound driver (using %1) that could be used. "

(and do away entirely with "Your sound device is not compatible in its current state." as it would then be somewhat redundant).

And then the if checking the number of errors (more than 1 or not) is irrelevant, too.

pljones · 2021-12-16T21:48:10Z

src/soundbase.cpp

 #ifdef _WIN32
            // to be able to access the ASIO driver setup for changing, e.g., the sample rate, we
            // offer the user under Windows that we open the driver setups of all registered
            // ASIO drivers
-            sErrorMessage += "<br/>" + tr ( "You might be able to fix this error by changing settings of your ASIO driver. Do you want to open these settings now?" );
+            sErrorMessage += "<br/>" + tr ( "You may be able to fix errors in the driver settings." );


This seems worthless - i.e. adding extra words to an already lengthy message that don't help the user. They're offered the option to Open ASIO settings if they want, which should be enough. Maybe add "What's this" help to it.

And has inconsistent use of   vs   elsewhere.

pljones · 2021-12-16T21:51:40Z

src/soundbase.cpp

@@ -166,30 +166,38 @@ QString CSoundBase::SetDev ( const QString strDevName )
        if ( !vsErrorList.isEmpty() )


Lines 154 to 165 are irrelevant if this we enter this if. They should be placed after the block.

Might the problem be that this code is run once at startup and also during runtime if someone changes the sound devices?

Before line 197 was introduced, the variable set there wasn't referenced until after this if block. However, now line 197 is there, it makes sense.

pljones · 2021-12-16T22:02:17Z

src/soundbase.cpp


-            if ( QMessageBox::Yes == QMessageBox::information ( nullptr, APP_NAME, sErrorMessage, QMessageBox::Yes | QMessageBox::No ) )
+            if ( mbWinErrorBox.clickedButton() == btnASIOSettings )
            {
                LoadAndInitializeFirstValidDriver ( true );


vsErrorList = LoadAndInitializeFirstValidDriver ( true ); and then check the list size. If it's zero (and I've not checked the code to make sure I'm right here...), return strReturn rather than continuing.

I think there's more work needed then just that. See the commit I mentioned: Probably it's in a deeper level.

ann0see · 2021-12-19T21:17:10Z

Ok. Maybe something like that would be ok:

ann0see · 2021-12-19T21:19:32Z

src/soundbase.cpp

+            if ( vsErrorList.isEmpty() ) {
+                return strReturn;
            }


As mentioned before, I think this is not enough.

It looks like it's what's needed and removes my concern at https://github.com/jamulussoftware/jamulus/pull/2168/files#r770944946

ann0see · 2021-12-19T21:20:22Z

src/soundbase.cpp

+            QMessageBox qmASIOWarningBox;
+            QPushButton *btnASIOSettings = qmASIOWarningBox.addButton ( tr ( "Open ASIO settings" ), QMessageBox::AcceptRole );
+
+            qmASIOWarningBox.addButton ( QMessageBox::Cancel );
+            qmASIOWarningBox.setText ( sErrorMessage );
+            qmASIOWarningBox.setIcon ( QMessageBox::Warning );
+            qmASIOWarningBox.exec();


Probably this can be simplified somehow

I think there's a constructor that allows most of those things to be set?

Yes, I think/thought so too. However I couldn't find it in the documentation (or the code gave an error). The problem was with adding a "custom" button.

gilgongo · 2021-12-20T07:14:40Z

src/soundbase.cpp

@@ -166,24 +166,35 @@ QString CSoundBase::SetDev ( const QString strDevName )
        if ( !vsErrorList.isEmpty() )
        {
            // create error message with all details
-            QString sErrorMessage = "<b>" + QString ( tr ( "No usable %1 audio device found." ) ).arg ( strSystemDriverTechniqueName ) +
-                                    "</b><br><br>" + tr ( "These are all the available drivers with error messages:" ) + "<ul>";
+            QString sErrorMessage = tr ( "<b>Could not find compatible sound driver (using %1).</b><br><br>" ).arg ( strSystemDriverTechniqueName );


The "... (using ASIO)." seems a bit confusing here as it implies a driver might be found using something else. Is there a reason we're saying this?

It won't necessarily say "ASIO" - it provides meaningful information for diagnosing a problem preventing the use of the program.

OK, but we have a button that says "Open ASIO settings", so is that button conditional on what %1 is? This PR is to help people who aren't technical deal with the problem. If we qualify an error message in this way, they'd be excused for wondering why it's qualified, should they be using something else, etc.

No, see line 203 (new). The button only appear on Windows but this string is always used. (The button should also only appear for ASIO -- that needs fixing.)

The whole flow is really bad, I think - huge block of platform-specific code at level that should be platform-independent.

I still think "Could not find compatible sound driver (using ASIO)" feels a bit odd. Like it's expecting them to switch to something that isn't ASIO where the problem might not arise.

It would be less mysterious to just make it the subject in that case and say, "Could not find a compatible ASIO sound driver." That is, unless we want also to say "Maybe you should try using [other system]." which I doubt.

Maybe go back to No usable %1 audio device found. then?

That reads better than a parenthesised suffix (I think).

BTW re. "audio device", it's probably the subject of a different ticket, but for such an important thing we should keep to a standard term. The style guide says we should be using "sound hardware" here but "audio device" does feel a bit more conventional somehow.

Ok. Fixed that.

The whole flow is really bad, I think - huge block of platform-specific code at level that should be platform-independent.

I'm a bit unsure, but I remember there was a discussion about fixing this. Wasn't @npostavs involved?

I guess pljones' remark at #1378 (comment) is similar.

pljones · 2021-12-21T22:23:11Z

src/soundbase.cpp

            }
-            sErrorMessage += "</ul>";

 #ifdef _WIN32


Is this defined if we're running JACK on Windows? If so, it's the wrong symbol.

I think so. Similar to the ASIO Device setting button.

OK, probably needs a bug raised to fix it, then.

Ok. Fixed this now.

pljones · 2021-12-22T10:14:31Z

Before approval, this PR should show behavour when:

results for all platforms (iOS, Android, MacOS, Linux, Window JACK and Windows ASIO)
results when the final throw CGenErr ( sErrorMessage ); occurs
results when the final return strReturn; occurs
okay to say "platform tested but could not achieve failure", though, I suppose...

This is needed to ensure it remains sane...

ann0see · 2021-12-25T09:38:07Z

I think it still throws an exception without returning. We‘d need some more debugging, but that would be out of scope of this PR.

ann0see · 2022-02-02T21:02:12Z

No longer sure if this is moving into the right direction. Probably the exception handling needs to be part of another PR.

ann0see · 2022-02-16T21:03:24Z

src/soundbase.cpp

+
+            if ( qmASIOWarningBox.clickedButton() == btnASIOSettings )
+            {
+                vsErrorList = LoadAndInitializeFirstValidDriver ( true );


I think we should revert this to the original state and do functional changes here later.
Related to #2238

Done. This should be handled in another PR.

ann0see · 2022-02-26T21:56:30Z

src/soundbase.cpp

+                // TODO: This or some related code aborts the app even if the driver is valid after user changes.
+                // This should be handled differently. For reference, please see https://github.com/jamulussoftware/jamulus/pull/2168


@pljones I wrote a TODO comment here.

ann0see

Rebased this PR.

pljones · 2022-02-27T09:43:56Z

Overall, far too much of the code appears to be trying to handle Windows-ASIO specific issues in src/soundbase.cpp, rather than windows/sound.cpp. I'd rather any improvements were focussed on removing ASIO code from src/*.

If there's only one driver with an error message, there's no need to imply that multiple devices have errors. This change is done to simplify the error message.

ann0see

I think the text part should be split out into another PR (and the addition of the open ASIO Settings button overthought again)

Split out from jamulussoftware#2168

ann0see · 2022-03-12T23:29:04Z

Closing in favour of #2496 we should probably split this PR.

Split out from jamulussoftware#2168

ann0see commented Dec 16, 2021

View reviewed changes

src/soundbase.cpp Outdated Show resolved Hide resolved

pljones reviewed Dec 16, 2021

View reviewed changes

pljones referenced this pull request Dec 16, 2021

try to fix Misconfigured ASIO4ALL config can't be corrected #117

00b11ee

ann0see commented Dec 19, 2021

View reviewed changes

gilgongo reviewed Dec 20, 2021

View reviewed changes

pljones reviewed Dec 21, 2021

View reviewed changes

ann0see mentioned this pull request Dec 25, 2021

Do not show ASIO Device settings and open ASIO settings on JACK version of Jamulus #2186

Closed

ann0see mentioned this pull request Jan 13, 2022

Windows: Unable to configure ASIO device (major bug) #2238

Open

ann0see marked this pull request as ready for review February 16, 2022 20:59

ann0see commented Feb 16, 2022

View reviewed changes

ann0see force-pushed the feature/smaller-asio-error-msg branch 2 times, most recently from f9bff4f to a42fd50 Compare February 26, 2022 21:45

ann0see closed this Feb 26, 2022

ann0see force-pushed the feature/smaller-asio-error-msg branch from a42fd50 to b37732e Compare February 26, 2022 21:47

ann0see reopened this Feb 26, 2022

ann0see commented Feb 26, 2022

View reviewed changes

ann0see requested a review from pljones February 26, 2022 21:58

ann0see added the stale label Mar 4, 2022

ann0see added 2 commits March 12, 2022 23:20

Don't imply that multiple drivers have errors

0d8ae5b

If there's only one driver with an error message, there's no need to imply that multiple devices have errors. This change is done to simplify the error message.

Do not show ASIO related warning message if using JACK

fd7bfe6

ann0see force-pushed the feature/smaller-asio-error-msg branch from 3657809 to fd7bfe6 Compare March 12, 2022 22:21

ann0see mentioned this pull request Mar 12, 2022

Do not show ASIO related warning message if using JACK #2494

Merged

5 tasks

ann0see commented Mar 12, 2022

View reviewed changes

ann0see added a commit to ann0see/jamulus that referenced this pull request Mar 12, 2022

Simplify device driver error message

f6b6008

Split out from jamulussoftware#2168

ann0see mentioned this pull request Mar 12, 2022

Simplify device driver error message #2496

Merged

5 tasks

ann0see closed this Mar 12, 2022

ann0see added a commit to ann0see/jamulus that referenced this pull request Mar 13, 2022

Simplify device driver error message

cc03211

Split out from jamulussoftware#2168

ann0see added a commit to ann0see/jamulus that referenced this pull request Mar 13, 2022

Simplify device driver error message

e966f06

Split out from jamulussoftware#2168

ann0see added a commit to ann0see/jamulus that referenced this pull request Mar 13, 2022

Simplify device driver error message

7c9032e

Split out from jamulussoftware#2168

pgScorpio mentioned this pull request Mar 13, 2022

audio redesign #2407

Closed

5 tasks

ann0see added a commit to ann0see/jamulus that referenced this pull request Mar 22, 2022

Simplify device driver error message

aaf4db6

Split out from jamulussoftware#2168

ann0see added a commit to ann0see/jamulus that referenced this pull request Apr 12, 2022

Simplify device driver error message

8b98911

Split out from jamulussoftware#2168

ann0see added a commit to ann0see/jamulus that referenced this pull request Apr 13, 2022

Simplify device driver error message

4cf7dec

Split out from jamulussoftware#2168

ann0see added a commit to ann0see/jamulus that referenced this pull request May 16, 2022

Simplify device driver error message

c1be675

Split out from jamulussoftware#2168

		@@ -166,30 +166,38 @@ QString CSoundBase::SetDev ( const QString strDevName )
		if ( !vsErrorList.isEmpty() )

		// TODO: This or some related code aborts the app even if the driver is valid after user changes.
		// This should be handled differently. For reference, please see https://github.com/jamulussoftware/jamulus/pull/2168

Don't imply that multiple drivers have errors #2168

Don't imply that multiple drivers have errors #2168

Conversation

ann0see commented Dec 10, 2021 • edited Loading

Checklist

gilgongo commented Dec 11, 2021

pljones commented Dec 11, 2021 • edited Loading

gilgongo commented Dec 11, 2021 • edited Loading

pljones commented Dec 11, 2021 • edited Loading

gilgongo commented Dec 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pljones Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

pljones Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ann0see commented Dec 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pljones Dec 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pljones commented Dec 22, 2021

ann0see commented Dec 25, 2021

ann0see commented Feb 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ann0see Feb 26, 2022 • edited Loading

Choose a reason for hiding this comment

ann0see left a comment

Choose a reason for hiding this comment

pljones commented Feb 27, 2022

ann0see left a comment

Choose a reason for hiding this comment

ann0see commented Mar 12, 2022

ann0see commented Dec 10, 2021 •

edited

Loading

pljones commented Dec 11, 2021 •

edited

Loading

gilgongo commented Dec 11, 2021 •

edited

Loading

pljones commented Dec 11, 2021 •

edited

Loading

gilgongo commented Dec 11, 2021 •

edited

Loading

pljones Dec 16, 2021 •

edited

Loading

pljones Dec 16, 2021 •

edited

Loading

pljones Dec 20, 2021 •

edited

Loading

ann0see Feb 26, 2022 •

edited

Loading