Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move driver setup button under driver selection #976 #977

Merged
merged 10 commits into from
Feb 15, 2021
Merged

Conversation

ann0see
Copy link
Member

@ann0see ann0see commented Feb 10, 2021

Solves #973

The tooltip of this still tells you about the buffer settings. I'm not sure how to change that. Done

@ann0see ann0see linked an issue Feb 10, 2021 that may be closed by this pull request
@pljones
Copy link
Collaborator

pljones commented Feb 10, 2021

How to find out:

peter@fs-peter:~/git/Jamulus-wip$ grep -nr butDriverSetup src
src/clientsettingsdlgbase.ui:245:          <widget class="QPushButton" name="butDriverSetup">
src/clientsettingsdlgbase.ui:677:  <tabstop>butDriverSetup</tabstop>
src/clientsettingsdlg.cpp:185:    butDriverSetup->setWhatsThis ( strSndCrdBufDelay );
src/clientsettingsdlg.cpp:186:    butDriverSetup->setAccessibleName ( tr ( "ASIO setup push button" ) );
src/clientsettingsdlg.cpp:187:    butDriverSetup->setToolTip ( strSndCrdBufDelayTT );
src/clientsettingsdlg.cpp:281:    butDriverSetup->setText ( tr ( "ASIO Setup" ) );
src/clientsettingsdlg.cpp:284:    butDriverSetup->hide();
src/clientsettingsdlg.cpp:418:    QObject::connect ( butDriverSetup, &QPushButton::clicked,

So 185, 186, 187.

@ann0see
Copy link
Member Author

ann0see commented Feb 10, 2021

Ok. Thanks! Now the question is: What should the text be?

My first thought:


    // Driver setup button
    QString strSndCardDriverSetup = "<b>" tr ( "Sound card driver settings" ) + "</b>" +
        tr ( "The sound card driver settings button opens the driver settings pannel of " +
        "your sound card. Some drivers allow you to change buffer settings, others like the " +
        "ASIO4ALL let you choose input or outputs of your device(s). To show the inputs or outputs " +
        "in ASIO4ALL, click on the tool icon (= advanced view) on the bottom right, " +
        "click the plus icon left to the sound card and afterwards enable the correct input and output. " +
        "By hovering over an input output under a soundcard, you can see if it is an input or output. " +
        "Make sure to only enable devices which support a sample rate of " ) + SYSTEM_SAMPLE_RATE_HZ +
        tr ( "Hz. To check this in ASIO4ALL, hover over the device or input/output and check the " +
        "supported sample rate. New devices will not automatically be added to ASIO4ALL. Therefore, " +
        "please restart " ) + APP_NAME + tr ( " if you connect new devices." );

    QString strSndCardDriverSetupTT = tr ( "Opens the driver settings. Note: " + APP_NAME + " currently " +
        "only supports devices supporting a sample rate of " + SYSTEM_SAMPLE_RATE_HZ "Hz. " +
        "You will not be able to select a driver/device which doesn't. " +
        "If you use ASIO4ALL, you might need to choose the correct input output devices which also support " +
        "this sample rate. Check this by hovering over a device. Selecting devices " +
        "in ASIO4ALL can be done by clicking here, afterwards in ASIO4ALL clicking on the tool icon "
        "(advanced view), then on the plus icon left to the sound card. Now select the correct input or output "
        "devices. For more help see the Website." ) + TOOLTIP_COM_END_TEXT;

@pljones
Copy link
Collaborator

pljones commented Feb 10, 2021

I would not add anything specific to any driver. Reference the web site for more guidance on setting up your sound card but nothing deeper than that.

@ann0see
Copy link
Member Author

ann0see commented Feb 10, 2021

Ok. So I'd be happy about a suggestion from you and @gilgongo

@ann0see
Copy link
Member Author

ann0see commented Feb 12, 2021

What about:


    // Driver setup button
    QString strSndCardDriverSetup = "<b>" tr ( "Sound card driver settings" ) + "</b>" +
        tr ( "The sound card driver settings button opens the driver settings pannel of " +
        "your sound card. Some drivers allow you to change buffer settings, others like the " +
        "ASIO4ALL let you choose input or outputs of your device(s)." );

    QString strSndCardDriverSetupTT = tr ( "Opens the driver settings. Note: " + APP_NAME + " currently " +
        "only supports devices supporting a sample rate of " + SYSTEM_SAMPLE_RATE_HZ "Hz. " +
        "You will not be able to select a driver/device which doesn't. " +
        "For more help see the Website." ) + TOOLTIP_COM_END_TEXT;

@pljones
Copy link
Collaborator

pljones commented Feb 12, 2021

I've not thought of anything better myself. I'd still be inclined to say less, though, on strSndCardDriverSetup -- maybe completely lose the last sentence? It should just say it gets you to the sound card settings - I feel saying more is making it sound like it's somehow something Jamulus controls.

@ann0see
Copy link
Member Author

ann0see commented Feb 12, 2021

Sure, It can be removed. I might need to add my explanation to this:

My intention behind adding ASIO4ALL was to help new users (who probably use ASIO4ALL and will therefore click this button and open this help text), should at least receive some help here. That's why I initially wrote about this information. We might need to think a bit more of ASIO4ALL first user problems. That's probably a topic for another issue.

@pljones
Copy link
Collaborator

pljones commented Feb 12, 2021

Yep, I understand the intent - but for non-ASIO4ALL users, it could be off-putting / misleading. I think things like this are better handled in "first time user" hand-holding.

@ann0see
Copy link
Member Author

ann0see commented Feb 12, 2021

I think things like this are better handled in "first time user" hand-holding.

... which doesn't exist yet ;-). But of course, this is another issue.

Ok. So maybe your suggestion should be applied then? Any other thoughts?

@gene96817
Copy link

I appreciate the level of attention and effort for addressing issues with AISO4ALL.

MacOS users need to be assured they do not have similar or related problems.

With all the discussion about AISO4ALL, how should Windows users think about manufacture specific AISO drivers. The current documentation suggest they use the AISO4ALL driver. I have seen the Dell AISO driver perform better. Could a user see more than three ASIO drivers installed in their machine?

@ann0see
Copy link
Member Author

ann0see commented Feb 13, 2021

I have seen the Dell AISO driver perform better.

Just a short question: is it a generic driver too?

Could a user see more than three ASIO drivers installed in their machine?

Yes. I don't know why they shouldn't.

@gene96817
Copy link

The Dell ASIO appears to only know about the built-in audio circuit.

@ann0see
Copy link
Member Author

ann0see commented Feb 13, 2021

Ok. Can you link it? I'd like to test it on my HP PC. Maybe it's compatible?

@hoffie
Copy link
Member

hoffie commented Feb 13, 2021

Ok. Can you link it? I'd like to test it on my HP PC. Maybe it's compatible?

Just found this: https://www.dell.com/support/driver/de/de/debsdt1/driversdetails?driverid=7776f

There are also attempts at getting similar things running by editing INF files:
https://www.tenforums.com/sound-audio/150202-realtek-asio-driver-installs-but-fails-operate.html

I have no experience with this. Maybe it'll work better. Doesn't sound easier to set up than ASIO4ALL at the first glance though.

@pljones
Copy link
Collaborator

pljones commented Feb 13, 2021

Could a user see more than three ASIO drivers installed in their machine?

Yep - I've got six on one of my two machines (not the one I actually use for jamming, either!).

@ann0see ann0see self-assigned this Feb 13, 2021
@ann0see
Copy link
Member Author

ann0see commented Feb 14, 2021

Note to self: syntax error.

Still to be tested.

Sorry for the force pushes. I shouldn't work on the main repo.

@ann0see
Copy link
Member Author

ann0see commented Feb 14, 2021

Ok. Ready for review.
I'd especially like to have a feedback on the description here from @gilgongo

@pljones
Should this be part of the next release or should we still wait? I'm a bit afraid that existing users might be confused with the change here. New users in contrary would probably find and understand the new position faster.

@pljones
Copy link
Collaborator

pljones commented Feb 14, 2021

I think most users won't notice the difference, to be honest. If their settings are working, they won't be looking at the ASIO and audio buffer bits. I think it's pretty safe to merge.

@pljones
Copy link
Collaborator

pljones commented Feb 14, 2021

Can you confirm that you have tested this on Linux with JACK and that the hover text and What's This still make sense there?

@ann0see
Copy link
Member Author

ann0see commented Feb 14, 2021

This doesn't even show up on Linux. I will re-test it.

@ann0see
Copy link
Member Author

ann0see commented Feb 14, 2021

Debian 10:

JamulusLinuxSettings

This doesn't even show up.

There are still some things up in #977 but they can also be included in another PR.

@gilgongo
Copy link
Member

Would it be better to use "this" to refer to the element the user is looking at? Maybe also remove the redundant "panel". Shorter at least:

"This opens the driver settings of your sound card. Some drivers allow you to change buffer settings, others like the ASIO4ALL let you choose input or outputs of your device(s)"

@ann0see
Copy link
Member Author

ann0see commented Feb 14, 2021

@gilgongo now I use your suggestion.

Copy link
Member Author

@ann0see ann0see left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pljones @mulyaj added a few suggestions here.

@@ -674,7 +675,6 @@
<tabstop>rbtBufferDelayPreferred</tabstop>
<tabstop>rbtBufferDelayDefault</tabstop>
<tabstop>rbtBufferDelaySafe</tabstop>
<tabstop>butDriverSetup</tabstop>
Copy link
Member Author

@ann0see ann0see Feb 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mulyaj I think we need to re-add this. So you'd add

 <tabstop>cbxSoundcard</tabstop>
 <tabstop>butDriverSetup</tabstop>

right under <tabstops> . I will test if this works.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, it seems to work without this too. Nevertheless, I will do some more tests.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep them. They help accessibility tools, as far as I'm aware.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also quite sure that they should stay. @mulyaj or me can add them (but I'll not do it before tomorrow)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add them back

Added back tabstops, created name for device groupbox
@ann0see
Copy link
Member Author

ann0see commented Feb 15, 2021

Ok. It doesn't seem to change anything. But that's ok I think. So from my point of view, this is ready to be merged now.

@ann0see ann0see requested a review from pljones February 15, 2021 09:34
@pljones pljones merged commit b59fad7 into master Feb 15, 2021
@ann0see ann0see deleted the MoveASIO branch February 15, 2021 20:53
ann0see added a commit that referenced this pull request Feb 15, 2021
@pljones pljones added this to the Release 3.7.0 milestone Nov 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Triage
Development

Successfully merging this pull request may close these issues.

Move ASIO-Setup button above Driver selection
6 participants