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

Fix nvda find title cancellation #11632

Merged
merged 13 commits into from
Oct 8, 2020
Merged
3 changes: 2 additions & 1 deletion source/NVDAObjects/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,8 @@ def objectWithFocus():

@staticmethod
def objectInForeground():
"""Retrieves the object representing the current foreground control according to the Operating System. This differes from NVDA's foreground object as this object is the real foreground object according to the Operating System, not according to NVDA.
"""Retrieves the object representing the current foreground control according to the
Operating System. This may differ from NVDA's cached foreground object.
@return: the foreground object
@rtype: L{NVDAObject}
"""
Expand Down
12 changes: 11 additions & 1 deletion source/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,23 @@ def getFocusObject():

def getForegroundObject():
"""Gets the current foreground object.
This (cached) object is the (effective) top-level "window" (hwnd).
EG a Dialog rather than the focused control within the dialog.
The cache is updated as queued events are processed, as such there will be a delay between the winEvent
and this function matching. However, within NVDA this should be used in order to be in sync with other
functions such as "getFocusAncestors".
@returns: the current foreground object
@rtype: L{NVDAObjects.NVDAObject}
"""
return globalVars.foregroundObject

def setForegroundObject(obj):
"""Stores the given object as the current foreground object. (Note: it does not physically change the operating system foreground window, but only allows NVDA to keep track of what it is).
"""Stores the given object as the current foreground object.
Note: does not cause the operating system to change the foreground window,
but simply allows NVDA to keep track of what the foreground window is.
Alternative names for this function may have been:
- setLastForegroundWindow
- setLastForegroundEventObject
@param obj: the object that will be stored as the current foreground object
@type obj: NVDAObjects.NVDAObject
"""
Expand Down
99 changes: 62 additions & 37 deletions source/eventHandler.py
Original file line number Diff line number Diff line change
Expand Up @@ -152,11 +152,62 @@ def _trackFocusObject(eventName, obj) -> None:

if eventName == "gainFocus":
lastQueuedFocusObject = obj
setattr(obj, WAS_GAIN_FOCUS_OBJ_ATTR_NAME, True)
if speech.manager._shouldDoSpeechManagerLogging():
log.debug(f"Changing last queued focus object: {obj!r}")
elif not hasattr(obj, WAS_GAIN_FOCUS_OBJ_ATTR_NAME):
setattr(obj, WAS_GAIN_FOCUS_OBJ_ATTR_NAME, False)


class FocusLossCancellableSpeechCommand(_CancellableSpeechCommand):
def __init__(self, obj, reportDevInfo: bool):
from NVDAObjects import NVDAObject
if not isinstance(obj, NVDAObject):
log.warning("Unhandled object type. Expected all objects to be descendant from NVDAObject")
raise TypeError(f"Unhandled object type: {obj!r}")
self._obj = obj
super(FocusLossCancellableSpeechCommand, self).__init__(reportDevInfo=reportDevInfo)

if self.isLastFocusObj():
# Objects may be re-used.
# WAS_GAIN_FOCUS_OBJ_ATTR_NAME state should be cleared at some point?
# perhaps instead keep a weak ref list of obj that had focus, clear on keypress?

# Assumption: we only process one focus event at a time, so even if several focus events are queued,
# all focused objects will still gain this tracking attribute. Otherwise, this may need to be set via
# api.setFocusObject when globalVars.focusObject is set.
setattr(obj, WAS_GAIN_FOCUS_OBJ_ATTR_NAME, True)
elif not hasattr(obj, WAS_GAIN_FOCUS_OBJ_ATTR_NAME):
setattr(obj, WAS_GAIN_FOCUS_OBJ_ATTR_NAME, False)

def _checkIfValid(self):
stillValid = (
self.isLastFocusObj()
or not self.previouslyHadFocus()
or self.isAncestorOfCurrentFocus()
# Ensure titles for dialogs gaining focus are reported, EG NVDA Find dialog
or self.isForegroundObject()
Copy link
Member

Choose a reason for hiding this comment

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

Is this still necessary after the change from lastQueuedFocusObject to api.getFocusObject()? I wouldn't have thought so, unless the bug is different to what we hypothesized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your are right, checking for self.isForegroundObject() is not required to fix this particular bug, and I have confirmed that. However, it doesn't harm the core use case of cancellable speech (rapid focus changes in Gmail), and I can't think of a reason why we would want to cancel speech for the foreground object if it wasn't the focus ancestor for some reason.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I think it is redundant and should be removed, as getforegroundObject always returns a focus ancestor, which is checked already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I follow this. First, I want to confirm you want the self.isForgroundObject() check removed?

This may not be set by the focus ancestor, usually it is from api.getDesktopObject().objectInForeground(), see the following code from doPreGainFocus

		newForeground=api.getDesktopObject().objectInForeground()
		if not newForeground:
			log.debugWarning("Can not get real foreground, resorting to focus ancestors")
			ancestors=api.getFocusAncestors()
			if len(ancestors)>1:
				newForeground=ancestors[1]
			else:
				newForeground=obj
		api.setForegroundObject(newForeground)

)
return stillValid

def _getDevInfo(self):
return (
f"isLast: {self.isLastFocusObj()}"
f", previouslyHad: {self.previouslyHadFocus()}"
f", isAncestorOfCurrentFocus: {self.isAncestorOfCurrentFocus()}"
f", is foreground obj {self.isForegroundObject()}"
)

def isLastFocusObj(self):
# Use '==' rather than 'is' because obj may have been created multiple times
# pointing to the same underlying object.
return self._obj == api.getFocusObject()

def previouslyHadFocus(self):
return getattr(self._obj, WAS_GAIN_FOCUS_OBJ_ATTR_NAME, False)

def isAncestorOfCurrentFocus(self):
return self._obj in api.getFocusAncestors()

def isForegroundObject(self):
foreground = api.getForegroundObject()
return self._obj is foreground or self._obj == foreground


def _getFocusLossCancellableSpeechCommand(
obj,
Expand All @@ -169,34 +220,9 @@ def _getFocusLossCancellableSpeechCommand(
log.warning("Unhandled object type. Expected all objects to be descendant from NVDAObject")
return None

def previouslyHadFocus():
return getattr(obj, WAS_GAIN_FOCUS_OBJ_ATTR_NAME, False)

def isLastFocusObj():
# Obj may have been created multiple times pointing to the same underlying object.
return obj is lastQueuedFocusObject or obj == lastQueuedFocusObject

def isAncestorOfCurrentFocus():
return obj in api.getFocusAncestors()
shouldReportDevInfo = speech.manager._shouldDoSpeechManagerLogging()
return FocusLossCancellableSpeechCommand(obj, reportDevInfo=shouldReportDevInfo)

def isSpeechStillValid():
stillValid = (
isLastFocusObj()
or not previouslyHadFocus()
or isAncestorOfCurrentFocus()
)
return stillValid

if not speech.manager._shouldDoSpeechManagerLogging():
return _CancellableSpeechCommand(isSpeechStillValid)

def getDevInfo():
return (
f"isLast: {isLastFocusObj()}"
f", previouslyHad: {previouslyHadFocus()}"
f", isAncestorOfCurrentFocus: {isAncestorOfCurrentFocus()}"
)
return _CancellableSpeechCommand(isSpeechStillValid, getDevInfo)

def executeEvent(eventName, obj, **kwargs):
"""Executes an NVDA event.
Expand Down Expand Up @@ -232,13 +258,12 @@ def doPreGainFocus(obj,sleepMode=False):
# ask speechManager to check if any of it's queued utterances should be cancelled
# Note: Removing cancelled speech commands should happen after all dependencies for the isValid check
# have been updated:
# - lastQueuedFocusObject
# - obj.WAS_GAIN_FOCUS_OBJ_ATTR_NAME
# - api.setFocusObject()
# - api.getFocusAncestors()
# These are updated:
# - lastQueuedFocusObject & obj.WAS_GAIN_FOCUS_OBJ_ATTR_NAME
# - Set in stack: _trackFocusObject, eventHandler.queueEvent
# - Which results in executeEvent being called, then doPreGainFocus
# When these are updated:
# - obj.WAS_GAIN_FOCUS_OBJ_ATTR_NAME
# - Set during creation of the _CancellableSpeechCommand.
# - api.getFocusAncestors() via api.setFocusObject() called in doPreGainFocus
speech._manager.removeCancelledSpeechCommands()

Expand Down
2 changes: 1 addition & 1 deletion source/globalVars.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
#This file is covered by the GNU General Public License.
#See the file COPYING for more details.
"""global variables module
@var foregroundObject: holds the current foreground object
@var foregroundObject: holds the current foreground object. The object for the last foreground event received.
@type foregroundObject: L{NVDAObjects.NVDAObject}
@var focusObject: holds the current focus object
@type focusObject: L{NVDAObjects.NVDAObject}
Expand Down
35 changes: 17 additions & 18 deletions source/speech/commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,18 +33,22 @@ class _CancellableSpeechCommand(SpeechCommand):

def __init__(
self,
checkIfValid: Optional[Callable[[], bool]] = None,
getDevInfo: Optional[Callable[[], str]] = None,
reportDevInfo=False
):
"""
@param checkIfValid: Callable that returns True if the utterance is still valid.
@param getDevInfo: An optional used to supply more information when __repr__ is called.
@param reportDevInfo: If true, developer info is reported for repr implementation.
"""
self._isCancelled = False
if checkIfValid:
self._checkIfValid = checkIfValid
self._getCheckIfValidDevInfo = getDevInfo
self._utteranceIndex = None
self._reportDevInfo = reportDevInfo

@abstractmethod
def _checkIfValid(self):
raise NotImplementedError()

@abstractmethod
def _getDevInfo(self):
raise NotImplementedError()

def _checkIfCancelled(self):
if self._isCancelled:
Expand All @@ -60,26 +64,21 @@ def isCancelled(self):
def cancelUtterance(self):
self._isCancelled = True

@staticmethod
def _checkIfValid() -> bool:
"""Overridable behavior."""
return True
def _getFormattedDevInfo(self):

def _getDevInfo(self):
isValidCallbackDevInfo = "" if not self._getCheckIfValidDevInfo else self._getCheckIfValidDevInfo()
return (
f"devInfo("
return "" if not self._reportDevInfo else (
f", devInfo<"
f" isCanceledCache: {self._isCancelled}"
f", isValidCallback: {self._checkIfValid()}"
f", isValidCallbackDevInfo: {isValidCallbackDevInfo}"
f", isValidCallbackDevInfo: {self._getDevInfo()} >"
)

def __repr__(self):
return (
f"CancellableSpeech ("
f"{ 'cancelled' if self._checkIfCancelled() else 'still valid' }"
f" {self._getDevInfo()}"
f" )"
f"{self._getFormattedDevInfo()}"
f")"
)


Expand Down
41 changes: 33 additions & 8 deletions tests/unit/test_speechManager/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
"""Unit tests for speech/manager module
"""
import unittest
from typing import Optional, Callable

import config
import speech
from unittest import mock
Expand Down Expand Up @@ -152,6 +154,29 @@ def test_expectedProsodyNotMatching(self):
self.assertNotEqual(p, e)


class _CancellableSpeechCommand_withLamda(_CancellableSpeechCommand):
"""
A test helper to control when speech gets cancelled.
"""

def _getDevInfo(self):
return ""

def _checkIfValid(self):
return True

def __init__(
self,
checkIfValid: Optional[Callable[[], bool]] = None,
getDevInfo: Optional[Callable[[], str]] = None,
):
if checkIfValid is not None:
self._checkIfValid = checkIfValid
if getDevInfo is not None:
self._getDevInfo = getDevInfo
super().__init__(reportDevInfo=True)


class CancellableSpeechTests(unittest.TestCase):
"""Tests behaviour related to CancellableSpeech"""

Expand All @@ -166,7 +191,7 @@ def test_validSpeechSpoken(self):

with smi.expectation():
text = "This stays valid"
smi.speak([text, _CancellableSpeechCommand(lambda: True)])
smi.speak([text, _CancellableSpeechCommand_withLamda(lambda: True)])
smi.expect_synthSpeak(sequence=[text, smi.create_ExpectedIndex(expectedToBecomeIndex=1)])

def test_invalidSpeechNotSpoken(self):
Expand All @@ -175,7 +200,7 @@ def test_invalidSpeechNotSpoken(self):

with smi.expectation():
text = "This stays invalid"
smi.speak([text, _CancellableSpeechCommand(lambda: False)])
smi.speak([text, _CancellableSpeechCommand_withLamda(lambda: False)])

def test_invalidated_indexHit(self):
"""Hitting an index should cause a cancellation of speech that has become
Expand All @@ -195,7 +220,7 @@ def _checkIfValid(speechNumber):
smi.create_CallBackCommand(expectedToBecomeIndex=1),
"text 2",
smi.create_ExpectedIndex(expectedToBecomeIndex=2),
_CancellableSpeechCommand(lambda: _checkIfValid(1)),
_CancellableSpeechCommand_withLamda(lambda: _checkIfValid(1)),
]
isSpeechNumberValid[1] = True # initially valid
smi.speak(initiallyValidSequence)
Expand Down Expand Up @@ -226,7 +251,7 @@ def _checkIfValid(speechNumber):
smi.create_CallBackCommand(expectedToBecomeIndex=1),
"text 2",
smi.create_ExpectedIndex(expectedToBecomeIndex=2),
_CancellableSpeechCommand(lambda: _checkIfValid(1)),
_CancellableSpeechCommand_withLamda(lambda: _checkIfValid(1)),
]
isSpeechNumberValid[1] = True # initially valid
smi.speak(initiallyValidSequence)
Expand Down Expand Up @@ -263,7 +288,7 @@ def _checkIfValid(speechNumber):
smi.create_CallBackCommand(expectedToBecomeIndex=1),
"text 2",
smi.create_ExpectedIndex(expectedToBecomeIndex=2),
_CancellableSpeechCommand(lambda: _checkIfValid(1)),
_CancellableSpeechCommand_withLamda(lambda: _checkIfValid(1)),
]
isSpeechNumberValid[1] = True # initially valid
smi.speak(initiallyValidSequence)
Expand All @@ -277,7 +302,7 @@ def _checkIfValid(speechNumber):
smi.create_CallBackCommand(expectedToBecomeIndex=3),
"text 4",
smi.create_ExpectedIndex(expectedToBecomeIndex=4),
_CancellableSpeechCommand(lambda: _checkIfValid(2)),
_CancellableSpeechCommand_withLamda(lambda: _checkIfValid(2)),
]
smi.speak(newValidSequence)
smi.expect_synthCancel()
Expand All @@ -292,12 +317,12 @@ def test_validSpeechAfterInvalid(self):
with smi.expectation():
smi.speak([
"Stays invalid",
_CancellableSpeechCommand(lambda: False),
_CancellableSpeechCommand_withLamda(lambda: False),
smi.create_ExpectedIndex(expectedToBecomeIndex=1)
])

with smi.expectation():
smi.speak(["Stays valid", _CancellableSpeechCommand(lambda: True)])
smi.speak(["Stays valid", _CancellableSpeechCommand_withLamda(lambda: True)])
smi.expect_synthSpeak(sequence=[
"Stays valid", smi.create_ExpectedIndex(expectedToBecomeIndex=2)
])
Expand Down
1 change: 1 addition & 0 deletions user_docs/en/changes.t2t
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ What's New in NVDA
- When using the SAPI5 Ivona voices from harposoftware.com, NvDA is now able to save configuration, switch synthesizers, and no longer will stay silent after restarting. (#11650)
- It is now possible to enter number 6 in computer braille from a braille keyboard on HIMS displays. (#11710)
- Major performance improvements in Azure Data Studio. (#11533, #11715)
- With "Attempt to Cancel speech for expired focus events" enabled the title of the NVDA Find dialog is announced again. (#11632)


== Changes for Developers ==
Expand Down