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

Add support for custom providers for braille display auto detection #14531

Merged
merged 27 commits into from Feb 14, 2023

Conversation

LeonarddeR
Copy link
Collaborator

@LeonarddeR LeonarddeR commented Jan 11, 2023

Link to issue number:

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

Noone. User shouldn't experience anything different for now.

Description of development approach

  1. 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.
  2. 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)
  3. 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.
  4. 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.
  5. 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.

Change log entries:

For Developers

  • Added a new extension point type called Chain, which can be used to iterate over iterables returned by registered handlers.
  • Added the bdDetect.scanForDevices extension point. Handlers can be registered that yield BrailleDisplayDriver/DeviceMatch pairs that don't fit in existing categories, like USB or Bluetooth.

Code Review Checklist:

  • Pull Request description:
    • description is up to date
    • change log entries
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • API is compatible with existing add-ons.
  • Documentation:
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • Security precautions taken.

@AppVeyorBot
Copy link

See test results for failed build of commit f748e46ba0

@AppVeyorBot
Copy link

See test results for failed build of commit 52662b1e0c

@LeonarddeR LeonarddeR marked this pull request as ready for review January 12, 2023 07:26
@LeonarddeR LeonarddeR requested a review from a team as a code owner January 12, 2023 07:26
@LeonarddeR LeonarddeR added the merge-early Merge Early in a developer cycle label Jan 12, 2023
@LeonarddeR
Copy link
Collaborator Author

I'm pretty sure all these tests failures are unrelated.

@AppVeyorBot
Copy link

See test results for failed build of commit 7a36ab513c

@seanbudd seanbudd self-assigned this Jan 12, 2023
@seanbudd seanbudd added this to the 2023.2 milestone Jan 13, 2023
source/bdDetect.py Outdated Show resolved Hide resolved
source/bdDetect.py Show resolved Hide resolved
source/bdDetect.py Outdated Show resolved Hide resolved
source/bdDetect.py Outdated Show resolved Hide resolved
source/bdDetect.py Outdated Show resolved Hide resolved
source/braille.py Outdated Show resolved Hide resolved
source/braille.py Outdated Show resolved Hide resolved
source/extensionPoints/__init__.py Outdated Show resolved Hide resolved
source/extensionPoints/util.py Show resolved Hide resolved
self._handlers = collections.OrderedDict()
self._handlers = OrderedDict[
HandlerKeyT,
Union[BoundMethodWeakref[HandlerT], AnnotatableWeakref[HandlerT]]
Copy link
Member

Choose a reason for hiding this comment

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

this type also might be worth turning into a named variable, as it is used repeatedly

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same issue as abbove, nesting TypeVars in variables doesn't seem to be respected by PyRight.

source/bdDetect.py Outdated Show resolved Hide resolved
source/bdDetect.py Outdated Show resolved Hide resolved
source/bdDetect.py Outdated Show resolved Hide resolved
@LeonarddeR LeonarddeR marked this pull request as ready for review January 17, 2023 15:52
source/bdDetect.py Show resolved Hide resolved
source/bdDetect.py Outdated Show resolved Hide resolved
source/bdDetect.py Outdated Show resolved Hide resolved
source/bdDetect.py Outdated Show resolved Hide resolved
@seanbudd seanbudd removed the blocked label Feb 13, 2023
@seanbudd
Copy link
Member

@LeonarddeR can you address merge conflicts now that #14524 is merged

@LeonarddeR
Copy link
Collaborator Author

@LeonarddeR can you address merge conflicts now that #14524 is merged

Done.

@AppVeyorBot
Copy link

See test results for failed build of commit 35813eaf81

@seanbudd seanbudd merged commit 58a66fb into nvaccess:master Feb 14, 2023
LeonarddeR added a commit to LeonarddeR/nvda that referenced this pull request Feb 16, 2023
seanbudd pushed a commit that referenced this pull request Feb 17, 2023
Fix for #14531

Summary of the issue:
Error 'property' object is not iterable when enabling autodetect for the first time with Bluetooth on.

Description of user facing changes
None

Description of development approach
The detector was trying to save the bdDefsCache on the _DeviceInfoFfetcher class (not the instance).
michaelDCurran pushed a commit that referenced this pull request Mar 1, 2023
As part of Pull request #14531, I ordered display order in bdDetect alphabetically. However, it seems that querying Albatross first has a major negative impact on detecting other displays using the same Serial to USB converter (see Pull request #13045)

Description of user facing changes
Handy Tech displays such as the Modular Evolution instantly auto detect again.

Description of development approach
Restored the order, Albatross is last in the dictionary again. I'm pretty unhappy with how these displays work, but it looks like it's a design thing in the displays itself.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-early Merge Early in a developer cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Braille does not work on secure Windows screens while normal copy of NVDA is running
3 participants