Add support for custom providers for braille display auto detection (#…

…14531)

Related to #3564, #2315

Summary of the issue:
Currently, USB and Bluetooth devices are supported for braille display detection. Other devices using other protocols or software devices aren't supported. This pr intends to add support for this.

Description of user facing changes
None. User shouldn't experience anything different for now.

Description of development approach
Added Chain, a new extension point type that allows to register handlers that return iterables (mainly generators). Calling iter on the Chain returns a generator that iterates over all the handlers.
The braille display detector now relies on a new Chain. By default, functions are registered to the chain that yield usb and bluetooth devices. A custom provider can yield own driver names and device matches that are supported by that particular driver. A potential use case would be implementing automatic detection for displays using BRLTTY, for example. It will also be used to fix Braille does not work on secure Windows screens while normal copy of NVDA is running #2315 (see below)
Added a moveToEnd method on HandlerRegistrar, which allows changing the order of registered handlers. This allows add-ons to give priority to their handlers, which is especially helpful for both Chain and Filter. NVDA Remote should come for the braille viewer, otherwise controlling a system with braille viewer on with a 80 cell display connected to the controller would lower the display size to 40 unnecessarily. This will also be used to register a custom handler to bdDetect.scanForDevices to support auto detection of the user display on the secure screen instance of NVDA, which should come before USB and Bluetooth.
As a bonus, added type hints to extension points. For Filters and Chains, you could provide the value type and then a type checker can check validity.
As another bonus, all features are covered by new tests. So there are tests for the Chain extension point and for the specific use case in bdDetect
Testing strategy:
As this touches braille display auto detection quite a lot, probably merge this early in the 2023.2 cycle.

Known issues with pull request:
bdDetect.Detector does no longer take constructor parameters, rather queueBgScan should be called explicitly. This is because if we queue a scan in the constructor of Detector, the detector could switch to a display and disable detection before the _detector was set on the braille handler. Ideally we should use a lock as well, but that might be something as a follow up for both this pr and #14524. Note that though we're changing the constructor of a public class in bdDetect, the doc string of the class explicitly states that the Detector class should be used by the braille module only. That should be enough warning for users not to use this class and therefore I don't consider this API breaking.

source/braille.py

@@ -2565,7 +2565,8 @@ def _enableDetection(
 			self._detector.rescan(usb=usb, bluetooth=bluetooth, limitToDevices=limitToDevices)
 			return
 		config.conf["braille"]["display"] = AUTO_DISPLAY_NAME
-		self._detector = bdDetect.Detector(usb=usb, bluetooth=bluetooth, limitToDevices=limitToDevices)
+		self._detector = bdDetect._Detector()
+		self._detector._queueBgScan(usb=usb, bluetooth=bluetooth, limitToDevices=limitToDevices)
 
 	def _disableDetection(self):
 		"""Disables automatic detection of braille displays."""
@@ -2640,7 +2641,6 @@ def initialize():
 	newTableName = brailleTables.RENAMED_TABLES.get(oldTableName)
 	if newTableName:
 		config.conf["braille"]["translationTable"] = newTableName
-	bdDetect.initializeDetectionData()
 	handler = BrailleHandler()
 	# #7459: the syncBraille has been dropped in favor of the native hims driver.
 	# Migrate to renamed drivers as smoothly as possible.

source/core.py

@@ -212,6 +212,7 @@ def resetConfiguration(factoryDefaults=False):
 	import speech
 	import vision
 	import inputCore
+	import bdDetect
 	import hwIo
 	import tones
 	log.debug("Terminating vision")
@@ -224,6 +225,8 @@ def resetConfiguration(factoryDefaults=False):
 	speech.terminate()
 	log.debug("terminating tones")
 	tones.terminate()
+	log.debug("Terminating background braille display detection")
+	bdDetect.terminate()
 	log.debug("Terminating background i/o")
 	hwIo.terminate()
 	log.debug("terminating addonHandler")
@@ -243,6 +246,8 @@ def resetConfiguration(factoryDefaults=False):
 	# Hardware background i/o
 	log.debug("initializing background i/o")
 	hwIo.initialize()
+	log.debug("Initializing background braille display detection")
+	bdDetect.initialize()
 	# Tones
 	tones.initialize()
 	#Speech
@@ -521,6 +526,9 @@ def main():
 	log.debug("initializing background i/o")
 	import hwIo
 	hwIo.initialize()
+	log.debug("Initializing background braille display detection")
+	import bdDetect
+	bdDetect.initialize()
 	log.debug("Initializing tones")
 	import tones
 	tones.initialize()
@@ -791,6 +799,7 @@ def _doPostNvdaStartupAction():
 	_terminate(brailleInput)
 	_terminate(braille)
 	_terminate(speech)
+	_terminate(bdDetect)
 	_terminate(hwIo)
 	_terminate(addonHandler)
 	_terminate(garbageHandler)

source/extensionPoints/__init__.py

@@ -1,5 +1,5 @@
 # A part of NonVisual Desktop Access (NVDA)
-# Copyright (C) 2017-2021 NV Access Limited, Joseph Lee, Łukasz Golonka, Leonard de Ruijter
+# Copyright (C) 2017-2023 NV Access Limited, Joseph Lee, Łukasz Golonka, Leonard de Ruijter
 # This file is covered by the GNU General Public License.
 # See the file COPYING for more details.
 
@@ -13,13 +13,17 @@
 from logHandler import log
 from .util import HandlerRegistrar, callWithSupportedKwargs, BoundMethodWeakref
 from typing import (
+	Callable,
+	Generator,
 	Generic,
+	Iterable,
 	Set,
 	TypeVar,
+	Union,
 )
 
 
-class Action(HandlerRegistrar):
+class Action(HandlerRegistrar[Callable[..., None]]):
 	"""Allows interested parties to register to be notified when some action occurs.
 	For example, this might be used to notify that the configuration profile has been switched.
 
@@ -69,19 +73,23 @@ def notifyOnce(self, **kwargs):
 FilterValueT = TypeVar("FilterValueT")
 
 
-class Filter(HandlerRegistrar, Generic[FilterValueT]):
+class Filter(
+		HandlerRegistrar[Union[Callable[..., FilterValueT], Callable[[FilterValueT], FilterValueT]]],
+		Generic[FilterValueT]
+):
 	"""Allows interested parties to register to modify a specific kind of data.
 	For example, this might be used to allow modification of spoken messages before they are passed to the synthesizer.
 
 	First, a Filter is created:
 
-	>>> messageFilter = extensionPoints.Filter()
+	>>> import extensionPoints
+	>>> messageFilter = extensionPoints.Filter[str]()
 
 	Interested parties then register to filter the data, see
 	L{register} docstring for details of the type of handlers that can be
 	registered:
 
-	>>> def filterMessage(message, someArg=None):
+	>>> def filterMessage(message: str, someArg=None) -> str:
 	... 	return message + " which has been filtered."
 	...
 	>>> messageFilter.register(filterMessage)
@@ -111,7 +119,7 @@ def apply(self, value: FilterValueT, **kwargs) -> FilterValueT:
 		return value
 
 
-class Decider(HandlerRegistrar):
+class Decider(HandlerRegistrar[Callable[..., bool]]):
 	"""Allows interested parties to participate in deciding whether something
 	should be done.
 	For example, input gestures are normally executed,
@@ -162,7 +170,7 @@ def decide(self, **kwargs):
 		return True
 
 
-class AccumulatingDecider(HandlerRegistrar):
+class AccumulatingDecider(HandlerRegistrar[Callable[..., bool]]):
 	"""Allows interested parties to participate in deciding whether something
 	should be done.
 	In contrast with L{Decider} all handlers are executed and then results are returned.
@@ -216,3 +224,54 @@ def decide(self, **kwargs) -> bool:
 		if (not self.defaultDecision) in decisions:
 			return (not self.defaultDecision)
 		return self.defaultDecision
+
+
+ChainValueTypeT = TypeVar("ChainValueTypeT")
+
+
+class Chain(HandlerRegistrar[Callable[..., Iterable[ChainValueTypeT]]], Generic[ChainValueTypeT]):
+	"""Allows creating a chain of registered handlers.
+	The handlers should return an iterable, e.g. they are usually generator functions,
+	but returning a list is also supported.
+
+	First, a Chain is created:
+
+	>>> chainOfNumbers = extensionPoints.Chain[int]()
+
+	Interested parties then register to be iterated.
+	See L{register} docstring for details of the type of handlers that can be
+	registered:
+
+	>>> def yieldSomeNumbers(someArg=None) -> Generator[int, None, None]:
+		... 	yield 1
+		... 	yield 2
+		... 	yield 3
+	...
+	>>> def yieldMoreNumbers(someArg=42) -> Generator[int, None, None]:
+		... 	yield 4
+		... 	yield 5
+		... 	yield 6
+	...
+	>>> chainOfNumbers.register(yieldSomeNumbers)
+	>>> chainOfNumbers.register(yieldMoreNumbers)
+
+	When the chain is being iterated, it yields all entries generated by the registered handlers,
+	see L{util.callWithSupportedKwargs} for how args passed to iter are mapped to the handler:
+
+	>>> chainOfNumbers.iter(someArg=42)
+	"""
+
+	def iter(self, **kwargs) -> Generator[ChainValueTypeT, None, None]:
+		"""Returns a generator yielding all values generated by the registered handlers.
+		@param kwargs: Arguments to pass to the handlers.
+		"""
+		for handler in self.handlers:
+			try:
+				iterable = callWithSupportedKwargs(handler, **kwargs)
+				if not isinstance(iterable, Iterable):
+					log.exception(f"The handler {handler!r} on {self!r} didn't return an iterable")
+					continue
+				for value in iterable:
+					yield value
+			except Exception:
+				log.exception(f"Error yielding value from handler {handler!r} for {self!r}")

source/extensionPoints/util.py

@@ -1,23 +1,36 @@
-#util.py
-#A part of NonVisual Desktop Access (NVDA)
-#Copyright (C) 2017 NV Access Limited
-#This file is covered by the GNU General Public License.
-#See the file COPYING for more details.
+# A part of NonVisual Desktop Access (NVDA)
+# Copyright (C) 2017-2023 NV Access Limited, Leonard de Ruijter
+# This file is covered by the GNU General Public License.
+# See the file COPYING for more details.
 
 """Utilities used withing the extension points framework. Generally it is expected that the class in __init__.py are
 used, however for more advanced requirements these utilities can be used directly.
 """
+
 import weakref
-import collections
 import inspect
-
-
-class AnnotatableWeakref(weakref.ref):
+from typing import (
+	Callable,
+	Generator,
+	Generic,
+	Optional,
+	OrderedDict,
+	Tuple,
+	TypeVar,
+	Union,
+)
+
+HandlerT = TypeVar("HandlerT", bound=Callable)
+HandlerKeyT = Union[int, Tuple[int, int]]
+
+
+class AnnotatableWeakref(weakref.ref, Generic[HandlerT]):
 	"""A weakref.ref which allows annotation with custom attributes.
 	"""
+	handlerKey: int
 
 
-class BoundMethodWeakref(object):
+class BoundMethodWeakref(Generic[HandlerT]):
 	"""Weakly references a bound instance method.
 	Instance methods are bound dynamically each time they are fetched.
 	weakref.ref on a bound instance method doesn't work because
@@ -26,8 +39,9 @@ class BoundMethodWeakref(object):
 	which can then be used to bind an instance method.
 	To get the actual method, you call an instance as you would a weakref.ref.
 	"""
+	handlerKey: Tuple[int, int]
 
-	def __init__(self, target, onDelete):
+	def __init__(self, target: HandlerT, onDelete):
 		def onRefDelete(weak):
 			"""Calls onDelete for our BoundMethodWeakref when one of the individual weakrefs (instance or function) dies.
 			"""
@@ -37,7 +51,7 @@ def onRefDelete(weak):
 		self.weakInst = weakref.ref(inst, onRefDelete)
 		self.weakFunc = weakref.ref(func, onRefDelete)
 
-	def __call__(self):
+	def __call__(self) -> Optional[HandlerT]:
 		inst = self.weakInst()
 		if not inst:
 			return
@@ -46,7 +60,8 @@ def __call__(self):
 		# Get an instancemethod by binding func to inst.
 		return func.__get__(inst)
 
-def _getHandlerKey(handler):
+
+def _getHandlerKey(handler: HandlerT) -> HandlerKeyT:
 	"""Get a key which identifies a handler function.
 	This is needed because we store weak references, not the actual functions.
 	We store the key on the weak reference.
@@ -58,7 +73,7 @@ def _getHandlerKey(handler):
 	return id(handler)
 
 
-class HandlerRegistrar(object):
+class HandlerRegistrar(Generic[HandlerT]):
 	"""Base class to Facilitate registration and unregistration of handler functions.
 	The handlers are stored using weak references and are automatically unregistered
 	if the handler dies.
@@ -75,12 +90,16 @@ def __init__(self):
 		#: Registered handler functions.
 		#: This is an OrderedDict where the keys are unique identifiers (as returned by _getHandlerKey)
 		#: and the values are weak references.
-		self._handlers = collections.OrderedDict()
+		self._handlers = OrderedDict[
+			HandlerKeyT,
+			Union[BoundMethodWeakref[HandlerT], AnnotatableWeakref[HandlerT]]
+		]()
 
-	def register(self, handler):
+	def register(self, handler: HandlerT):
 		"""You can register functions, bound instance methods, class methods, static methods or lambdas.
-		However, the callable must be kept alive by your code otherwise it will be de-registered. This is due to the use
-		of weak references. This is especially relevant when using lambdas.
+		However, the callable must be kept alive by your code otherwise it will be de-registered.
+		This is due to the use of weak references.
+		This is especially relevant when using lambdas.
 		"""
 		if inspect.isfunction(handler):
 			sig = inspect.signature(handler)
@@ -95,7 +114,24 @@ def register(self, handler):
 		weak.handlerKey = key
 		self._handlers[key] = weak
 
-	def unregister(self, handler):
+	def moveToEnd(self, handler: HandlerT, last: bool = False) -> bool:
+		"""Move a registered handler to the start or end of the collection with registered handlers.
+		This can be used to modify the order in which handlers are called.
+		@param last: Whether to move the handler to the end.
+			If C{False} (default), the handler is moved to the start.
+		@returns: Whether the handler was found.
+		"""
+		if isinstance(handler, (AnnotatableWeakref, BoundMethodWeakref)):
+			key = handler.handlerKey
+		else:
+			key = _getHandlerKey(handler)
+		try:
+			self._handlers.move_to_end(key=key, last=last)
+		except KeyError:
+			return False
+		return True
+
+	def unregister(self, handler: Union[AnnotatableWeakref[HandlerT], BoundMethodWeakref[HandlerT], HandlerT]):
 		if isinstance(handler, (AnnotatableWeakref, BoundMethodWeakref)):
 			key = handler.handlerKey
 		else:
@@ -107,7 +143,7 @@ def unregister(self, handler):
 		return True
 
 	@property
-	def handlers(self):
+	def handlers(self) -> Generator[HandlerT, None, None]:
 		"""Generator of registered handler functions.
 		This should be used when you want to call the handlers.
 		"""

tests/unit/extensionPointTestHelpers.py

@@ -5,9 +5,17 @@
 
 """Helper functions to test extension points."""
 
-from extensionPoints import Action, Decider, Filter, FilterValueT
+from extensionPoints import (
+	Action,
+	Chain,
+	ChainValueTypeT,
+	Decider,
+	Filter,
+	FilterValueT,
+)
 import unittest
 from contextlib import contextmanager
+from typing import Iterable
 
 
 @contextmanager
@@ -87,7 +95,7 @@ def filterTester(
 		useAssertDictContainsSubset: bool = False,
 		**expectedKwargs
 ):
-	"""A function that allows testing a Filter.
+	"""A context manager that allows testing a Filter.
 	@param testCase: The test case to apply the assertion on.
 	@param filter: The filter that will be applied by the test case.
 	@param expectedInput: The expected input as entering the filter handler.
@@ -116,3 +124,39 @@ def handler(value: FilterValueT, **kwargs):
 		filter.unregister(handler)
 		testFunc = testCase.assertDictContainsSubset if useAssertDictContainsSubset else testCase.assertDictEqual
 		testFunc(expectedKwargs, actualKwargs)
+
+
+@contextmanager
+def chainTester(
+		testCase: unittest.TestCase,
+		chain: Chain,
+		expectedOutput: Iterable[ChainValueTypeT],
+		useAssertDictContainsSubset: bool = False,
+		**expectedKwargs
+):
+	"""A context manager that allows testing a Filter.
+	@param testCase: The test case to apply the assertion on.
+	@param chain: The Chain that will be iterated by the test case.
+	@param expectedOutput: The expected output as returned by L{Chain.iter}
+		it will also be yielded by the context manager
+	@param useAssertDictContainsSubset: Whether to use L{unittest.TestCase.assertDictContainsSubset} instead of
+		L{unittest.TestCase.assertDictEqual}
+		This can be used if a Chain is iterated with dictionary values that can't be predicted at test time,
+		such as a driver instance.
+	@param expectedKwargs: The kwargs that are expected to be passed to the Chain handler.
+	"""
+	expectedKwargs["_called"] = True
+	actualKwargs = {}
+
+	def handler(**kwargs):
+		actualKwargs.update(kwargs)
+		actualKwargs["_called"] = True
+		return expectedOutput
+
+	chain.register(handler)
+	try:
+		yield expectedOutput
+	finally:
+		chain.unregister(handler)
+		testFunc = testCase.assertDictContainsSubset if useAssertDictContainsSubset else testCase.assertDictEqual
+		testFunc(expectedKwargs, actualKwargs)