Skip to content

Use enum for speech reason #10703

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.

Sign up for GitHub

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

Merged
feerrenrut merged 3 commits into from
Jan 30, 2020
Merged

Use enum for speech reason #10703

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1982432
Use enum instead of strings.
feerrenrut Dec 10, 2019
91cb815
Modified reason annotations to use enum class
feerrenrut Dec 10, 2019
aceffab
fix lint warnings
feerrenrut Jan 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion source/appModules/kindle.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import api
from scriptHandler import willSayAllResume, isScriptWaiting
import controlTypes
from controlTypes import OutputReason
import treeInterceptorHandler
from cursorManager import ReviewCursorManager
import browseMode
Expand Down Expand Up @@ -277,7 +278,7 @@ def getFormatFieldSpeech(
attrs: textInfos.Field,
attrsCache: Optional[textInfos.Field] = None,
formatConfig: Optional[Dict[str, bool]] = None,
reason: Optional[str] = None,
reason: Optional[OutputReason] = None,
unit: Optional[str] = None,
extraDetail: bool = False,
initialFormat: bool = False
Expand Down
5 changes: 3 additions & 2 deletions source/browseMode.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
from scriptHandler import isScriptWaiting, willSayAllResume
import aria
import controlTypes
from controlTypes import OutputReason
import config
import textInfos
import braille
Expand All @@ -39,7 +40,7 @@
from abc import ABCMeta, abstractmethod
from typing import Optional

REASON_QUICKNAV = "quickNav"
REASON_QUICKNAV = OutputReason.QUICKNAV

def reportPassThrough(treeInterceptor,onlyIfChanged=True):
"""Reports the pass through mode if it has changed.
Expand Down Expand Up @@ -304,7 +305,7 @@ def event_treeInterceptor_gainFocus(self):
controlTypes.ROLE_CHECKMENUITEM,
})

def shouldPassThrough(self, obj, reason=None):
def shouldPassThrough(self, obj, reason: Optional[OutputReason] = None):
"""Determine whether pass through mode should be enabled (focus mode) or disabled (browse mode) for a given object.
@param obj: The object in question.
@type obj: L{NVDAObjects.NVDAObject}
Expand Down
70 changes: 44 additions & 26 deletions source/controlTypes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#See the file COPYING for more details.
#Copyright (C) 2007-2016 NV Access Limited, Babbage B.V.
from typing import Dict, Union, Set, Any, Optional, List
from enum import Enum, auto

ROLE_UNKNOWN=0
ROLE_WINDOW=1
Expand Down Expand Up @@ -622,27 +623,44 @@
ROLE_APPLICATION,
}

#{ Output reasons
# These constants are used to specify the reason that a given piece of output was generated.
#: An object to be reported due to a focus change or similar.
REASON_FOCUS="focus"
#: An ancestor of the focus object to be reported due to a focus change or similar.
REASON_FOCUSENTERED="focusEntered"
#: An item under the mouse.
REASON_MOUSE="mouse"
#: A response to a user query.
REASON_QUERY="query"
#: Reporting a change to an object.
REASON_CHANGE="change"
#: A generic, screen reader specific message.
REASON_MESSAGE="message"
#: Text reported as part of a say all.
REASON_SAYALL="sayAll"
#: Content reported due to caret movement or similar.
REASON_CARET="caret"
#: No output, but any state should be cached as if output had occurred.
REASON_ONLYCACHE="onlyCache"
#}

class OutputReason(Enum):
"""Specify the reason that a given piece of output was generated.
"""
#: An object to be reported due to a focus change or similar.
FOCUS = auto()
#: An ancestor of the focus object to be reported due to a focus change or similar.
FOCUSENTERED = auto()
#: An item under the mouse.
MOUSE = auto()
#: A response to a user query.
QUERY = auto()
#: Reporting a change to an object.
CHANGE = auto()
#: A generic, screen reader specific message.
MESSAGE = auto()
#: Text reported as part of a say all.
SAYALL = auto()
#: Content reported due to caret movement or similar.
CARET = auto()
#: No output, but any state should be cached as if output had occurred.
ONLYCACHE = auto()

QUICKNAV = auto()

# The following constants are kept for backwards compatibility.
# In future, OutputReason should be used directly


REASON_FOCUS = OutputReason.FOCUS

@michaelDCurran michaelDCurran Jan 30, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As there are several other places we should use enums in future (textInfos.position, textInfos.unit, controlTypes.role, controlTypes.state), I'd like to see the exploding of the enum values into the outer scope here be more generic rather than explicitly listing each one, as it would be very easy to forget one.
Something like:

for r in OutputReason:
	globals()['REASON_%s' % r.name] = r

@feerrenrut feerrenrut Jan 30, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I don't like about doing this, is that it means new constants also become available automatically at this scope. Forgetting to update this "exploded enum" when adding new constants is a feature in my mind. I agree it means when creating the enum and exploded values we have to be careful to double check that they are all available, but just like the quickNav value it is easy to miss one because it is defined in another file. I want to deprecate the constants at this scope and remove them in 2021.1

@michaelDCurran michaelDCurran Jan 30, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. I'm not sure how I'd feel about a new value being added to the enum in the future and not being then duplicated at module level, unless we go through the code and remove all model-level references to the values. I think defining some differently is extremely confusing. I certainly agree that values for new enums should never be duplicated, but pre-existing ones should either be duplicated and or all module-level references removed.
However, I'll approve this as is, and you can make the final decision.

@feerrenrut feerrenrut Jan 30, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this stemmed from the speech function refactor I only did "the minimal". But I think we should follow up with another PR to do the same for the rest of the constants in control types, and ensure that all of NVDA uses the enum versions and officially deprecate (and give a date of removal for) the module level constants.

@feerrenrut feerrenrut Jan 30, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have created #10732 to do this follow up work.

@feerrenrut feerrenrut Jan 30, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have also created a new label deprecated/2021.1 to keep track of deprecations and their removal date. This should make it easier to give a concrete set of "breaking changes" in the 2021.1 release.

REASON_FOCUSENTERED = OutputReason.FOCUSENTERED
REASON_MOUSE = OutputReason.MOUSE
REASON_QUERY = OutputReason.QUERY
REASON_CHANGE = OutputReason.CHANGE
REASON_MESSAGE = OutputReason.MESSAGE
REASON_SAYALL = OutputReason.SAYALL
REASON_CARET = OutputReason.CARET
REASON_ONLYCACHE = OutputReason.ONLYCACHE

#: Text to use for 'current' values. These describe if an item is the current item
#: within a particular kind of selection.
Expand All @@ -661,15 +679,15 @@
"time":_("current time"),
}

def processPositiveStates(role, states, reason, positiveStates=None):

def processPositiveStates(role, states, reason: OutputReason, positiveStates=None):
"""Processes the states for an object and returns the positive states to output for a specified reason.
For example, if C{STATE_CHECKED} is in the returned states, it means that the processed object is checked.
@param role: The role of the object to process states for (e.g. C{ROLE_CHECKBOX}.
@type role: int
@param states: The raw states for an object to process.
@type states: set
@param reason: The reason to process the states (e.g. C{REASON_FOCUS}.
@type reason: str
@param positiveStates: Used for C{REASON_CHANGE}, specifies states changed from negative to positive;
@type positiveStates: set
@return: The processed positive states.
Expand Down Expand Up @@ -719,15 +737,15 @@ def processPositiveStates(role, states, reason, positiveStates=None):
positiveStates.discard(STATE_EDITABLE)
return positiveStates

def processNegativeStates(role, states, reason, negativeStates=None):

def processNegativeStates(role, states, reason: OutputReason, negativeStates=None):
"""Processes the states for an object and returns the negative states to output for a specified reason.
For example, if C{STATE_CHECKED} is in the returned states, it means that the processed object is not checked.
@param role: The role of the object to process states for (e.g. C{ROLE_CHECKBOX}.
@type role: int
@param states: The raw states for an object to process.
@type states: set
@param reason: The reason to process the states (e.g. C{REASON_FOCUS}.
@type reason: str
@param negativeStates: Used for C{REASON_CHANGE}, specifies states changed from positive to negative;
@type negativeStates: set
@return: The processed negative states.
Expand Down Expand Up @@ -787,7 +805,7 @@ def processNegativeStates(role, states, reason, negativeStates=None):
def processAndLabelStates(
role: int,
states: Set[Any],
reason: str,
reason: OutputReason,
positiveStates: Optional[Set[Any]] = None,
negativeStates: Optional[Set[Any]] = None,
positiveStateLabelDict: Dict[int, str] = {},
Expand Down
23 changes: 12 additions & 11 deletions source/speech/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import colors
import api
import controlTypes
from controlTypes import OutputReason
import tones
import synthDriverHandler
from synthDriverHandler import getSynth, setSynth
Expand Down Expand Up @@ -302,7 +303,7 @@ def getCharDescListFromText(text,locale):

def speakObjectProperties( # noqa: C901
obj,
reason: str = controlTypes.REASON_QUERY,
reason: OutputReason = controlTypes.REASON_QUERY,
_prefixSpeechCommand: Optional[SpeechCommand] = None,
priority: Optional[Spri] = None,
**allowedProperties
Expand All @@ -322,7 +323,7 @@ def speakObjectProperties( # noqa: C901
# and move logic out into smaller helper functions.
def getObjectPropertiesSpeech( # noqa: C901
obj,
reason: str = controlTypes.REASON_QUERY,
reason: OutputReason = controlTypes.REASON_QUERY,
_prefixSpeechCommand: Optional[SpeechCommand] = None,
**allowedProperties
) -> SpeechSequence:
Expand Down Expand Up @@ -423,7 +424,7 @@ def getObjectPropertiesSpeech( # noqa: C901

def _getPlaceholderSpeechIfTextEmpty(
obj,
reason: str,
reason: OutputReason,
) -> Tuple[bool, SpeechSequence]:
""" Attempt to get speech for placeholder attribute if text for 'obj' is empty. Don't report the placeholder
value unless the text is empty, because it is confusing to hear the current value (presumably typed by the
Expand All @@ -439,7 +440,7 @@ def _getPlaceholderSpeechIfTextEmpty(

def speakObject(
obj,
reason: str = controlTypes.REASON_QUERY,
reason: OutputReason = controlTypes.REASON_QUERY,
_prefixSpeechCommand: Optional[SpeechCommand] = None,
priority: Optional[Spri] = None
):
Expand All @@ -461,7 +462,7 @@ def _flattenNestedSequences(nestedSequences: Iterator[SpeechSequence]) -> Iterat
# and move logic out into smaller helper functions.
def getObjectSpeech( # noqa: C901
obj,
reason: str = controlTypes.REASON_QUERY,
reason: OutputReason = controlTypes.REASON_QUERY,
_prefixSpeechCommand: Optional[SpeechCommand] = None,
):
from NVDAObjects import NVDAObjectTextInfo
Expand Down Expand Up @@ -598,7 +599,7 @@ def _objectSpeech_calculateAllowedProps(reason, shouldReportTextContent):

def speakText(
text: str,
reason: str = controlTypes.REASON_MESSAGE,
reason: OutputReason = controlTypes.REASON_MESSAGE,
symbolLevel: Optional[int] = None,
priority: Optional[Spri] = None
):
Expand Down Expand Up @@ -1044,7 +1045,7 @@ def speakTextInfo(
useCache: Union[bool, SpeakTextInfoState] = True,
formatConfig: Dict[str, bool] = None,
unit: Optional[str] = None,
reason: str = controlTypes.REASON_QUERY,
reason: OutputReason = controlTypes.REASON_QUERY,
_prefixSpeechCommand: Optional[SpeechCommand] = None,
onlyInitialFields: bool = False,
suppressBlanks: bool = False,
Expand Down Expand Up @@ -1074,7 +1075,7 @@ def getTextInfoSpeech( # noqa: C901
useCache: Union[bool, SpeakTextInfoState] = True,
formatConfig: Dict[str, bool] = None,
unit: Optional[str] = None,
reason: str = controlTypes.REASON_QUERY,
reason: OutputReason = controlTypes.REASON_QUERY,
_prefixSpeechCommand: Optional[SpeechCommand] = None,
onlyInitialFields: bool = False,
suppressBlanks: bool = False
Expand Down Expand Up @@ -1444,7 +1445,7 @@ def isControlEndFieldCommand(x):
# Note: when working on getPropertiesSpeech, look for opportunities to simplify
# and move logic out into smaller helper functions.
def getPropertiesSpeech( # noqa: C901
reason: str = controlTypes.REASON_QUERY,
reason: OutputReason = controlTypes.REASON_QUERY,
**propertyValues
) -> SpeechSequence:
global oldTreeLevel, oldTableID, oldRowNumber, oldRowSpan, oldColumnNumber, oldColumnSpan
Expand Down Expand Up @@ -1629,7 +1630,7 @@ def getControlFieldSpeech( # noqa: C901
fieldType: str,
formatConfig: Optional[Dict[str, bool]] = None,
extraDetail: bool = False,
reason: Optional[str] = None
reason: Optional[OutputReason] = None
) -> SpeechSequence:
if attrs.get('isHidden'):
return []
Expand Down Expand Up @@ -1894,7 +1895,7 @@ def getFormatFieldSpeech( # noqa: C901
attrs: textInfos.Field,
attrsCache: Optional[textInfos.Field] = None,
formatConfig: Optional[Dict[str, bool]] = None,
reason: Optional[str] = None,
reason: Optional[OutputReason] = None,
unit: Optional[str] = None,
extraDetail: bool = False,
initialFormat: bool = False,
Expand Down
5 changes: 3 additions & 2 deletions source/textInfos/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import baseObject
import config
import controlTypes
from controlTypes import OutputReason
import locationHelper


Expand Down Expand Up @@ -521,7 +522,7 @@ def getControlFieldSpeech(
fieldType: str,
formatConfig: Optional[Dict[str, bool]] = None,
extraDetail: bool = False,
reason: Optional[str] = None
reason: Optional[OutputReason] = None
) -> SpeechSequence:
# Import late to avoid circular import.
import speech
Expand All @@ -541,7 +542,7 @@ def getFormatFieldSpeech(
attrs: Field,
attrsCache: Optional[Field] = None,
formatConfig: Optional[Dict[str, bool]] = None,
reason: Optional[str] = None,
reason: Optional[OutputReason] = None,
unit: Optional[str] = None,
extraDetail: bool = False,
initialFormat: bool = False,
Expand Down
3 changes: 2 additions & 1 deletion source/treeInterceptorHandler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import braille
import vision
from speech.types import SpeechSequence
from controlTypes import OutputReason

runningTable=set()

Expand Down Expand Up @@ -246,7 +247,7 @@ def getFormatFieldSpeech(
attrs: textInfos.Field,
attrsCache: Optional[textInfos.Field] = None,
formatConfig: Optional[Dict[str, bool]] = None,
reason: Optional[str] = None,
reason: Optional[OutputReason] = None,
unit: Optional[str] = None,
extraDetail: bool = False,
initialFormat: bool = False,
Expand Down