Process injected mouse events by LeonarddeR · Pull Request #8459 · nvaccess/nvda

LeonarddeR · 2018-06-28T13:12:56Z

Link to issue number:

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:

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.
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. (Let NVDA process injected mouse events if desired #8452)
  - This will allow NVDA to track the mouse when a system is controlled remotely using TeamViewer or other remote control software.

feerrenrut

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

feerrenrut · 2018-07-02T10:29:12Z

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


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

feerrenrut · 2018-07-02T10:35:39Z


+		# Translators: This is the label for a checkbox in the
+		# mouse settings panel.
+		handleInjectedMouseControlText = _("Handle mouse control from other &applications")


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

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

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"?

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"

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?

feerrenrut · 2018-07-02T10:35:50Z

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


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.

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

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.

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?

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 · 2018-07-30T18:13:27Z

@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 · 2018-07-30T18:17:56Z

Hi, last time I checked, wx.Yield in wxPython 4 calls wx.AppConsole.Yield. I, @jcsteh, and wxPython developer had a discussion about this a while back. Thanks. From: Leonard de Ruijter <notifications@github.com> Sent: Monday, July 30, 2018 11:13 AM To: nvaccess/nvda <nvda@noreply.github.com> Cc: Joseph Lee <joseph.lee22590@gmail.com>; Mention <mention@noreply.github.com> Subject: Re: [nvaccess/nvda] Process injected mouse events (#8459) @josephsl <https://github.com/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. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8459 (comment)> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AHgLkPfL3Uzr_hIqBQiIsoyw5pfpjDDbks5uL0zJgaJpZM4U7XRW> .

LeonarddeR · 2018-08-27T08:18:11Z

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

LeonarddeR · 2018-12-05T13:16:00Z

@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 · 2018-12-05T22:40:46Z

I can look into this.

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.

michaelDCurran · 2018-12-05T23:00:22Z

 	audioCoordinates_minPitch = integer(default=220)
 	audioCoordinates_maxPitch = integer(default=880)
 	reportMouseShapeChanges = boolean(default=false)
+	ignoreINjectedMouseInput = boolean(default=false)


Should not be a capital N in INjected.

michaelDCurran · 2018-12-05T23:01:22Z


+		# Translators: This is the label for a checkbox in the
+		# mouse settings panel.
+		ignoreINjectedMouseInputText = _("Ignore mouse input from other &applications")


Again INjected should not have capital N

michaelDCurran · 2018-12-05T23:02:05Z

+		# 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"])


Okay, looks like a bad find and replace ;)

michaelDCurran · 2018-12-05T23:05:02Z

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


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.

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

michaelDCurran · 2018-12-07T03:58:38Z

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

LeonarddeR · 2018-12-07T04:48:23Z

Sure!

Leonard de Ruijter added 2 commits June 27, 2018 15:07

Handle mouse control from other applications

9287958

Update user guide

f7024c8

LeonarddeR requested a review from feerrenrut June 28, 2018 13:30

feerrenrut suggested changes Jul 2, 2018

View reviewed changes

Leonard de Ruijter added 2 commits July 2, 2018 13:40

Merge remote-tracking branch 'origin/master' into mouseInjection

1da8016

Make ignoreInjection context managers thread safe

48b3186

LeonarddeR mentioned this pull request Jul 30, 2018

NVDA not reading the objects under the mouse cursor when mouse emulator is used. #7480

Closed

Leonard de Ruijter added 3 commits August 27, 2018 10:22

Merge remote-tracking branch 'origin/master' into mouseINjection

5cb5c10

Review actions, rewording of ignore mouse option

c5ab7a7

Merge remote-tracking branch 'origin/master' into mouseINjection

e9f0abc

michaelDCurran requested changes Dec 5, 2018

View reviewed changes

Leonard de Ruijter added 4 commits December 6, 2018 09:15

Add clarifying comment to KeyboardInputGesture.send

094c11a

Add a new executeMouseEvent wrapper function within mouseHandler

1d7cd65

Fix bad find/replace with an undesired uppercase n in the word injection

7e4ab55

Use the new mouseHandler.executeMouseEvent wrapper

6ba0f57

michaelDCurran approved these changes Dec 7, 2018

View reviewed changes

michaelDCurran added 2 commits December 7, 2018 13:55

Merge branch 'master' into HEAD

e6eea76

Update what's new.

433fb90

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

nvaccessAuto added this to the 2018.4 milestone Dec 7, 2018

LeonarddeR added the BabbageWork Pull requests filed on behalf of Babbage B.V. label Oct 11, 2019

Uh oh!

Conversation

LeonarddeR commented Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing performed:

Known issues with pull request:

Change log entry:

Uh oh!

feerrenrut left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LeonarddeR commented Jul 30, 2018

Uh oh!

josephsl commented Jul 30, 2018 via email

Uh oh!

LeonarddeR commented Aug 27, 2018

Uh oh!

LeonarddeR commented Dec 5, 2018

Uh oh!

michaelDCurran commented Dec 5, 2018 via email

Uh oh!

michaelDCurran left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelDCurran commented Dec 7, 2018

Uh oh!

LeonarddeR commented Dec 7, 2018 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

LeonarddeR commented Jun 28, 2018 •

edited

Loading