Speech commands use sequences by feerrenrut · Pull Request #10371 · nvaccess/nvda

feerrenrut · 2019-10-10T13:51:40Z

Link to issue number:

Summary of the issue:

In order to get many of the benefits of the speech refactor, we need to have speech sequences (rather than single strings) returned from our various getXSpeech methods. This will be necessary if we want to use sounds instead of text to indicate spelling errors, change voice parameters for emphasized text or links, etc.

Description of how this pull request fixes the issue:

The following functions have had their definitions and usages converted:

getControlFieldSpeech
speech.getControlFieldSpeech
- browseMode.BrowseModeDocumentTextInfo.getControlFieldSpeech
- textInfos.TextInfo.getControlFieldSpeech
- speech.getFormatFieldSpeech
getFormatFieldSpeech
- appModules.kindle.BookPageViewTextInfo.getFormatFieldSpeech
- textInfos.TextInfo.getFormatFieldSpeech
- treeInterceptorHandler.RootProxyTextInfo.getFormatFieldSpeech
speech.getSpeechTextForProperties
speech.getIndentationSpeech
speech.getTableInfoSpeech

The speech function getSpeechTextForProperties has been renamed to getPropertiesSpeech

Testing performed:

Generally ran NVDA and inspected the log for errors:

In Chrome and Firefox
Excel, Word
Start menu

Known issues with pull request:

Change log entry:

Section: Changes For Developers

Several speech functions have been changed to return speech sequences
- getControlFieldSpeech
- getFormatFieldSpeech
- getSpeechTextForProperties / getPropertiesSpeech
- getIndentationSpeech
- getTableInfoSpeech

The speech function getSpeechTextForProperties has been renamed to getPropertiesSpeech

Definitions and all usages converted: - getControlFieldSpeech - speech.getControlFieldSpeech - browseMode.BrowseModeDocumentTextInfo.getControlFieldSpeech - textInfos.TextInfo.getControlFieldSpeech - speech.getFormatFieldSpeech - getFormatFieldSpeech - appModules.kindle.BookPageViewTextInfo.getFormatFieldSpeech - textInfos.TextInfo.getFormatFieldSpeech - treeInterceptorHandler.RootProxyTextInfo.getFormatFieldSpeech - speech.getSpeechTextForProperties - speech.getIndentationSpeech - speech.getTableInfoSpeech

feerrenrut · 2019-10-10T13:53:07Z

-		out = ""
+	def getFormatFieldSpeech(
+			self, attrs, attrsCache=None, formatConfig=None, reason=None, unit=None, extraDetail=False,
+			initialFormat=False, separator=speech.CHUNK_SEPARATOR


Param separator can be removed. Anyone disagree with this?

nvdaes · 2019-10-10T18:47:03Z

Not sure if issue #10363 is related to this. In this case, as an alternative, characterProcessing.processSpeechSymbols may return a sequence too?

LeonarddeR

Currently this goes pretty wild on Github with firefox, it announces sections, groupings, property pages in situations I don't expect it to. Could find out that quickly where the bug is, probably somewhere in getControlFieldSpeech.

Log errors in speech sequences (unknown types, None entries, empty list entries) Must be enabled in the advanced panel, logging options.

feerrenrut · 2019-10-15T14:51:05Z

Looks like system tests are failing. From the output it seems to be to do with the separator that we are no longer using.

feerrenrut · 2019-10-15T15:24:48Z

Seems like the only cases that doesn't use CHUNK_SEPARATOR = " " is in _reportFormattingHelper (source/globalCommands.py) where separator="\n" is used instead.

The definition of CHUNK_SEPARATOR the following comment:

#: The string used to separate distinct chunks of text when multiple chunks should be spoken without pauses.
# #555: Use two spaces so that numbers from adjacent chunks aren't treated as a single number
# for languages such as French and German which use space as a thousands separator.

The CHUNK_SEPARATOR is appended to each string (when not in character mode) at the end of the speak method in source/speech/__init__.py

This was introduced in 7f87073, which introduces browseable messages. It may be necessary to separate text with new lines for the browseable message viewer?

In the future it would be better to test speech as structured data. This would allow us to test commands as well.

feerrenrut · 2019-10-15T17:43:20Z

This is ready for testing: try build

I wouldn't suggest starting to review this again until we get some confirmation from testers. The commits are a bit of a jumble at this stage. I think there is value in making these changes easy to track, so I may rebase to clean up (re-order) the commits, then do a regular merge rather than squash merge.

nvdaes · 2019-10-16T03:44:05Z

I'm testing the try build and I find that, in browse mode, NVDA reports "paragraph" and "section" too frequently. For example, reading the last email corresponding to this PR on in Google's home page.
Congrats for this wonderful work for speech.

LeonarddeR

Not really reviewing, just trying to find out what causes the reporting of roles.

LeonarddeR · 2019-10-16T06:20:37Z

@@ -887,21 +959,36 @@ def speakTextInfo(info, useCache=True, formatConfig=None, unit=None, reason=cont
 				presCat=field.getPresentationCategory(newControlFieldStack[0:count],formatConfig,reason)
 				if not presCat or presCat is field.PRESCAT_LAYOUT:


Just a suggestion:

Initially, I thought that the presentation category code might cause the section reporting, but it is untouched. Yet, I'd like to point your attention at it. textInfos.ControlField.PRESCAT_LAYOUT is None, which to me is pretty confusing. may be it should be changed to "layout" so we can use proper type checking to avoid issues now or in the future.

This is a good point. I would be with you on this idea, I'd be interested to know what @jcsteh thinks though?

Jamie:
In short the value for textInfos.ControlField.PRESCAT_LAYOUT is None, how risky would it be to give this an actual default value like "layout"? Is there a reliance on not being able to retrieve an attribute or dictionary value and therefore None matches the intended default?

Happy for the answer to be "Not sure, please investigate" but if you know off-hand it might save us time.

feerrenrut · 2019-10-16T08:17:09Z

@nvdaes Thanks for testing

in browse mode, NVDA reports "paragraph" and "section" too frequently.

Could you confirm whether you see the same behaviour with the latest alpha?

@LeonarddeR

this goes pretty wild on Github with firefox, it announces sections, groupings, property pages in situations I don't expect it to

I went looking for strange behaviour on Github with firefox with the most recent commit. I found some things I didn't expect, but this is also the case using 2019.2.

feerrenrut · 2019-10-16T09:15:18Z

textInfos.ControlField.PRESCAT_LAYOUT is None, which to me is pretty confusing. may be it should be changed to "layout" so we can use proper type checking to avoid issues now or in the future.

I agree in theory, though by having a value of None the result might be that it is acting as a default when nothing else is set. We would need to track down all these cases and make it explicit.

LeonarddeR · 2019-10-16T09:15:54Z

On alpha, when I do ctrl+end from the comment edit box in browse mode to get to the bottom of this github page, NVDA says: content info landmark visited link About

With the code of this pr, the announcement is: content info landmark unknown list item visited link About

The unknown and list item roles shouldn't be announced.

The speech viewer also shows several unexpected empty lines.

feerrenrut · 2019-10-16T13:27:03Z

With the code of this pr, the announcement is: content info landmark unknown list item visited link About

I am having trouble reproducing this (using Chrome 77.0.3865.90):
I move to comment field, NVDA announces:

Comment body
edit
required
multi line
blank

I press control+end, NVDA announces:

out of edit
section

shows several unexpected empty lines.

The most interesting part to me is:

shows several unexpected empty lines.

This is from the end of your comment, I have no idea how that is ending up in the announcement.

feerrenrut · 2019-10-16T13:32:07Z

I am having trouble reproducing this (using Firefox):

My mistake, I have too many windows open, that result was from Chrome, I will edit the comment.

In Firefox 69.0.3 (64-bit), I get the following after pressing ctrl+end:

out of edit
content info landmark
section

list
with 6 items
list item

link

About

nvdaes · 2019-10-16T13:38:51Z

Just to answer your question:
feerrenrut commented 5 hours ago
@nvdaes Thanks for testing
in browse mode, NVDA reports "paragraph" and "section" too frequently.
Could you confirm whether you see the same behaviour with the latest alpha?

No, this is specific of this PR.

LeonarddeR · 2019-11-05T09:13:10Z

Working on #10462, I realized that braille contains the getBrailleTextForProperties function. Could we consider renaming this to getPropertiesBraille, in line with the rename in the speech package? There doesn't have to change anything else for braille, but I think that renaming one without the other could lead to confusion or at least introduces an inconsistency.

feerrenrut · 2019-11-05T10:13:10Z

Could we consider renaming this to getPropertiesBraille, in line with the rename in the speech package?

I agree, this would be a good idea. Next question is where to do it? I think my preference is to follow this PR with another one specifically to rename it.

LeonarddeR · 2019-11-05T10:19:51Z

I think my preference is to follow this PR with another one

specifically to rename it. Agreed.

LeonarddeR

The behavior of the speech viewer still differs:

Old situation:

Show Speech Viewer on Startup  check box  not checked  Alt+s

New situation:

Show Speech Viewer on Startup
check box
not checked
Alt+s

Turns out that every chunk of speech is written to a new line.

LeonarddeR · 2019-11-05T14:11:56Z

Apart from the speech viewer issue mentioned above, I've been using the branch from source for some time now without issues.

feerrenrut · 2019-11-06T06:42:11Z

Turns out that every chunk of speech is written to a new line.

Initially, my thought was this would be helpful, and it was for debugging purposes. From a usability standpoint, it's probably overall better if sequences would be joined by a space. Though there are certainly cases where people get confused looking at the speechviewer thinking that information is double speaking. I think I would prefer to solve that problem with more formatting in the speechviewer.

I've been using the branch from source for some time now without issues.

Great!

LeonarddeR · 2019-11-06T06:51:38Z

Initially, my thought was this would be helpful, and it was for debugging purposes.

Of course, I don't know how it looks visually. I only use the speech viewer to be able to copy speech sequences (also for debugging purposes), and in that case it's a bit confusing. I'd also separate every sequence with an empty line to make it visible where one sequence ends and the other starts, in that case.

SpeechViewer.appendText renamed to appendSpeechSequence. It takes a SpeechSequence instead of a str. This allows more control over the presentation of speech, which will separate speech items with a space, and conclude with a newline.

feerrenrut · 2019-11-06T09:07:35Z

I'd also separate every sequence with an empty line

I think this will make speech viewer too sparse. Especially considering how often newlines are already in the speech sequence. What I have now should restore the old behaviour, I think these other ideas should be discussed separately.

LeonarddeR

I had another look through this. I think it looks pretty good, just some small comments and thoughts.

LeonarddeR

Github insisted on hiding https://github.com/nvaccess/nvda/pull/10371/files#r342995158 . Doesn't hold me back from approving this, though.

Unless @michaelDCurran wants to review this, I'd like to suggest merging this ASAP for broader testing.

In PR #10371, getSpeechTextForProperties was renamed to getPropertiesSpeech, make the equivalent change for Braille.

LeonarddeR and others added 3 commits October 10, 2019 15:44

Rename getSpeechTextForProperties to getPropertiesSpeech

dcb6145

Unify line endings (use Windows line endings)

cfbf173

feerrenrut commented Oct 10, 2019

View reviewed changes

Comment thread source/textInfos/__init__.py Outdated

feerrenrut commented Oct 10, 2019

View reviewed changes

Comment thread source/speech/__init__.py

LeonarddeR reviewed Oct 10, 2019

View reviewed changes

Comment thread source/NVDAObjects/window/excel.py Outdated

This comment has been minimized.

Sign in to view

LeonarddeR suggested changes Oct 11, 2019

View reviewed changes

feerrenrut added 6 commits October 14, 2019 18:19

Passing None to LangChangeCommand is accepted

86c7df9

Add type hints to help track down issues with sequence building

99086de

Add speechSequence checking

ddd1ecb

Log errors in speech sequences (unknown types, None entries, empty list entries) Must be enabled in the advanced panel, logging options.

Fix several found issues with building speech sequences.

d349bfc

Fix lint errors

49f282f

Other review actions.

eb67825

feerrenrut added 2 commits October 15, 2019 18:27

Remove separator argument from getFormatFieldSpeech method

7a76f02

Include padding added to strings for speech in test

de1343e

In the future it would be better to test speech as structured data. This would allow us to test commands as well.

LeonarddeR reviewed Oct 16, 2019

View reviewed changes

LeonarddeR suggested changes Nov 5, 2019

View reviewed changes

Comment thread source/config/configSpec.py Outdated

Rename "speechSequences" logging category to "speech"

6d8008b

LeonarddeR reviewed Nov 6, 2019

View reviewed changes

feerrenrut added 4 commits November 6, 2019 11:18

Fix wrong "types" import.

8d5ca11

Fix type annotations for getIndentationSpeech

cf8ff2f

Remove unnecessary logBadSequenceTypes wrapper

a0e3ffa

Fix missing EOF newline

adc11f4

LeonarddeR approved these changes Nov 6, 2019

View reviewed changes

LeonarddeR mentioned this pull request Nov 6, 2019

Rename getBrailleTextForProperties to getPropertiesBraille #10469

Merged

feerrenrut merged commit 4b80c41 into master Nov 6, 2019

nvaccessAuto added this to the 2019.3 milestone Nov 6, 2019

feerrenrut added a commit that referenced this pull request Nov 6, 2019

Update changes file for PR #10371

d9c5eaa

feerrenrut pushed a commit that referenced this pull request Nov 6, 2019

Rename getBrailleTextForProperties to getPropertiesBraille (PR #10469)

46fc5eb

In PR #10371, getSpeechTextForProperties was renamed to getPropertiesSpeech, make the equivalent change for Braille.

This was referenced Nov 6, 2019

Store speech priorities in an enum #10471

Merged

speech.getIndentationSpeech: use BeepCommand #10473

Merged

Import languageHandler in speech package #10475

Merged

feerrenrut deleted the i10098-speechCommandsUseSequences branch January 17, 2020 08:56

seanbudd added the deprecated/2021.1 Label used to track deprecations due for removal in the 2021.1 release label Mar 5, 2021

seanbudd mentioned this pull request Mar 5, 2021

Deprecations for 2021.1 #12123

Closed

18 tasks

seanbudd added deprecated/2021.1 Label used to track deprecations due for removal in the 2021.1 release and removed deprecated/2021.1 Label used to track deprecations due for removal in the 2021.1 release labels Mar 5, 2021

seanbudd mentioned this pull request Mar 5, 2021

Deprecate import of Commands directly from speech module in favour of speech.commands #12126

Merged

7 tasks

		@@ -887,21 +959,36 @@ def speakTextInfo(info, useCache=True, formatConfig=None, unit=None, reason=cont
		presCat=field.getPresentationCategory(newControlFieldStack[0:count],formatConfig,reason)
		if not presCat or presCat is field.PRESCAT_LAYOUT:

Uh oh!

Conversation

feerrenrut commented Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing performed:

Known issues with pull request:

Change log entry:

Uh oh!

feerrenrut Oct 10, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

nvdaes commented Oct 10, 2019

Uh oh!

LeonarddeR left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

feerrenrut commented Oct 15, 2019

Uh oh!

feerrenrut commented Oct 15, 2019

Uh oh!

feerrenrut commented Oct 15, 2019

Uh oh!

nvdaes commented Oct 16, 2019

Uh oh!

LeonarddeR left a comment

Choose a reason for hiding this comment

Uh oh!

LeonarddeR Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

feerrenrut Nov 6, 2019

Choose a reason for hiding this comment

Uh oh!

feerrenrut commented Oct 16, 2019

Uh oh!

feerrenrut commented Oct 16, 2019

Uh oh!

LeonarddeR commented Oct 16, 2019 • edited by feerrenrut Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

feerrenrut commented Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

feerrenrut commented Oct 16, 2019

Uh oh!

nvdaes commented Oct 16, 2019

Uh oh!

LeonarddeR commented Nov 5, 2019

Uh oh!

feerrenrut commented Nov 5, 2019

Uh oh!

LeonarddeR commented Nov 5, 2019 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LeonarddeR left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LeonarddeR commented Nov 5, 2019

Uh oh!

feerrenrut commented Nov 6, 2019

Uh oh!

feerrenrut commented Oct 10, 2019 •

edited

Loading

LeonarddeR commented Oct 16, 2019 •

edited by feerrenrut

Loading

feerrenrut commented Oct 16, 2019 •

edited

Loading

LeonarddeR commented Nov 5, 2019 via email •

edited

Loading