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

Vision framework: add a screen curtain #10090

Merged
merged 10 commits into from
Sep 6, 2019
Merged

Vision framework: add a screen curtain #10090

merged 10 commits into from
Sep 6, 2019

Conversation

LeonarddeR
Copy link
Collaborator

Link to issue number:

Closes #7857

Summary of the issue:

It has been a long awaited feature to make your screen black when using NVDA. There has been an initial version of this functionality in the Screen Curtain add-on. Now the vision framework is live, we can bring it to core very easily.

Description of how this pull request fixes the issue:

Adds a screen curtain based on the Windows Magnification API, windows 8 and above. This function is not supported for Windows 7. Since Windows 7 is EOL in around half a year, I do not care much myself. It shouldn't be too difficult to create something based on the code for the highlighter, but that's beyond the scope of this pr. It can be distributed as an add-on of course.

Testing performed:

  • Tested that the screen is black. when the curtain is enabled.
  • Tested that NVDA+control+/ toggles the curtain.

Known issues with pull request:

Change log entry:

@zstanecic
Copy link
Contributor

zstanecic commented Aug 13, 2019 via email

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented Aug 13, 2019 via email

@zstanecic
Copy link
Contributor

zstanecic commented Aug 13, 2019 via email

@lukaszgo1
Copy link
Contributor

Then they can remap the shortcut to something better for them, can't they?

@zstanecic
Copy link
Contributor

zstanecic commented Aug 13, 2019 via email

@lukaszgo1
Copy link
Contributor

Translators can already remap particular shortcuts for they language. If shortcut is really uncomfortable for a specific language community it should be remapped as part of the translation IMO.

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented Aug 13, 2019 via email

@josephsl
Copy link
Collaborator

josephsl commented Aug 13, 2019 via email

@XLTechie
Copy link
Collaborator

XLTechie commented Aug 13, 2019 via email

@beqabeqa473
Copy link
Contributor

beqabeqa473 commented Aug 14, 2019 via email

@zstanecic
Copy link
Contributor

zstanecic commented Aug 14, 2019 via email

@zstanecic
Copy link
Contributor

zstanecic commented Aug 14, 2019 via email

@zstanecic
Copy link
Contributor

zstanecic commented Aug 14, 2019 via email

@feerrenrut feerrenrut added this to the 2019.3 milestone Aug 14, 2019
@ruifontes
Copy link
Contributor

ruifontes commented Aug 14, 2019 via email

@zstanecic
Copy link
Contributor

zstanecic commented Aug 14, 2019 via email

@LeonarddeR
Copy link
Collaborator Author

I think most people have made their points clear. I think there are tree options:

  1. Leave the gesture as is
  2. Use another gesture. I'd like to suggest NVDA+shift+c.
  3. Remove the mapping altogether.

Apart from feedback on option 2, I think that the point about gestures being incompatible to keyboard layouts sis an issue that is much broader than this particular pull request. Therefore, I'd like to kindly ask you to move discussion about this to one of the mailing lists, preferably NVDA devel.

Co-Authored-By: André-Abush Clause <[email protected]>
@feerrenrut
Copy link
Contributor

Not really wanting to start the debate about a default gesture again. Personally I think it should be un-mapped by default. More importantly, a mechanism is necessary to prevent this getting stuck on. For a user who relies on sight, there is no warning that checking screen curtain will make it black. If this user had speech disabled, they may find it quite hard to turn screen curtain off.

Suggestion:

  • Before enabling screen curtain, present a dialog explaining this will make the screen black, allow the user to proceed or cancel.
  • On the dialog, have a checkbox "don't warn me again."
  • In the configuration for screen curtain also present the "don't warn before starting screen curtain"

When enabling or disabling screen curtain, I think it should be announced as a high priority. Something brief like "screen curtain disabled".

As already mentioned, I expect turning on and off screen curtain will be rare. Getting feedback for such a deliberate action seems important to me.

@AppVeyorBot
Copy link

PR introduces Flake8 errors 😲

See test results for Failed build of commit dfe910866f

@LeonarddeR
Copy link
Collaborator Author

@feerrenrut: as discussed by voice, this is now in the stage where the globalCommand is unassigned by default but where there is no warning when initializing. Warning gui will be added to #10082

source/globalCommands.py Outdated Show resolved Hide resolved
Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for your work on this @LeonarddeR !

@feerrenrut feerrenrut merged commit c628071 into nvaccess:master Sep 6, 2019
feerrenrut added a commit that referenced this pull request Sep 6, 2019
@mohdshara
Copy link

just a small typo:
Toggles the state of the screen curtain, either by making the screen black or SHOWING the contents of the screen. If pressed to enable once, the screen curtain is enabled until you restart NVDA. If pressed tree times, it is enabled until you disable it
Obviously it should be three times or thrice.

@LeonarddeR LeonarddeR added the BabbageWork Pull requests filed on behalf of Babbage B.V. label Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BabbageWork Pull requests filed on behalf of Babbage B.V.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Give an option to make the computer screen black to nvda