Skip to content

Fix HID after Custom driver#13168

Merged
feerrenrut merged 5 commits into
rcfrom
fixHidAfterCustomDriver
Dec 17, 2021
Merged

Fix HID after Custom driver#13168
feerrenrut merged 5 commits into
rcfrom
fixHidAfterCustomDriver

Conversation

@feerrenrut

@feerrenrut feerrenrut commented Dec 15, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Link to issue number:

#13153

Summary of the issue:

It was still possible for a standard HID braille driver to take preference over a custom braille driver.

Description of how this pull request fixes the issue:

Ensure all devices are processed looking for a custom braille driver before reprocessing all devices looking for a standard HID braille driver.

While here I have tried to disambiguate the hid.BrailleDisplayDriver class.

  • Changing the name of the class should make logging clearer, instead of logging from BrailleDisplayDriver logging should come from HidBrailleDriver. This class is still mapped to the BrailleDisplayDriver name in the module.
  • Changing the name of the file / name property on the class, makes it harder to confuse with other usages of "hid" constant (now only KEY_HID)
  • Rather than yielding a string constant that happens to match the name of the driver, lazy import and expose from the class.

Testing strategy:

Ask RC users to test again.

Known issues with pull request:

None

Change log entries:

None

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

@feerrenrut

feerrenrut commented Dec 15, 2021

Copy link
Copy Markdown
Contributor Author

NVDA-devel mailing list has been asked to test this via the try-build. However, anyone else with a Braille display is invited to test this and give feedback.

@feerrenrut feerrenrut requested review from LeonarddeR and removed request for michaelDCurran December 16, 2021 03:19
@feerrenrut feerrenrut marked this pull request as ready for review December 16, 2021 10:49
@feerrenrut feerrenrut requested a review from a team as a code owner December 16, 2021 10:49
@feerrenrut feerrenrut requested review from michaelDCurran and removed request for a team December 16, 2021 10:49
@feerrenrut

feerrenrut commented Dec 16, 2021

Copy link
Copy Markdown
Contributor Author

After following up on testers reports, the results are inconclusive. Several testers did find issues, however it is not clear if these are issues that previously existed (I.E. in 2021.2) or if they are new with this release / branch. Several users have said this is an improvement. Without more solid information, I advise we merge this and put out an RC2 for wider testing.

@feerrenrut

feerrenrut commented Dec 16, 2021

Copy link
Copy Markdown
Contributor Author

In case it is required, there is another branch that extends this to put the autodetection behind a config flag, the flag is not yet exposed in the UI. There is a try-build of this that can be tested in case it is helpful.

LeonarddeR
LeonarddeR reviewed Dec 16, 2021
View reviewed changes
Comment thread source/bdDetect.py Outdated
LeonarddeR
LeonarddeR previously approved these changes Dec 16, 2021
View reviewed changes
seanbudd
seanbudd previously approved these changes Dec 17, 2021
View reviewed changes
@AppVeyorBot

AppVeyorBot commented Dec 17, 2021

Copy link
Copy Markdown

See test results for failed build of commit c8b6d566f3

lukaszgo1
lukaszgo1 reviewed Dec 17, 2021
View reviewed changes
Comment thread source/bdDetect.py Outdated
Comment thread source/bdDetect.py Outdated
@feerrenrut feerrenrut force-pushed the fixHidAfterCustomDriver branch from 2d1355c to f293991 Compare December 17, 2021 10:44
@feerrenrut feerrenrut force-pushed the fixHidAfterCustomDriver branch from f293991 to 4761a84 Compare December 17, 2021 10:46
@feerrenrut

feerrenrut commented Dec 17, 2021

Copy link
Copy Markdown
Contributor Author

@lukaszgo1 I have addressed the typo's, good catch. Would you like to review again?

@feerrenrut

feerrenrut commented Dec 17, 2021

Copy link
Copy Markdown
Contributor Author

I have rebased this, due to renamed files I want each commit to be clean and focused on a single goal so that it can be merged rather than squashed.

lukaszgo1
lukaszgo1 previously approved these changes Dec 17, 2021
View reviewed changes

@lukaszgo1 lukaszgo1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks fine to me in terms of code, though I am unable to test as the braille display I have is not compatible with HID nor had auto-detection issues with 2021.3.

LeonarddeR
LeonarddeR previously approved these changes Dec 17, 2021
View reviewed changes

@LeonarddeR LeonarddeR left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I like the overall improvements in here. Just a suggestion to avoid an inline func. N.b. Unfortunately I wasn't able to create a multi line comment, but you'll get the idea.

Comment thread source/bdDetect.py Outdated
@feerrenrut
Extract common test for HID Braille match
ddfa1dc
@feerrenrut
Rely on known implementation to provide special case braille driver name
a224a64 
Duplication of the string makes it a "magic value" and hard to trace its
meaning.
@feerrenrut
Rename class for clarity when printing name of class
d1344cf 
Explicitly map to BrailleDisplayDriver on the module.
@feerrenrut
Rename standard hid braille driver
d8c857d 
Ensure there is no confusion with KEY_HID="hid" in bdDetect.py
@feerrenrut feerrenrut dismissed stale reviews from LeonarddeR and lukaszgo1 via d8c857d December 17, 2021 13:35
@feerrenrut feerrenrut force-pushed the fixHidAfterCustomDriver branch from 4761a84 to d8c857d Compare December 17, 2021 13:35
@feerrenrut feerrenrut merged commit ca02205 into rc Dec 17, 2021
@feerrenrut feerrenrut deleted the fixHidAfterCustomDriver branch December 17, 2021 14:13
@nvaccessAuto nvaccessAuto added this to the 2022.1 milestone Dec 17, 2021
@feerrenrut feerrenrut modified the milestones: 2022.1, 2021.3.1 Dec 17, 2021
@feerrenrut

feerrenrut commented Dec 17, 2021

Copy link
Copy Markdown
Contributor Author

build of RC to test merged changes: https://ci.appveyor.com/api/buildjobs/pl56jgcjg2ym6xap/artifacts/output%2Fnvda_snapshot_rc-24378%2Ccaf14822.exe

feerrenrut added a commit that referenced this pull request Dec 22, 2021
@feerrenrut
Merge pull request #13168 from nvaccess/fixHidAfterCustomDriver
d46f6c5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seanbudd seanbudd seanbudd left review comments

@michaelDCurran michaelDCurran Awaiting requested review from michaelDCurran michaelDCurran is a code owner automatically assigned from nvaccess/developers

+2 more reviewers

@LeonarddeR LeonarddeR LeonarddeR left review comments

@lukaszgo1 lukaszgo1 lukaszgo1 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2021.3.1

Development

Successfully merging this pull request may close these issues.

6 participants

@feerrenrut @AppVeyorBot @LeonarddeR @seanbudd @lukaszgo1 @nvaccessAuto