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

Process injected mouse events #8459

Merged
merged 13 commits into from Dec 7, 2018

Conversation

Projects
None yet
5 participants
@leonardder
Collaborator

leonardder commented Jun 28, 2018

Link to issue number:

Closes #8452
Fixes #7480

Summary of the issue:

NVDA silently ignores mouse events that are injected by third party applications, particularly remote control software such as TeamViewer. When mouse tracking is enabled, NVDA does not announce what is under the mouse when a TeamViewer partner is controlling it.

Description of how this pull request fixes the issue:

  1. This pr adds a new mouse option to enable/disable the processing of injected mouse control, similar to the "handle keys from other applications" keyboard option. This option is enabled by default.
  2. Since NVDA also executes mouse events by itself, events executed with winUser.mouse_event are still ignored. To accomplish this without having winUser tamper with a boolean variable in mouseHandler, there is now a ignoreINjection context manager in mouseHandler that deals with this. For consistency reasons, i also added an ignoreINjection context manager to keyboardHandler.

Testing performed:

Tested injected mouse events by having a colleague control my system remotely via TeamViewer. His mouse movement was announced by NVDA.

Known issues with pull request:

Mouse movement caused by SetCursorPos, which is strictly spoken only moving the mouse cursor, is still ignored by NVDA. We probably have to hook SetCursorPos if we want to support this, but I don't think that this is relevant. IN any case, it falls outside the scope of this pr.

Change log entry:

  • New features
    • Added an option to NVDA's mouse settings to make NVDA handle situations where the mouse is controlled by another application. (#8452)
      • This will allow NVDA to track the mouse when a system is controlled remotely using TeamViewer or other remote control software.

@leonardder leonardder requested a review from feerrenrut Jun 28, 2018

@feerrenrut

I'm a little worried about the robustness of this approach.

@contextmanager
def ignoreInjection():
"""Context manager that allows ignoring injected keys temporarily by using a with statement."""
global ignoreInjected

This comment has been minimized.

@feerrenrut

feerrenrut Jul 2, 2018

Contributor

This won't be thread safe. If multiple threads use this context manager at once, the value of ignoreInjected could be squashed.

@@ -1353,13 +1353,20 @@ def makeSettings(self, settingsSizer):
self.audioDetectBrightnessCheckBox=sHelper.addItem(wx.CheckBox(self,label=audioDetectBrightnessText))
self.audioDetectBrightnessCheckBox.SetValue(config.conf["mouse"]["audioCoordinates_detectBrightness"])
# Translators: This is the label for a checkbox in the
# mouse settings panel.
handleInjectedMouseControlText = _("Handle mouse control from other &applications")

This comment has been minimized.

@feerrenrut

feerrenrut Jul 2, 2018

Contributor

I don't think the naming of this setting conveys what it does. Maybe something like "Ignore mouse movement triggered by other applications"

This comment has been minimized.

@leonardder

leonardder Jul 2, 2018

Collaborator

I started with the word movement, but the problem with that word is that strictly spoken, mouse clicks aren't movement either.

This comment has been minimized.

@feerrenrut

feerrenrut Jul 3, 2018

Contributor

I was more concerned with the word "handle". Though "mouse control" doesn't quite sound right to me either. Perhaps "Ignore mouse input from other applications"?

This comment has been minimized.

@leonardder

leonardder Jul 3, 2018

Collaborator

Hmm, ignore mouse input would turn the boolean upside down and causes a symmetrical conflict with handle keys from other applications in the keyboard settings.

How about "Process mouse input from other applications"

This comment has been minimized.

@leonardder

leonardder Aug 27, 2018

Collaborator

After considering this a little more, I think that I agree now with the wording using "ignore" now. May be it makes sense to reword the keyboard settings option as well?

# Import late to avoid circular import
import mouseHandler
with mouseHandler.ignoreInjection():
return user32.mouse_event(*args)

This comment has been minimized.

@feerrenrut

feerrenrut Jul 2, 2018

Contributor

I don't think that we can rely on the timing of receiving these events. Currently this design relies on the events being received while this contextManager is active.

This comment has been minimized.

@leonardder

leonardder Jul 2, 2018

Collaborator

Hmmmm, a similar approach is used in keyboardHandler.KeyboardInputGesture.send, though there, wx.Yield is called before closing the context manager.

This comment has been minimized.

@feerrenrut

feerrenrut Jul 3, 2018

Contributor

I have a feeling that code relies on a sleep, which I'm not a fan of either! Really we need a way to identify that a future received event is the one we planned to ignore, and only ignore that one.

This comment has been minimized.

@leonardder

leonardder Jul 3, 2018

Collaborator

Hmm yes, this could be a cause of #8020.
Note that similar to wx.Yield, we also have wx.YieldIfNeeded.
I"m still not sure what to do here. @michaelDCurran or @jcsteh, thoughts?

This comment has been minimized.

@leonardder

leonardder Aug 27, 2018

Collaborator

I discovered that there's also wx.GetApp().SafeYieldFor which allows a bitmask of events to be provided, limiting the yield to whatever events you provide.

Regarding SafeYield:

This function is similar to wx.Yield , except that it disables the user input to all program windows before calling wx.AppConsole.Yield and re-enables it again afterwards.

@leonardder

This comment has been minimized.

Collaborator

leonardder commented Jul 30, 2018

@josephsl: Could you elaborate on differences between WX Python 3 and 4 regarding wx.Yield? This code depends on wx.Yield, so it might make sense to change this as part of this pr at least in this area of the code.

@josephsl

This comment has been minimized.

Collaborator

josephsl commented Jul 30, 2018

@leonardder

This comment has been minimized.

Collaborator

leonardder commented Aug 27, 2018

It might make sense to implement #8679 as part of this pr.

@leonardder

This comment has been minimized.

Collaborator

leonardder commented Dec 5, 2018

@feerrenrut: I'm a bit unsure as what to do to make this pr to a success now. I and others would really appreciate this being included in a next release of NVDA>

@michaelDCurran

This comment has been minimized.

Contributor

michaelDCurran commented Dec 5, 2018

@michaelDCurran

KeyboardInputGesture.send requires a sleep and wx.yield because we want to guarantee that by the time that send returns, the keyboard input generated has been injected and NVDA has received and processed it. As we do not have scripts for mouse input currently, this is not relevant for the mouse.
From what I can read on msdn, low-level keyboard hooks will be run within the call to keybd_event, so to low-level mouse hooks will be run within mouse_event. Thus we can guarantee that the related ignore input variable we are setting with the context manager will be correct when the hooks run.
Perhaps we should look into replacing wx.yield in keyboardHandler, but I feel this is not something for this pr.
In short, once the other review actions in my review here are addressed, I am happy to approve this.

@@ -101,6 +101,7 @@
audioCoordinates_minPitch = integer(default=220)
audioCoordinates_maxPitch = integer(default=880)
reportMouseShapeChanges = boolean(default=false)
ignoreINjectedMouseInput = boolean(default=false)

This comment has been minimized.

@michaelDCurran

michaelDCurran Dec 5, 2018

Contributor

Should not be a capital N in INjected.

@@ -1363,13 +1363,20 @@ def makeSettings(self, settingsSizer):
self.audioDetectBrightnessCheckBox=sHelper.addItem(wx.CheckBox(self,label=audioDetectBrightnessText))
self.audioDetectBrightnessCheckBox.SetValue(config.conf["mouse"]["audioCoordinates_detectBrightness"])
# Translators: This is the label for a checkbox in the
# mouse settings panel.
ignoreINjectedMouseInputText = _("Ignore mouse input from other &applications")

This comment has been minimized.

@michaelDCurran

michaelDCurran Dec 5, 2018

Contributor

Again INjected should not have capital N

# mouse settings panel.
ignoreINjectedMouseInputText = _("Ignore mouse input from other &applications")
self.ignoreINjectedMouseInputCheckBox=sHelper.addItem(wx.CheckBox(self,label=ignoreINjectedMouseInputText))
self.ignoreINjectedMouseInputCheckBox.SetValue(config.conf["mouse"]["ignoreINjectedMouseInput"])

This comment has been minimized.

@michaelDCurran

michaelDCurran Dec 5, 2018

Contributor

Okay, looks like a bad find and replace ;)

# #8452: NVDA should ignore mouse events that are injected by itself.
# Import late to avoid circular import
import mouseHandler
with mouseHandler.ignoreInjection():

This comment has been minimized.

@michaelDCurran

michaelDCurran Dec 5, 2018

Contributor

I do not like NVDA specific state being affected in winUser. winUser should really just be a module that pythonizes win32 APIs. Rather add a wrapper function in mouseHandler or a new mouseUtils module or something.

This comment has been minimized.

@leonardder

leonardder Dec 6, 2018

Collaborator

Valid point there. It required some changes to get this right, though.

@michaelDCurran

This comment has been minimized.

Contributor

michaelDCurran commented Dec 7, 2018

@leonardder Are you happy for this to be merged to master now?

@leonardder

This comment has been minimized.

Collaborator

leonardder commented Dec 7, 2018

@michaelDCurran michaelDCurran merged commit 0a5c767 into nvaccess:master Dec 7, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@nvaccessAuto nvaccessAuto added this to the 2018.4 milestone Dec 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment