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

Abstract vision framework with included support for focus, navigator object and browse mode caret highlighting #9064

Merged
merged 121 commits into from Aug 12, 2019

Conversation

leonardder
Copy link
Collaborator

@leonardder leonardder commented Dec 13, 2018

Pr provided on behalf of @BabbageCom

Link to issue number

Implements #971, but without a gui

Background

For years, NVDA has been an open source screen reader for the blind and partially sighted people who rely on braille and speech output. There are no built-in facilities within NVDA to manipulate the contents of the screen. There are several outstandign issues asking for such features, such as:

  • #7857: Making the screen black (i.e. a screen curtain) while NVDA is active, mainly for privacy reasons
  • #971: Visual highlight of focus, review or browse mode caret location

There is also a long standing desire to have a basic screen magnification facility within NVDA.

This pr intends to lay the base of a vision framework that can be used to implement such functionality in the core of NVDA. Though there is no GUI in the current pull request, the framework is fully working. The GUI will be added in a follow up pr.

A new vision module

Most magic is supposed to happen within a new vision module. The vision module consists of two distinct concepts:

  • Vision enhancement providers: providers of functionality that change the contents of the screen. A vision enhancement provider can be a magnifier, highlighter, or whatever else that changes the screen contents.

    This pr contains a highlighter based on a transparent overlay window on which lines are drawn around the several objects on screen.

    The Screen Curtain add-on, which is a prototype add-on written by me on behalf of @BabbageCom and ported to an add-on by @josephsl, is supposed to be added to NVDA in subsequent pr.

    Vision enhancement providers will also be able to benefit from the functionality to set driver/provider specific options in the gui (#8214).

  • The vision handler, which receives information about focus, foreground, navigator object, review and mouse changes from the core of NVDA. It keeps track of the location of objects on screen and communicates this information to one or more active vision enhancement providers. This information is communicated to vision enhancement providers using extension points. The handler keeps track of active enhancement providers and can activate and deactivate them. Therefore, it is very much similar to the braille handler in its approach.

Follow up strategy

@feerrenrut has requested for this framework to be filed into subsequent pull requests. After this pr has been fully reviewed and merged, I intent to file at least two additional pr's:

  1. A graphical interface for the framework
  2. A built-in screen curtain

Known issues

The current highlighter colors might be difficult for some people to see. In the future, I'd like this to be configurable.

Changelog entries:

  • Changes for developers
    • A vision framework has been added to NVDA. It allows you to create vision enhancement providers; modules that can change screen contents, optionally based on input from NVDA about object locations.
      • Add-ons can bundle their own providers in a visionEnhancementProviders folder.
      • See the vision and visionEnhancementProviders modules for the implementation of the framework and examples, respectively.

@leonardder leonardder added sightedDevIdeal component/vision labels Dec 13, 2018
@leonardder leonardder requested a review from feerrenrut Dec 13, 2018
@Adriani90
Copy link
Collaborator

@Adriani90 Adriani90 commented Dec 13, 2018

Support from @nishimotz is very welcome I think. As part of the japanese NVDA Team, he is the creator of the add-on focus highlight
https://github.com/nvdajp/focusHighlight
I am sure that the japanese NVDA Team can bring much knowledge in this work as well.

@nishimotz
Copy link
Contributor

@nishimotz nishimotz commented Dec 16, 2018

I am testing the AppVeyor build (nvda_snapshot_pr9064-16441,7aaee3c0.exe)
and confirmed the default highlighter works if nvda.ini is modified something like this

[vision]
        highlighter = NVDAHighlighter
        [[NVDAHighlighter]]
                highlightFocus = true
                highlightCaret = true
                highlightNavigatorObj = true

except NotImplementedError:
rects = None
if rects:
index = 0 if self.obj.isTextSelectionAnchoredAtStart else -1
Copy link
Contributor

@nishimotz nishimotz Dec 17, 2018

Choose a reason for hiding this comment

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

I got errors as follows regarding this line:

ERROR - queueHandler.flushQueue (13:47:02.601):
Error in func _loadBufferDone from eventQueue
Traceback (most recent call last):
  File "queueHandler.pyc", line 53, in flushQueue
  File "virtualBuffers\__init__.pyc", line 464, in _loadBufferDone
  File "browseMode.pyc", line 1152, in event_treeInterceptor_gainFocus
  File "browseMode.pyc", line 1465, in event_gainFocus
  File "vision\__init__.pyc", line 601, in handleGainFocus
  File "vision\NVDAHighlighter.pyc", line 63, in updateContextRect
  File "vision\__init__.pyc", line 270, in updateContextRect
  File "vision\__init__.pyc", line 190, in getContextRect
  File "vision\__init__.pyc", line 205, in _getRectFromTextInfo
NameError: global name 'self' is not defined

Leonard de Ruijter added 3 commits Dec 17, 2018
…a gainFocus, since the updated object might have a caret in it and we also want to account for this
@nishimotz
Copy link
Contributor

@nishimotz nishimotz commented Jan 18, 2019

nvda_snapshot_pr9064-16574,1ca723ff.exe fails initialization as follows:

INFO - core.main (20:56:31.969):
NVDA version pr9064-16574,1ca723ff
INFO - core.main (20:56:31.969):
Using Windows version 10.0.17134 workstation

INFO - core.main (20:56:33.881):
NVDA initialized
ERROR - unhandled exception (20:56:33.881):
Traceback (most recent call last):
  File "wx\core.pyc", line 3240, in <lambda>
  File "vision.pyc", line 545, in setProvider
  File "vision.pyc", line 440, in getProvider
ImportError: No module named NVDAHighlighter

I cannot import this via Python console as well:

>>> import visionEnhancementProviders.NVDAHighlighter
Traceback (most recent call last):
  File "<console>", line 1, in <module>
ImportError: No module named NVDAHighlighter

@leonardder
Copy link
Collaborator Author

@leonardder leonardder commented Jan 18, 2019

Thanks @nishimotz! This should have been fixed in the most recent code.

Leonard de Ruijter added 4 commits Jan 22, 2019
…n the chain. We don't want random global plugins to override vision enhancement provider gesture people might rely on.

Also update some copyright headers
Leonard de Ruijter added 2 commits Feb 11, 2019
…merge conflict and adding vision enhancement providers to scratchpad.
…rt due to an error caused by locationHelper conversion
@feerrenrut
Copy link
Member

@feerrenrut feerrenrut commented Aug 5, 2019

Except there is a failing unit test, "No module named 'wx'"

@leonardder
Copy link
Collaborator Author

@leonardder leonardder commented Aug 5, 2019

@leonardder leonardder requested a review from feerrenrut Aug 6, 2019
@leonardder
Copy link
Collaborator Author

@leonardder leonardder commented Aug 6, 2019

@feerrenrut I'm afraid I had to use some noqa in the unit module to get this work. This is because we have to change the path before importing modules, and this is something Flake8 doesn't agree with.

The sourceEnv noqa is intentional, as this module is only imported to manipulate the path.

@@ -33,7 +31,9 @@
SOURCE_DIR = os.path.join(TOP_DIR, "source")
# Let us import modules from the NVDA source.
sys.path.insert(1, SOURCE_DIR)
import sourceEnv
import sourceEnv # noqa: F401
Copy link
Member

@feerrenrut feerrenrut Aug 6, 2019

Choose a reason for hiding this comment

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

I'm fine with using noqa to suppress this case, however please include a comment about why and a hint about what the lint error is. It's not much fun for a future developer to have to look these up and understand why it is like this.

Copy link
Collaborator Author

@leonardder leonardder Aug 6, 2019

Choose a reason for hiding this comment

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

What would be proper syntax for this? I assume something like this is not going to work

Suggested change
import sourceEnv # noqa: F401
import sourceEnv # noqa: F401 module imported but unused

Copy link
Member

@feerrenrut feerrenrut Aug 6, 2019

Choose a reason for hiding this comment

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

Yep something like that works, though it gets messy if you have multiple errors to ignore.

I would do:

# F401: module imported but unused. We need sourceEnv for unit tests because of ?
import sourceEnv  # noqa: F401

Copy link
Collaborator Author

@leonardder leonardder Aug 6, 2019

Choose a reason for hiding this comment

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

I think I've done something along those lines.

@leonardder
Copy link
Collaborator Author

@leonardder leonardder commented Aug 6, 2019

Mm, I was ignoring linter warning F402, which had to be E402. Interesting enough, Ignoring F402 also ignored E402 unexpectedly, that's why I didn't see this before.

@AppVeyorBot
Copy link

@AppVeyorBot AppVeyorBot commented Aug 8, 2019

PR introduces Flake8 errors 😲

See test results for Failed build of commit 6fbc40b7de

@@ -70,7 +75,13 @@ class AppArgs:
# NVDAObjects need appModuleHandler to be initialized.
import appModuleHandler
appModuleHandler.initialize()
# Anything which notifies of cursor updates requires braille to be initialized.
# Vision needs a wx app to be available.
Copy link
Member

@feerrenrut feerrenrut Aug 8, 2019

Choose a reason for hiding this comment

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

From a unit testing perspective it would be better if we can avoid this. Which part of the vision framework relies on a wx app being available? Can we mock this part instead?

Copy link
Member

@feerrenrut feerrenrut left a comment

Generally this looks good. But I am a little worried about the dependencies of vision, if possible I would prefer it rely on dependency injection if possible. Can we start by identifying what fails if wx is missing?

@leonardder
Copy link
Collaborator Author

@leonardder leonardder commented Aug 8, 2019

Leonard de Ruijter added 3 commits Aug 9, 2019
* CustomWindow.className should be abstract, which is now possible in Python 3.
* Ensure that when calling del on a window, handle the case where handle is not yet set on the window
* Add additional logging to windowUtils Window destruction.
* Use the typing module
* In Python 2, we couldn't join the highlighter thread, as it stopped the quit message from arriving. This now seems to work again.
* Reinitialization is no longer broken. We should create the transparent brush at window class creation time and not at class construction time.
@feerrenrut feerrenrut merged commit 5ad32bf into nvaccess:master Aug 12, 2019
1 check passed
@nvaccessAuto nvaccessAuto added this to the 2019.3 milestone Aug 12, 2019
feerrenrut added a commit that referenced this issue Aug 12, 2019
JulienCochuyt added a commit to accessolutions/nvda that referenced this issue Sep 1, 2019
… navigator object and browse mode caret highlighting (PR nvaccess#9064)
michaelDCurran pushed a commit that referenced this issue Sep 3, 2019
… navigator object and browse mode caret highlighting (PR #9064) (#10160)
@leonardder leonardder added the BabbageWork label Oct 11, 2019
feerrenrut added a commit that referenced this issue Nov 18, 2019
To preserve history of moved code, PR #10082 was rebased and condensed
prior to merging.

Follow up from #9064
Closes #971
Closes #10082
JulienCochuyt added a commit to accessolutions/nvda that referenced this issue May 2, 2020
feerrenrut pushed a commit that referenced this issue May 7, 2020
…sole (PR #11090)

A few new entries were missing as of PR #9064 and PR #9790
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BabbageWork component/vision sightedDevIdeal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants