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

Manual update check doesn't always respect progress bar output config #6759

Closed
dkager opened this Issue Jan 18, 2017 · 7 comments

Comments

Projects
None yet
5 participants
@dkager
Collaborator

dkager commented Jan 18, 2017

gui.IndeterminateProgressDialog.done() doesn't check the configured output method for progress bars.
Also slight ugliness in the hard-coded frequencies and lengths of the beeps. Maybe add functions beepIndeterminate() and beepDone() to the tones module?

@jcsteh

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh Jan 18, 2017

Contributor
Contributor

jcsteh commented Jan 18, 2017

@dkager

This comment has been minimized.

Show comment
Hide comment
@dkager

dkager Jan 19, 2017

Collaborator

Would you expect this to say "done"?

No, but neither do I expect it to beep when I have disabled that feature. In that case it would be a simple check, either beep or just return.
Arguably the "done" beep doesn't provide much information because the dialog is likely to change at that time anyway.

Collaborator

dkager commented Jan 19, 2017

Would you expect this to say "done"?

No, but neither do I expect it to beep when I have disabled that feature. In that case it would be a simple check, either beep or just return.
Arguably the "done" beep doesn't provide much information because the dialog is likely to change at that time anyway.

@feerrenrut

This comment has been minimized.

Show comment
Hide comment
@feerrenrut

feerrenrut Jan 19, 2017

Contributor

I'm not familiar enough with all the ways that the tones module is used to be sure we are able to generalise. I can agree that the tones module isn't the place for those constants.

There are a few similarities in how beep is used in progress bars and in the mouse tracking code. In both cases there the beeps are indicating a value along an axis (or two). Perhaps we could abstract that pattern and generalise it, creating an in-between module/class that allows code like beepPosition(minValue, maxValue, actualValue) or similar. It could take the min and max frequency as well as the length as parameters on construction. Those values can either be constants in the class (in the case of IndeterminateProgressBar), or come from config (in the case of mouse tracking).

Contributor

feerrenrut commented Jan 19, 2017

I'm not familiar enough with all the ways that the tones module is used to be sure we are able to generalise. I can agree that the tones module isn't the place for those constants.

There are a few similarities in how beep is used in progress bars and in the mouse tracking code. In both cases there the beeps are indicating a value along an axis (or two). Perhaps we could abstract that pattern and generalise it, creating an in-between module/class that allows code like beepPosition(minValue, maxValue, actualValue) or similar. It could take the min and max frequency as well as the length as parameters on construction. Those values can either be constants in the class (in the case of IndeterminateProgressBar), or come from config (in the case of mouse tracking).

@feerrenrut

This comment has been minimized.

Show comment
Hide comment
@feerrenrut

feerrenrut Jan 19, 2017

Contributor

In terms of prioritising this issue, while it does not look like it would take long to fix the impact is fairly low. Setting to P3

Contributor

feerrenrut commented Jan 19, 2017

In terms of prioritising this issue, while it does not look like it would take long to fix the impact is fairly low. Setting to P3

@dkager

This comment has been minimized.

Show comment
Hide comment
@dkager

dkager Jan 19, 2017

Collaborator

So let's leave adding constants or otherwise refactoring the beeps out of this issue for now. It looks like it needs more thought.
But do we agree that the "done" state should not cause a beep if the output method is set to Speech or None? I can create a PR for this, it seems rather simple.

Collaborator

dkager commented Jan 19, 2017

So let's leave adding constants or otherwise refactoring the beeps out of this issue for now. It looks like it needs more thought.
But do we agree that the "done" state should not cause a beep if the output method is set to Speech or None? I can create a PR for this, it seems rather simple.

@jcsteh

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh Jan 19, 2017

Contributor
Contributor

jcsteh commented Jan 19, 2017

dkager added a commit to dkager/nvda that referenced this issue Jan 21, 2017

@feerrenrut feerrenrut closed this in #6776 Jun 27, 2017

@nvaccessAuto nvaccessAuto added this to the 2017.3 milestone Jun 27, 2017

feerrenrut added a commit that referenced this issue Jun 27, 2017

feerrenrut added a commit that referenced this issue Jun 27, 2017

Update changes file for PR #6776
Beeps for indeterminate progress bar dialogs such as the update checker only when progress bar output is configured to include beeps. Issue #6759
@fisher729

This comment has been minimized.

Show comment
Hide comment
@fisher729

fisher729 Aug 4, 2017

I got confused with the wording of this change in the What's New. Of course, I know what it does, however. Essentially, when reporting progress bars is not set to include beeps, NVDA won't beep for the update checker dialog and other dialogs I've seen.

fisher729 commented Aug 4, 2017

I got confused with the wording of this change in the What's New. Of course, I know what it does, however. Essentially, when reporting progress bars is not set to include beeps, NVDA won't beep for the update checker dialog and other dialogs I've seen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment