Improve handling of keyboard emulation gestures and testing of braille display gesture map integrity by LeonarddeR · Pull Request #8240 · nvaccess/nvda

LeonarddeR · 2018-05-07T09:35:46Z

Link to issue number:

This pr also contains code from #7784. I consider this pr more important than #7784 and will revert duplicate code from #7784

Summary of the issue:

When using combined emulated system keyboard keys (i.e. including modifiers), the user gesture map is ignored. See Combining emulated keys via a braille display ignores "gestures.ini" #8149 for some examples.
When creating an input gesture from a key name (e.g. when emulating gestures), NVDA treats the last key in the sequence as the main key and the other keys as modifiers. This way of creating a keyboard input gesture (e.g. for emulated keys) is unreliable.

Description of how this pull request fixes the issue:

The user gesture map is now properly handled when looking up braille display modifiers and combined emulated system keyboard keys. I wrote unit tests to make sure the new code behaves as expected.
keyboardHandler.KeyboardInputGesture.fromName has been improved to support keys in indeterminate error. It will raise a ValueError when a key is malformed (i.e. kb:nothing or kb:a+s+x+control.

Other enhancements and improvements

The test module for brailleDisplayDrivers has been removed. Its tests are covered in the new test_inputCore module.
test_inputCore is now devoted to test gesture map integrity. It not only tests the validity of braille display gesture identifiers, but also tests whether all GlobalCommands scripts in every driver's gesture map are bound to the expected gesture. For example, the tests will fail if one would assign a gesture to a non-existent script. It will also test the emulated system keyboard keys framework, even though most of the logic behind that resides in the braille module. Of course, I'm open for any suggestion that might improve the structure of the inputCore tests.
Braille display gesture instances can now be created using a new fromIdentifier class method. this new class method is also available to create keyboard input gestures. It is used within the tests that are outlined above.

Testing performed:

Unit tests.

Known issues with pull request:

Using fromIdentifier to create braille display gestures doesn't support the braille input framework. For example, when pressing dot1 on a hims display, this would map to the identifiers bk:dots, bk:dot1 and br(hims):dot1. However, when creating an input gesture from br(hims):dot1, the braille input identifiers won't be available on that instance. This would require manual implementations of fromModifier for every specific driver, which is way beyond the scope of this pr.

Change log entry:

Bug fixes
- When combining emulated system keyboard keys from a braille display, manually changed or removed assignments will no longer be ignored. (Combining emulated keys via a braille display ignores "gestures.ini" #8149)
Changes for developers
- You can now create instances of braille.BrailleDisplayGesture using the new fromIdentifier class method.

…t gestures that have been disabled in the user gesture map

…y, thereby fixing several malformed gesture to script assignments

…w use fromIdentifier

…creating an emulated gesture

feerrenrut

I'm not sure that I fully understand this change. It bothers me somewhat that the code for processing gestures (including string manipulations etc) is spread around as it is. I wouldn't suggest it as part of this change, but I think it might be worthwhile considering the abstraction level for gestures. I would like to go over this again.

feerrenrut · 2018-05-14T06:04:56Z

+			for scriptCls, gesture, scriptName in globalMap.getScriptsForAllGestures()
+			if any(gesture.startswith(prefix.lower()) for prefix in prefixes)
+			and scriptCls is globalCommands.GlobalCommands
+			and (scriptName is None or scriptName.startswith("kb"))


The conditions in this block are fairly dense, and not very well explained. I would be all for trying to break this up a little bit. Something I am concerned about here (if I am reading this right), in the old version we took gesture / scriptName if only if scriptName started with "kb". Now we also take it if it is None?

We also take scriptNames when they are None, because the following can happen:

A display driver has br(alva):t3 assigned to control

A user removes this binding from the input gestures dialog

br(alva):t3 is now assigned to None

If we would ignore the None occurrences here, _getModifierGestures would still yield t3 as being assigned to control.

feerrenrut · 2018-05-14T06:06:43Z

+			and (scriptName is None or scriptName.startswith("kb"))
+		}
+		for gesture, scriptName in emuGestureToScriptMap.iteritems():
+			if scriptName:


we accept scriptName when it's none, but then check to ensure it isn't? See my other comment.

We accept the None to filter out the cases where a user has manually overridden an assignment as noted above. However, we only yield the assignments that are currently in effect. As in the example above, the function wouldn't yield anything for the br(alva):t3 key, as it has been re-assigned to None and therefore can be ignored within the scope of modifier gestures.

feerrenrut · 2018-05-14T06:57:21Z

 			# Find a script for L{gestureKeys}.
 			id = "+".join(gestureKeys)
-			fakeGestureIds = [u"br({source}):{id}".format(source=self.source, id=id),]
+			fakeGestureIds = [


What is a "fake gesture"?

I'm not very happy with this name either. I renamed fakeGestureId to gestureIdWithoutModifiers and added an explanation in comments, all in non committed code as of yet.

feerrenrut · 2018-05-14T07:00:36Z

 			for globalMap in globalMaps:
 				for fakeGestureId in fakeGestureIds:
-					scriptNames.extend(scriptName for cls, scriptName in globalMap.getScriptsForGesture(fakeGestureId.lower()) if scriptName.startswith("kb"))
+					scriptNames.extend(scriptName for cls, scriptName in globalMap.getScriptsForGesture(fakeGestureId) if scriptName is None or scriptName.startswith("kb"))


could you split this onto multiple lines?

feerrenrut · 2018-05-14T07:06:17Z

+					inputCore.normalizeGestureIdentifier(u"br({source}.{model}):{id}".format(source=self.source, model=self.model, id=id))
+				)
 			scriptNames = []
+			# We can't bother about multiple scripts for a gesture, we will just use the first one


I think this comment could be clarified. I think this means
"Gestures may be duplicated across different maps (EG the same gesture is present in both the userGestureMap and the gestureMap. User gestures should override display gestures, so we intentionally order the maps with the most important first when finding matches. Later we just take the first"

feerrenrut · 2018-05-14T07:07:44Z

-				keys=scriptName.split(":")[1]
-			)
-		else:
+			scriptName = scriptNames[0]


Given we are only interested in the first, could we just break out of the loop at line 2264 when we find the first match?

Hmm, instead of extending the scriptNames list, I could imagine the creation of a nested loop, i.e.

for globalMap in globalMaps: for gestureId in gestureIds: for cls, scriptName in globalMap.getScriptsForGesture(gestureId): if scriptName is something: break

However as you know, this only breaks out of the inner most loop. I guess the only way to do this, would be by means of a function. I can declare a local function inside _get_script, but don't know whether that is very ideal.

derekriemer · 2018-05-17T00:40:33Z

-		gestureKeys = set(self.keyNames)
+		# Input gestures are normalized in gesture maps.
+		# Therefore, if we look for possible modifiers in a gesture, we ought to convert the key names to lower case.
+		keyNames = [name.lower() for name in self.keyNames]


Why don't we run this through normalizedGesture?

derekriemer · 2018-05-17T00:46:12Z

@@ -540,15 +547,41 @@ def fromName(cls, name):
 					keys.append((winUser.VK_MENU, False))
 				# Not sure whether we need to support the Hankaku modifier (& 8).


What's this comment for

I have no idea honestly.

LeonarddeR · 2019-04-24T17:11:58Z

It is likely that this needs a significant rewrite/refactor after the python 3 transition. Therefore I'm closing this for now. It is probably best to reopen smaller, more manageable pull requests in the future.

Adriani90 · 2024-05-14T22:20:48Z

@LeonarddeR some work has been done on this during last years, including a fix for #8149.
What needs to be done still to fix #8201?

cc: @burmancomp

Leonard de Ruijter added 9 commits May 7, 2018 11:03

When querying possible modifier gestures for braille displays, respec…

51f4c3c

…t gestures that have been disabled in the user gesture map

Majorly improve keyboardHandler.KeyboardInputGesture.fromName

d2f1e8c

First tests for keyboardHandler

936518e

Add more unit tests and make error generation for fromName more verbose

9a262c6

Remove some accidentally duplicated code

c9e4e55

Add more unit tests and fix behavior

3c7f477

Improved braille display gesture map integrity checking in a major wa…

fcd8691

…y, thereby fixing several malformed gesture to script assignments

Add fromIdentifier method to input gestures

95fd1e8

Get rid of fake InputGesture class in the test suite, since we can no…

9fa30b1

…w use fromIdentifier

LeonarddeR requested a review from feerrenrut May 7, 2018 09:44

Update scriptHandler to use KeyboardInputGesture.fromIdentifier when …

63997fa

…creating an emulated gesture

feerrenrut reviewed May 14, 2018

View reviewed changes

derekriemer reviewed May 17, 2018

View reviewed changes

first review actions

5bb88ba

DrSooom mentioned this pull request Jun 3, 2018

ALVA: SP2 isn't allocated with the emulated ALT key #8360

Closed

LeonarddeR mentioned this pull request Jun 6, 2018

Add automatic background braille display detection #7741

Merged

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

a44f675

LeonarddeR added AfterPython3Transition and removed AfterPython3Transition labels Apr 24, 2019

LeonarddeR closed this Apr 24, 2019

Adriani90 added the Abandoned requested reports or updates are missing since more than 1 year, author or users are not available. label Sep 7, 2023

		@@ -540,15 +547,41 @@ def fromName(cls, name):
		keys.append((winUser.VK_MENU, False))
		# Not sure whether we need to support the Hankaku modifier (& 8).

Uh oh!

Conversation

LeonarddeR commented May 7, 2018

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Other enhancements and improvements

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LeonarddeR commented Apr 24, 2019

Uh oh!

Adriani90 commented May 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants