Refactor BrailleHandler.setDisplayByName #14524
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
See test results for failed build of commit d349afb9d3 |
seanbudd
reviewed
Jan 9, 2023
There was a problem hiding this comment.
Hi @LeonarddeR, the approach looks good to me, just minor suggestions.
Due to the risk of this PR, I think we should merge it after #14512
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
|
@seanbudd Thanks for the review. |
seanbudd
approved these changes
Jan 11, 2023
6 tasks
See test results for failed build of commit 7e3b7ecec9 |
seanbudd
pushed a commit
that referenced
this pull request
Feb 14, 2023
…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.
| log.debugWarning(f"Couldn't initialize display driver {name!r}", exc_info=True) | ||
| fallbackDisplayClass = _getDisplayDriver(NO_BRAILLE_DISPLAY_NAME) | ||
| # Only initialize the fallback if it is not already set | ||
| if self.display.__class__ == fallbackDisplayClass: |
There was a problem hiding this comment.
Should this not be != instead?
Getting reports if NVDA can not initialize a specifically configured display on start-up, then handler.display is None and the Braille page in the GUI is broken.
6 tasks
seanbudd
pushed a commit
that referenced
this pull request
Jul 28, 2023
Addresses issue introduced by pr #14524 Summary of the issue: Reports from users that if a specifically configured braille display cannot be initialized on start-up, then the Braille category in the NVDA settings dialog is broken and does not show a valid braille driver (noBraille or otherwise). Description of user facing changes The Braille category in NVDA settings will continue to be usable if the configured Braille display cannot be initialized on start-up. Description of development approach In braille.handler.setDisplayByName if setting the display failed, the display is not set to NoBraille if the display was (not) already NoBraille. Previously this logic was incorrectly reversed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Link to issue number:
None
Summary of the issue:
braille.BrailleHandler.setDisplayname is too complex. It also relies on reentrancy and is difficult to grasp.
Description of user facing changes
See bug fixes below
Description of development approach
Testing strategy:
Known issues with pull request:
None known
Change log entries:
Bug fixes
Code Review Checklist: