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

Speech commands use sequences #10371

Merged
merged 21 commits into from Nov 6, 2019

Conversation

@feerrenrut
Copy link
Contributor

feerrenrut commented Oct 10, 2019

Link to issue number:

#10098

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

leonardder and others added 3 commits Aug 14, 2019
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
out = ""
def getFormatFieldSpeech(
self, attrs, attrsCache=None, formatConfig=None, reason=None, unit=None, extraDetail=False,
initialFormat=False, separator=speech.CHUNK_SEPARATOR

This comment has been minimized.

Copy link
@feerrenrut

feerrenrut Oct 10, 2019

Author Contributor

Param separator can be removed. Anyone disagree with this?

@feerrenrut

This comment was marked as resolved.

Copy link
Contributor Author

feerrenrut commented Oct 10, 2019

One other thing I noticed will doing this. Refer to line 239 in speech.manager

outSeq.append(command)

All other isinstance checks end with continue, why is this one different?
Should there be a default "catch-all" here? Unknown commands? Unaccepted types?

@AppVeyorBot

This comment was marked as outdated.

Copy link

AppVeyorBot commented Oct 10, 2019

PR introduces Flake8 errors 😲

See test results for Failed build of commit bcd59981be

@nvdaes

This comment has been minimized.

Copy link
Contributor

nvdaes commented Oct 10, 2019

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

Copy link
Collaborator

leonardder left a comment

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.

source/appModules/kindle.py Outdated Show resolved Hide resolved
source/aria.py Outdated Show resolved Hide resolved
source/browseMode.py Outdated Show resolved Hide resolved
source/browseMode.py Outdated Show resolved Hide resolved
source/browseMode.py Outdated Show resolved Hide resolved
source/speech/__init__.py Outdated Show resolved Hide resolved
source/textInfos/__init__.py Show resolved Hide resolved
source/textInfos/__init__.py Outdated Show resolved Hide resolved
source/textInfos/__init__.py Outdated Show resolved Hide resolved
source/textInfos/__init__.py Outdated Show resolved Hide resolved
@feerrenrut

This comment has been minimized.

Copy link
Contributor Author

feerrenrut commented Oct 15, 2019

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

This comment has been minimized.

Copy link
Contributor Author

feerrenrut commented Oct 15, 2019

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?

feerrenrut added 2 commits Oct 15, 2019
In the future it would be better to test speech as structured data. This
would allow us to test commands as well.
@feerrenrut

This comment has been minimized.

Copy link
Contributor Author

feerrenrut commented Oct 15, 2019

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

This comment has been minimized.

Copy link
Contributor

nvdaes commented Oct 16, 2019

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.

Copy link
Collaborator

leonardder left a comment

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

@@ -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:

This comment has been minimized.

Copy link
@leonardder

leonardder Oct 16, 2019

Collaborator

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 comment has been minimized.

Copy link
@feerrenrut

feerrenrut Nov 6, 2019

Author Contributor

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

This comment has been minimized.

Copy link
Contributor Author

feerrenrut commented Oct 16, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

feerrenrut commented Oct 16, 2019

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

This comment has been minimized.

Copy link
Collaborator

leonardder commented Oct 16, 2019

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

This comment has been minimized.

Copy link
Contributor Author

feerrenrut commented Oct 16, 2019

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

This comment has been minimized.

Copy link
Contributor Author

feerrenrut commented Oct 16, 2019

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

This comment has been minimized.

Copy link
Contributor

nvdaes commented Oct 16, 2019

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.

@feerrenrut

This comment has been minimized.

Copy link
Contributor Author

feerrenrut commented Nov 4, 2019

I believe I have addressed the issues described. Please try out the build

source/speech/manager.py Outdated Show resolved Hide resolved
@leonardder

This comment has been minimized.

Copy link
Collaborator

leonardder commented Nov 5, 2019

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

This comment has been minimized.

Copy link
Contributor Author

feerrenrut commented Nov 5, 2019

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

This comment has been minimized.

Copy link
Collaborator

leonardder commented Nov 5, 2019

Copy link
Collaborator

leonardder left a comment

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.

source/config/configSpec.py Outdated Show resolved Hide resolved
@leonardder

This comment has been minimized.

Copy link
Collaborator

leonardder commented Nov 5, 2019

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

@feerrenrut

This comment has been minimized.

Copy link
Contributor Author

feerrenrut commented Nov 6, 2019

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

This comment has been minimized.

Copy link
Collaborator

leonardder commented Nov 6, 2019

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

This comment has been minimized.

Copy link
Contributor Author

feerrenrut commented Nov 6, 2019

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.

Copy link
Collaborator

leonardder left a comment

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

source/mathPres/__init__.py Show resolved Hide resolved
source/speech/__init__.py Show resolved Hide resolved
source/speech/__init__.py Outdated Show resolved Hide resolved
source/speech/__init__.py Outdated Show resolved Hide resolved
source/speech/__init__.py Show resolved Hide resolved
source/speech/__init__.py Show resolved Hide resolved
source/speech/__init__.py Show resolved Hide resolved
source/speech/__init__.py Show resolved Hide resolved
source/textInfos/__init__.py Show resolved Hide resolved
tests/system/NVDA Core/variables.py Outdated Show resolved Hide resolved
Copy link
Collaborator

leonardder left a comment

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.

@feerrenrut feerrenrut merged commit 4b80c41 into master Nov 6, 2019
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@nvaccessAuto nvaccessAuto added this to the 2019.3 milestone Nov 6, 2019
feerrenrut added a commit that referenced this pull request Nov 6, 2019
feerrenrut added a commit that referenced this pull request Nov 6, 2019
In PR #10371, getSpeechTextForProperties was renamed to getPropertiesSpeech, make the equivalent change for Braille.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.