Speak all symbols when moving by words (#11779)

[JulienCochuyt]

Link to issue number:

Fixes #11779
Fixes #10855

Summary of the issue:

Navigation by word is inconsistent among different editors.
This might lead to different symbol announcements, or opening and closing symbols not announced consistently.

Description of how this pull request fixes the issue:

• Add a new "symbolLevelWord" key in the "speech" config category, setting the symbol level to use when reviewing by word.
  Default: All
• Add a new constant in characterProcessing: SYMLVL_UNCHANGED = -1
• The Settings UI and an unassigned global command allow to toggle between "All" and "Unchanged"

Testing performed:

Played with #11779 STR in different editors.

System tests have been added to test the STR with notepad.

Known issues with pull request:

This effectively reverts PR #11167.
The reverted portion of the code would benefit from extra clean-up, as I guess there is no need to test for UNIT_WORD there anymore.

Change log entry:

Changes:

• NVDA now speaks by default every symbol when navigating by word. This can be toggled in the Preferences dialog or with a new (unassigned) command. (Inconsistent symbol pronunciation for words with only symbols when reading by word #11779)

For developers:

• characterProcessing.SYMLVL_* constants should be replaced using their equivalent SYMLVL.* before 2022.1. (Speak all symbols when moving by words (#11779) #11856)

Code Review Checklist:

• Pull Request description is up to date.
• Unit tests.
• System (end to end) tests.
• Manual testing.
• User Documentation.
• Change log entry.
• Context sensitive help for GUI changes.
• UX of all users considered:
  • Speech
  • Braille
  • Low Vision
  • Different web browsers

[OzancanKaratas]

It works as expected.

[CyrilleB79]

Sorry to comment a bit late on this topic.
I have just tested the snapshot of this PR.
Reading the following sentences (in notepad or Word) is less smooth with this PR than with 2020.3:

• (in English) I think he'd have liked it, but he didn't.
• (in French) Aujourd'hui, l'école n'a pas d'élèves absent.
  Indeed, the apostrophes in the words are pronounced and thus, the words are less understandable.
  Was it expected?

As an alternative, has it been considered to force symbol pronunciation only when they appear at the start of end of the word? Let's imagine the formatted word is retrieved as follows:

1. 0 or more symbols
2. one letter
3. 0 or more symbol or letter
4. one letter
5. 0 or more symbols

Taking the word "didn't." as an example we would split it as follows:

1. empty (0 symbol)
2. letter "d"
3. letters & symbols "idn'"
4. letter "t"
5. symbol "." (dot)

I would then suggest to ignore char pronunciation level for symbols in part 1. and 5., but not in part 3.

I know that finding a compromise on this topic is not easy. And maybe all this has already been discussed. In this case, feel free to ignore this comment and proposal.

[michaelDCurran]

I think the concern around apostrophe in the middle of words when navigating by word with this pr is valid, but at the same time I don't think it is anywhere near as bad as not saying anything at all / providing an unpredictable experience. My vote would be to take this pr, but perhaps a little more discussion should be had first.

[JulienCochuyt]

@CyrilleB79, I also agree your concerns are valid.
Still, I feel the proposed change improves upon the current situation, even if in an imperfect way.
I guess it all depends on what you consider as the primary use of navigating by words.
When used to re-read a misheard word, this change might have a negative impact depending on how the editor splits by word.
When used to proof-read text, this change is IMHO a big improvement, whatever the editor.
The "re-read" use case would benefit from handling differently symbols at the boundary or in the middle of a word.
The "proof-read" use case would not.

[LeonarddeR]

Couldn't we make this optional? So after the punctuation/symbol level combo box, add a checkbox "Always speak all symbols when navigating by word"

[lukaszgo1]

I wholeheartedly agree with @CyrilleB79 's comment. Making this configurable would not solve the problem he outlined IMHO and announcing symbols only at the beginnings/ends of words seems doable though certainly not as easy as what this PR implements.

[Adriani90]

Does this fix also #10855? If yes, then that issue should be contained in this description of the PR to make sure it will be closed once this is merged.

[JulienCochuyt]

@Adriani90 wrote:

Does this fix also #10855? If yes, then that issue should be contained in this description of the PR to make sure it will be closed once this is merged.

Good catch. Thanks!

[michaelDCurran]

My vote is to speak words with symbol level of all by default, but allow the user to turn this off with an option if they must. This then means that by default, no symbols are missed, and the rule is totally predictable. With it off, the user gets the current behaviour. I think that trying to come up with rules about speaking specific symbols at the start or end of a word will then produce functionality which is confusing and more likely to break.
NV Access will merge this if this pr is made configurable and on by default.

[CyrilleB79]

My vote is to speak words with symbol level of all by default, but allow the user to turn this off with an option if they must. This then means that by default, no symbols are missed, and the rule is totally predictable. With it off, the user gets the current behaviour. I think that trying to come up with rules about speaking specific symbols at the start or end of a word will then produce functionality which is confusing and more likely to break.
NV Access will merge this if this pr is made configurable and on by default.

I support this solution that allows to merge this PR quickly. In a second time, another PR could be suggested after testing this one to add a third option allowing to speak punctations only at the start and ends of words.

For the current PR, I would advocate to add an unbounded gesture to be able to change quickly this verbosity option.

[JulienCochuyt]

Alternative implementation

@michaelDCurran wrote:

I think that trying to come up with rules about speaking specific symbols at the start or end of a word will then produce functionality which is confusing and more likely to break.

I agree. This would also not fully fit the "proof-read" use case, which IMHO should take priority over the "re-read" one.

Configurability

@michaelDCurran wrote:

allow the user to turn this off with an option if they must

@LeonarddeR wrote:

after the punctuation/symbol level combo box, add a checkbox "Always speak all symbols when navigating by word"

Do you all agree with adding a new regular setting in the Speech category? Or would you prefer an experimental one in the Advanced category for less intrusive removal if we come up with a better approach?

@CyrilleB79 wrote:

I would advocate to add an unbounded gesture to be able to change quickly this verbosity option.

This would IMHO orient towards a regular setting in the Speech category.

What do you think?

[CyrilleB79]

I agree to put this option in the speech category.
The solution to speak words with symbol level of all seems quite simple. Thus I would not expect it to land in the advanced settings.

[AppVeyorBot]

• Build (for testing PR)

See test results for failed build of commit b77fcdd71f

[JulienCochuyt]

Added configurability and unassigned global command.
Edited PR description accordingly for clarity.

[CyrilleB79]

Also I have had a look at the code and have noted the following comments:

• Why are you using a 3-state checkbox for this option? It seems to me that the symbol level when navigating by word can only be unchanged or all.
• If this is the case, the code would also be easier to read if you were using a boolean in the config; you may get rid of the "unchanged" level and do not need to store a symbol level in the config, but only the state of the option to read all symbols when navigating by words.

Is all this coded the way you did in order to support more symbol levels in the future?

[JulienCochuyt]

@CyrilleB79 wrote:

Also I have had a look at the code and have noted the following comments:
Thank you for reviewing.

• Why are you using a 3-state checkbox for this option? It seems to me that the symbol level when navigating by word can only be unchanged or all.

The checkbox implements the high-level on/off toggling from the UI.
The third state allows the config to hold any other valid value, left unchanged when saving settings unless explicitly changed from the checkbox or the (unassigned) global command. A user won't ever be presented the third state unless it has manually edited the actual value.

• If this is the case, the code would also be easier to read if you were using a boolean in the config; you may get rid of the "unchanged" level and do not need to store a symbol level in the config, but only the state of the option to read all symbols when navigating by words.

Actually, the implementation of the new behavior really boils down to "Use another symbol level when reviewing by word"
That is, depending on the setting, pass a symbol level or let default.
Storing a bool versus storing an int is here mapping bool to int in speech module versus mapping bool to int in settingsDialogs module.
I choose the latter as I had no reason to impose a limitation.
I can of course modify it to store a bool instead if requested to.

Is all this coded the way you did in order to support more symbol levels in the future?

I indeed had your #11856 (comment) in mind when not limiting support for evolution.

[dpy013]

hi @JulienCochuyt
and
@michaelDCurran
Please fix
Conflicting files
source/globalCommands.py
thanks

[SeanTolstoyevski]

Sorry for participating in an irrelevant way. There is another similar problem (#12556).
But I don't know if this pr will fix it.
The problem is that there is a problem for Turkish. No problem for English.

Different speech behavior for each language precludes me from understanding whether this pr covers it.
If it works correctly for English, I think the problem may be something different for Turkish.
Maybe a wrong flag in the Turkish symbol tables.

[seanbudd]

You can test if this fixes your problem by running a copy of this build. Happy to reopen your issue if it is not a duplicate, and has a separate cause.

[feerrenrut]

This looks ready to go. I'll leave it up to you about what to do with the test.

[feerrenrut]

Logic in tests smells, being pragmatic it may not easily be avoidable. The problems it introduces are:

• higher possibility of a bug in the test itself.
• the test is harder to read, it doesn't serve as documentation for the expected behavior as well.

Parameterized tests are useful, but mostly when there is a lot of data to get through. Say every combination of a number of parameters. If it's a small number of combinations personally I think explicit is better.

You could instead have something like:

wordsToExpectedSymbolSpeech = {
'Say':'Say',
'(quietly)': 'left paren  quietly  right paren',
'"Hello,': 'quote  Hello  comma',
'Jim".': 'Jim  dot  quote',
}
def test_symbolLevelWord_symbolLevelWord:
	textStr = ' '.join(wordsToExpectedSymbolSpeech.keys())
	_notepad.prepareNotepad(f"Test: {textStr}")
	for expectedWord in wordsToExpectedSymbolSpeech.values():
		wordSpoken = _notepad.getSpeechAfterKey("numpad6")  # navigate to next word
		# unlike other symbols used, symbols.dic doesn't preserve quote symbols with SYMPRES_ALWAYS
		expectedWord = expectedWord.replace('"', ' ').strip()
		_asserts.strings_match(wordSpoken, expectedWord)

wordsToExpectedSymbolSpeech = {
'Say':'Say',
'(quietly)': 'quietly',
'"Hello,': 'Hello',
'Jim".': 'Jim',
}
def test_symbolLevelWord_symbolLevelSome:  # todo: is 'some' the correct level?
	textStr = ' '.join(wordsToExpectedSymbolSpeech.keys())
	_notepad.prepareNotepad(f"Test: {textStr}")
	for expectedWord in wordsToExpectedSymbolSpeech.values():
		wordSpoken = _notepad.getSpeechAfterKey("numpad6")  # navigate to next word
		# unlike other symbols used, symbols.dic doesn't preserve quote symbols with SYMPRES_ALWAYS
		expectedWord = expectedWord.replace('"', ' ').strip()
		_asserts.strings_match(wordSpoken, expectedWord)

[seanbudd]

I prefer that approach too, I've updated the tests to follow this pattern, but I have made some changes to simplify it further, if you want to take another look.

[feerrenrut]

Looks good to me.