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

Allow excluding display drivers from automatic detection #15200

Merged
merged 23 commits into from Sep 1, 2023

Conversation

LeonarddeR
Copy link
Collaborator

@LeonarddeR LeonarddeR commented Jul 27, 2023

Link to issue number:

closes #15196

Summary of the issue:

  1. It is currently not possible to disable display drivers for automatic detection, other than the HID braille standard
  2. There is no obvious way for add-on bundled braille display drivers to register themselves in automatic detection.

Description of user facing changes

  1. In the braille display selection dialog, when automatic is selected, there is now an additional checklistbox that allows you to disable drivers. The disabled drivers are saved in config.
  2. The advanced option for to disable HID braille has been removed. Users should use the new checklistbox instead. There is a config profile upgrade step to remember the user's decision about explicitly disabling HID.
  3. It is now possible to select the HID braille driver manually when it is disabled for automatic detection. The old option in advanced setting disabled HID completely, i.e. the driver wasn't available for manual selection either.

Description of development approach

  1. Added a registerAutomaticDetection classmethod to BrailleDisplayDriver. This method is called by bdDetect.initialize. The classmethod receives a bdDetect.DriverRegistrar that contains the following methods:
    • addUsbDevices: to register USB devices
    • addBluetoothDevices: To register bluetooth devices
    • addDeviceScanner: convenience method to register a handler to the scanForDevices extension point
  2. Added a supportsAutomaticDetection boolean attribute on braille display drivers. This should be set to True for bdDetect to recognize the driver.
  3. Removed the option from the config to enable/disable HID, instead rely on a new excludeDisplays lisst in the config that is populated by the new option in the braille display selection dialog. Note that exclusions are saved, not inclusions. This ensures that new drivers will be opt-out.
  4. bdDetect.Detector._queueBgScan has a backwards compatible change for its limitToDevices parameter. When None, it falls back to the list of enabled drivers. When no drivers are disabled, it checks all drivers.
  5. Logic from bdDetect.initialize() has been moved to the respective display drivers, calling the replacement methods on the driverRegistrar and omitting the name of the driver in the method call.

Testing strategy:

Performed the following tests:

  • Ensured that my APH mantis 40 is detected using both USB and Bluetooth
  • Ensured that mantis wouldn't be detected when explicitly disabled
  • Ensured that a config profile switch would detect a new display if an earlier detected display is disabled in a new profile, ensuring the disabled entry would be respected
  • Ensured that HID braille is properly disabled as part of the config profile upgrade step

Known issues with pull request:

None known

Change log entries:

New features

  • When using automatic detection of braille displays, it is now possible to opt-out drivers from detection from the Braille display Selection dialog. (Allow add-on braille display drivers to opt in for auto detection #15196)
    Changes
  • The advanced setting to enable support for HID braille has been removed in favor of the new option to disable specific drivers for braille display auto detection in the braille display selection dialog. (Allow add-on braille display drivers to opt in for auto detection #15196)
    For Developers
  • Added official support to register custom braille display drivers in the automatic braille display detection process. Consult the braille.BrailleDisplayDriver class documentation for more details. Most notably, the supportsAutomaticDetection attribute must be set to True and the registerAutomaticDetection classmethod must be implemented. (Allow add-on braille display drivers to opt in for auto detection #15196)
    Deprecations
  • bdDetect.addUsbDevices and bdDetect.addBluetoothDevices have been deprecated. Braille display drivers should implement the registerAutomaticDetection classmethod instead. That method receives a DriverRegistrar object on which the addUsbDevices and addBluetoothDevices methods can be used.
  • The default implementation of the check method on BrailleDisplayDriver uses bdDetect.driverHasPossibleDevices for devices that are marked as thread safe. Starting from NVDA 2024.1, in order for the base method to use bdDetect.driverHasPossibleDevices, the supportsAutomaticDetection attribute must be set to True as well.

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 0655eef6e9

@LeonarddeR LeonarddeR marked this pull request as ready for review July 27, 2023 11:02
@LeonarddeR LeonarddeR requested review from a team as code owners July 27, 2023 11:02
@AppVeyorBot
Copy link

See test results for failed build of commit b8f389c94c

@seanbudd seanbudd added the merge-early Merge Early in a developer cycle label Jul 28, 2023
@LeonarddeR
Copy link
Collaborator Author

It looks like #15208 also changed model names in the comments in bdDetect.py. I don't think that was a desirable change, therefore I left the dots in tact when fixing merge conflicts, effectively reverting that change.

@AppVeyorBot
Copy link

See test results for failed build of commit 7007bf6dcb

@AppVeyorBot
Copy link

See test results for failed build of commit 088dfaef14

@seanbudd seanbudd added the blocked/needs-product-decision A product decision needs to be made. Decisions about NVDA UX or supported use-cases. label Jul 31, 2023
@seanbudd
Copy link
Member

@michaelDCurran - this and the issue #15196 still need a product decision

@seanbudd seanbudd added conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. and removed blocked/needs-product-decision A product decision needs to be made. Decisions about NVDA UX or supported use-cases. labels Aug 29, 2023
Copy link
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

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

Thanks @LeonarddeR for taking on this large project

source/brailleDisplayDrivers/alva.py Outdated Show resolved Hide resolved
source/brailleDisplayDrivers/seikantk.py Outdated Show resolved Hide resolved
source/brailleDisplayDrivers/brailliantB.py Outdated Show resolved Hide resolved
source/bdDetect.py Outdated Show resolved Hide resolved
source/bdDetect.py Outdated Show resolved Hide resolved
source/bdDetect.py Show resolved Hide resolved
@seanbudd seanbudd removed the request for review from michaelDCurran August 31, 2023 08:07
@seanbudd seanbudd marked this pull request as draft August 31, 2023 08:07
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
@AppVeyorBot
Copy link

See test results for failed build of commit 45bceb0d43

@LeonarddeR LeonarddeR marked this pull request as ready for review August 31, 2023 11:07
LeonarddeR and others added 2 commits August 31, 2023 13:18
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
Copy link
Member

@Qchristensen Qchristensen left a comment

Choose a reason for hiding this comment

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

User guide looks good, thanks!

@seanbudd seanbudd merged commit 104de7c into nvaccess:master Sep 1, 2023
1 check was pending
@nvaccessAuto nvaccessAuto added this to the 2023.3 milestone Sep 1, 2023
@bramd
Copy link
Contributor

bramd commented Sep 4, 2023

The config migration seems to fail. When trying to update from a previous alpha build, I get:

Traceback (most recent call last):
  File "config\__init__.pyc", line 571, in _initBaseConf
  File "config\__init__.pyc", line 605, in _loadConfig
  File "config\__init__.pyc", line 601, in _loadConfig
  File "config\profileUpgrader.pyc", line 24, in upgrade
  File "config\profileUpgrader.pyc", line 41, in _doConfigUpgrade
  File "config\profileUpgradeSteps.pyc", line 368, in upgradeConfigFrom_10_to_11
  File "configobj\__init__.pyc", line 508, in __getitem__
KeyError: 'excludedDisplays'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. merge-early Merge Early in a developer cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow add-on braille display drivers to opt in for auto detection
6 participants