-
-
Notifications
You must be signed in to change notification settings - Fork 638
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
Allow NVDA to report indentation as tones #5906
Comments
Hi, |
Sounds good.
Makes sense.
Is both pitch and stereo useful here? Is stereo actually accurate enough in terms of a user's ability to detect a small, discrete change? What do you do if stereo is enabled but pitch isn't?
I'm concerned about complexity and confusion here. Too many options = can't be bothered, too hard.
It's not really ideal for getIndentationSpeech to actually produce output; it's supposed to just return output. In theory, it could be called but the result might not be spoken... or it might be called multiple times with only one result spoken. That said, the current speech system doesn't give you any other option, so this will have to do for now. Still, we should note somewhere that this should be fixed once speech refactor happens.
Understand not wanting to bloat, but I'm guessing it should only be a couple of lines of code. It doesn't make sense to me to have tones.reportIndentation or the like, since it'll only ever get used by this code. If you like, you could split it out into a private helper function in speech.
That's fine, though if you're going to fetch, say, more than 3 values from the same section, it's probably worth grabbing the section as a local variable. |
|
Edit: tab/four spaces = whole tone, two spaces is half that, a semi/half On 6/8/2016 5:11 PM, James Teh wrote:
Websites: email me at [email protected] mailto:[email protected] |
IMO, we should just drop stereo. It doesn't seem to be practical without pitch, and if you've got pitch, there's little point in expressing it with stereo as well. I think just go for 2 spaces as a semitone, tab as a tone. We can always add the edit field later if really necessary. Easier to add more options later than take them away later.
Meh. Just do the beep in getIndentationSpeech. We'll convert it to a SpeechCommand post speech refactor. |
Incubated in 5859154. |
It seems that, when reporting is set to tones, speech is still used to say "no indentation" while it shouldn't. |
confirmed. Let me fix. On 6/10/2016 8:39 AM, Leonard de Ruijter wrote:
Websites: email me at [email protected] mailto:[email protected] |
Hi, I just realized that we could make this better by getting the current then when played tones.beep(pitch, duration, left=left, right=right) On 6/10/2016 8:39 AM, Leonard de Ruijter wrote:
Websites: email me at [email protected] mailto:[email protected] |
|
In fact, I would highly recommend we do this change. without this, the volume is too loud for the synth at low volumes, which drowns it out. I'll add to the pull request later. |
@leonardder commented on Jun 10, 2016, 11:59 AM MDT:
This oddly doesn't happen with speech mode of beeps. @jcsteh any idea why it happens with off but not speech?
This might be a wx bug. I could remove the access key for this combo box. I don't know why this happens, maybe because this box is disabled, the access key still trys to go to it, and focus lands on the next enabled control.
|
I don't follow. Do you mean just add off to the combo box? |
That actually might be a nice UI improvement. The checkbox could be removed, and the options would be
|
That's exactly how I mean it to be. Not sure whether this breaks
compatibility with the older boolean behaviour though. May be off should
be mapped to false and speech should be mapped to true to be compatible
with earlier configurations.
|
That's why I did it this way but theoretically we could map off to false in any other value to true as well as that value in the config Sent from a mobile device.
|
Combining these into one option makes sense. One idea is to just have two booleans: reportIndentation used for speech (for backwards compat) and reportIndentationUsingTones. It seems pointless to bother with bitwise flags when we have to have two settings anyway. There's a separate issue open about having sounds follow the syth volume. There are problems with this; e.g. synth volume levels differ wildly, it's not entirely intuitive that adjusting voice volume will adjust sound volume, etc. So, I think this should perhaps be addressed separately. As to why it doesn't happen for speech mode beeps, tones will only play one beep at a time; a subsequent call will override the earlier one. So, the speech beep overrides the indentation beep. You should disable this if speech mode is not speech. |
The issue is that the tones can be too loud here. Is there a clean way On 6/10/2016 2:55 PM, James Teh wrote:
Websites: email me at [email protected] mailto:[email protected] |
Sure, but that's true for all tones (progress bar beeps, audio coordinates, etc.). By all means happy to discuss, but it's a more general issue.
|
The issue here (I would suggest using the tones for a while with code Also, did you see my fix for the no indent issue? On 6/10/2016 3:38 PM, James Teh wrote:
Websites: email me at [email protected] mailto:[email protected] |
I get it, but if you do this here, it's inconsistent with all other beeps. Aside from the questions that raises for users, you'll also need to test edge cases; e.g. if the synth doesn't support volume (which the API does allow). Also, if tones is later updated to use voice volume, this code has to be updated. |
I understand. I was just trying to think of a better solution. On 6/10/2016 4:10 PM, James Teh wrote:
Websites: email me at [email protected] mailto:[email protected] |
Your latest change looks fine. Are you planning to address these two issues:
and
|
Hi, I do plan to implement this (probably this weekend or some evening). Thanks. On 6/14/2016 11:53 PM, James Teh wrote:
Websites: email me at [email protected] mailto:[email protected] |
@jcsteh It's kind of a pointless feature if we don't have a only report indents as tones option. With the above in mind, disabling the whole feature if speech is on is a bad idea. I'd propose "report indents as tones or report indents as speech" as the condition. Therefore, the user can check either box. |
I'm not sure that was clear. I'm meaning that only if both boxes are unchecked we don't report indents. They'll be off both by default. |
If speech is off, I'm arguing it's not useful to play tones, even if
only tones are enabled. If you've turned off speech, presumably, you
want silence when pressing commands.
|
Oh. That issue. I thought this was referencing disabling tones if they On 6/17/2016 10:16 AM, James Teh wrote:
Websites: email me at [email protected] mailto:[email protected] |
Okay. This has gotten confused. So we were discussing this:
I meant combining these into one GUI option instead of two. When I said this:
I was just referring to the internal config variables. So, even though it's a single option in the GUI, it would be two boolean options in the config to preserve backwards compat, while still being simpler than the bitwise stuff. Either or both of them could be turned off; they aren't mutually exclusive. |
This no longer beeps with speech mode off. re nvaccess#5906 @jcsteh I haven't had time to update the user guide yet, I'll let you know when that's complete.
Extra clarification: a combo box with four options: off, speech, tones, both. |
Ah. I'll redesign the GUI later. On 6/17/2016 10:31 AM, James Teh wrote:
Websites: email me at [email protected] mailto:[email protected] |
@jcsteh considder this ready for further scrupulation.
@derekriemer, Could it be that you accidentally missed the script in globalCommands.GlobalCommands.script_toggleReportLineIndentation which allows for setting a shortcut to toggle line indent reporting? I think that script has to reflect the new tones and speech and tones settings. Thanks! |
Yes. @jcsteh I can't work on this until at least next week, and I will be On 9/8/2016 11:16 PM, Leonard de Ruijter wrote:
Websites: email me at [email protected] mailto:[email protected] |
Or at least stop it from going into stable? On 9/8/2016 11:16 PM, Leonard de Ruijter wrote:
Websites: email me at [email protected] mailto:[email protected] |
Stable is just master frozen at a certain point in time, so we can't
prevent things from going to stable without removing them from master.
I'll leave it in master for another week or so, but I'll probably need
to remove it soon after that if we don't have a fix.
|
Hi, If you don’t mind, I’ll take a stab at it tomorrow afternoon, as we also need to provide Gettext context also. From: James Teh [mailto:[email protected]] Stable is just master frozen at a certain point in time, so we can't — |
…toins into account. re nvaccess#5906 Noted by a reviewer: better to recognize and add new options early than back out. The line indentation toggle command now steps through speech, tones, speech and tones, and off.
…ption for line indentation. re nvaccess#5906
…for line indentation toggle script simplified. re nvaccess#5906 Reviewed by Jamie Teh (NV Access): * Simplified docstring for line indentation toggle script. * Capitalized messages, updated translator comments.
…#5906 Reviewed by Jamie Teh (NV Access): * Because it is no longer a simple toggle, translator comment should reflect this. * Say 'reported' instead of 'announced' for clarify.
…access#6520 Specifically: * nvaccess#5906: Now labeled as 'line indentation reporting'. * nvaccess#6099: clarify how to change values in spin controls. * nvaccess#5886: elements list is available in browse mode. * nvaccess#6206: changed bits such as 'adding new entries'. * nvaccess#6127: no more hyphen (dash). * nvaccess#5050: 'causes' -> 'which caused'. * nvaccess#4164: changed wording to reflect that read-only edit fields are now included.
In Visual Studio, when line numbers are enabled, this doesn't seem to work. Turning them off makes it work as expected, but when they are on it appears NVDA thinks the line numbers are the first characters of the line rather than the indentation chars, mucking things up. |
How do I enable/disable line numbers in visual studeo? On 11/22/2016 2:25 PM, Florian Beijers wrote:
Websites: email me at [email protected] mailto:[email protected] |
I propose adding functionality to NVDA to add the ability to report indents as tones.
My design ideas are based off of my indentone add-on at
https://github.com/derekriemer/nvda-indentone
Make NVDA report indents as tones when the user selects this option. This option might be in document formatting. We'd need to enable several new items if report indents is enabled that controls report indents .
The code design of my current add-on monkeyPatches speech.getIndentationSpeech to do this. I'd probably put most of this code there, and maybe put a tones.reportIndentation item in tones.py so that I don't bloat the speech.getIndentationSpeech function with unnecessary math and stuff. We could just use beep inside tones in that case. As for getting config values, is it fine performance wise to just use config.conf["documentFormatting"]["reportIndentsAsStereo"] for this or is that too slow?
The text was updated successfully, but these errors were encountered: