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

BrailleNote: support for QT and Apex BT scroll wheel #6316

Merged
merged 16 commits into from Jun 13, 2018

Conversation

Projects
None yet
7 participants
@josephsl
Copy link
Collaborator

josephsl commented Aug 30, 2016

This pull request addresses the following issues:

  • #5992: Support for Apex BT's scroll wheel.
  • #5993: Support for BraileNote QT (basic mode) when used as a braille display with NVDA.

All changes are documented in the user guide.

Suggested what's new:
Category: New features
Added support for BrailleNote QT and Apex BT's scroll wheel when BrailleNote is used as a braille display with NVDA.

Thanks.

josephsl added some commits May 24, 2016

BrailleNote: Various BT and QT commands now work correctly. re #5992.
Somehow, forgot to check that Rui used a tuple to specify key assignments. Now utilized properly to assign multiple gestures to a command. Also took this time to add codes for various QT keys so NVDA can properly recognize them.
@jcsteh

This comment has been minimized.

Copy link
Contributor

jcsteh commented Sep 21, 2016

Thanks for your work. I have a few overall questions/concerns about this first before I do a formal review.

Keyboard Emulation

Is your intention to support keyboard emulation, but you weren't able to manage this yet? Or is there any reason you don't want to support this? Supporting that will require a change to the approach you're using. I think it might be worth considering that now, rather than having to do a major refactor later. Following are some thoughts on that.

Rather than emitting BrailleNote gestures and mapping them to key presses, I think it'd be simpler to just emit most keys as KeyboardInputGestures. The QT keys are actually Windows vk codes. So, for most key presses, you would do the following:

  1. Map the QT modifiers to Windows vk codes; e.g. control to VK_CONTROL.
  2. Instantiate a KeyboardInputGesture. Some notes on the arguments:
    • modifiers: The vk codes you mapped in step 1.
    • vkCode: The key code you got from the BrailleNote.
    • scanCode: None.
    • isExtended: Normally True, unless the vk code is 0xD (enter), in which case False.
  3. Call inputCore.manager.emulateGesture with this gesture.

For most keys involving the QT function or read modifiers, you'd still want to produce BrailleNote specific gestures as you do now, as these are specific to the BrailleNote. To convert the vk code to a name, you can use KeyboardInputGesture.getVkName.

You might want to map control+read to the Windows key and then just treat it as a KeyboardInputGesture as above.

Handling Key Packets

At present, you read the extra bytes for QT_MOD_TAG in _readKeys. I suggest moving this to _readPacket, as this is where "reading" is done. Also, data is really just a second argument, so perhaps call it arg2.

@josephsl

This comment has been minimized.

Copy link
Collaborator Author

josephsl commented Sep 21, 2016

Hi,

  1. When to read packet: I'll do some experiments this weekend to see what's possible and moving the parsing routine to readPacket function.
  2. QT emulation: what's making this confusing is the fact that Humanware has implemented PC keyboard emulation mode. Effectively, a QT user can switch between standard terminal mode (the one currently implemented) and PC keyboard mode where some combinations map directly to key presses.
  3. QT terminal commands: My intention is to provide similar experience to that of BT users, but considering that people would like to type on QT keyboard and to serve as an example for other QWERTY keyboard notetakers, I'd be happy to let the branch support suggestions as outlined above (scan code/VK code entry).

Thanks.

@jcsteh

This comment has been minimized.

Copy link
Contributor

jcsteh commented Sep 21, 2016

When to read packet: I'll do some experiments this weekend to see what's possible and moving the parsing routine to readPacket function.

I'd have readPacket return three values, with the third being None for most commands except the QT modifier command.

QT emulation: what's making this confusing is the fact that Humanware has implemented PC keyboard emulation mode. Effectively, a QT user can switch between standard terminal mode (the one currently implemented) and PC keyboard mode where some combinations map directly to key presses.

Yes, control mode and input mode according to the protocol documentation. I don't quite understand the point of input mode, though, because control mode also provides key presses. if anything, control mode is actually more "raw"; it gives you real modifiers. From what I've read, Window-Eyes allows QWERTY input in both, whereas JAWS only allows QWERTY input in input mode.

  1. QT terminal commands: My intention is to provide similar experience to that of BT users, but considering that people would like to type on QT keyboard and to serve as an example for other QWERTY keyboard notetakers, I'd be happy to let the branch support suggestions as outlined above (scan code/VK code entry).

I wouldn't worry about serving as an example. If you don't feel this is appropriate, don't do it; it was just a suggestion because I thought people might like to be able to type. Different QWERTY notetakers do things differently anyway. For example, Papenmeier actually passes entirely raw key presses, right down to key ups and key downs for modifiers, whereas BrailleNote does special handling for modifiers.

josephsl added some commits Sep 23, 2016

BrailleNote QT: handle QT input directly from read packet function. re
…#5992

Comment from Jamie Teh (Nv Access): there's no need to call read packet again if we're dealing with QT, so handle this from read packet directly. Thus readPacket returns three things: command, arg (dot mask on BT/command for QT), arg2 (char from QT).
BrailleNote QT: QT key parser refactor.
Instead of relying on a dedicated QT key parser, handle parsing directly in readPacket. Also simplifies gesture ID assignment, as the packet will tell the gesture constructor as to what key it is.
@jcsteh
Copy link
Contributor

jcsteh left a comment

Thanks!

Did you decide against doing QWERTY emulation and mapping vk codes using the existing KeyboardInputGesture stuff as I suggested? If so, that's fine, but be aware that emulation will require a significant refactor later.

if command == QT_MOD_TAG:
key = self._serial.read(2)[-1]
arg2 = _qtKeys[ord(key)] if ord(key) in _qtKeys else key
else:

This comment has been minimized.

@jcsteh

jcsteh Sep 27, 2016

Contributor

You could simplify this to: arg2 = _qtKeys.get(ord(key), key)


# Data is invoked if we're dealing with a BrailleNote QT.
def _dispatch(self, command, arg, data=None):
def _dispatch(self, command, arg, arg2=None):

This comment has been minimized.

@jcsteh

jcsteh Sep 27, 2016

Contributor

data -> arg2. Also, maybe passed instead of invoked?

if self.qt:
self.id = qtData if qtMod == 0 else "+".join(("+".join(names), qtData))
else:

This comment has been minimized.

@jcsteh

jcsteh Sep 27, 2016

Contributor

Gesture identifiers actually must be in Python set order; you can't reorder things. Otherwise, they might not match gesture maps. If you want to reorder them for display reasons, you will need to override displayName and logIdentifier. If you want to do that, you'll need a helper method which all of them call which returns the names in the correct order (list not set).

@@ -2093,6 +2100,39 @@ Please check your BrailleNote's documentation to find where these keys are locat
| Windows key | space+dot2+dot4+dot5+dot6 (space+w) |
| Alt key | space+dot1+dot3+dot4 (space+m) |
| Toggle input help | space+dot2+dot3+dot6 (space+lower h) |

Following are commands assigned to BrailleNote QT.
%kc:beginInclude

This comment has been minimized.

@jcsteh

jcsteh Sep 27, 2016

Contributor
  1. A kc include block was already started earlier.
  2. You probably want to include the sentence explaining the purpose of each table in the kc document anyway, so just leave the kc include block open from before.
  3. Should the braille dot commands be split out of the main commands into a separate table, since I assume you can't execute those on a QT?

This comment has been minimized.

@josephsl

josephsl Sep 27, 2016

Author Collaborator

You can ask QT to emulate braille dots by pressing a key (it's a toggle). I can clarify this.

| Toggle input help | read+1 |

Following are commands assigned to the scroll wheel:
%kc:beginInclude

This comment has been minimized.

@jcsteh

jcsteh Sep 27, 2016

Contributor

See above re kc include block.

%kc:beginInclude
|| Name | Key |
| NvDA menu | read+n |
| Up arrow key | up arrow |

This comment has been minimized.

@jcsteh

jcsteh Sep 27, 2016

Contributor

The convention in NVDA is normally to write keys in camel case; e.g. downArrow instead of down arrow.

josephsl added some commits Sep 27, 2016

BrailleNote QT: comment text change, correct ticket, simplified dicti…
…onary key lookup. re #5993

A tip from Jamie Teh (NV Access0: use dict.get to simplify the turnary operation. Also, BNQT ticket is 5993, not 5992, and correct comment text.
User guide: cosmetics. re #5992
Review from Jamie Teh (NV Access):
* QT can emulate a BrailleNote BT, hence clarify this.
* Removed redundant kcinclude statement.
BrailleNote QT: use set instead of worrying too much about formatting…
… gesture identifier right. #5993

Comment from Jamie Teh (NV Access): use sets instead of turning that into a list just for formatting purposes (and turns out this is unnecessary).
@leonardder

This comment has been minimized.

Copy link
Collaborator

leonardder commented Nov 8, 2017

This has now merge conflicts since this driver is hwIo based.

@josephsl

This comment has been minimized.

Copy link
Collaborator Author

josephsl commented Nov 8, 2017

josephsl added some commits Nov 17, 2017

Merge branch 'master' into i5992
# Conflicts:
#	source/appModules/win32calc.py
#	source/brailleDisplayDrivers/brailleNote.py

@feerrenrut feerrenrut requested a review from leonardder Apr 11, 2018

@feerrenrut

This comment has been minimized.

Copy link
Contributor

feerrenrut commented Apr 11, 2018

@leonardder Would you mind reviewing this when you have a chance?

@leonardder

This comment has been minimized.

Copy link
Collaborator

leonardder commented Apr 11, 2018

@josephsl: Could you please resolve another merge conflict?

@leonardder
Copy link
Collaborator

leonardder left a comment

Looks very good overall. Apart from the inline comments, could you please also revert the mode switch that occurred for appModules/win32calc.py

@@ -74,13 +84,48 @@
0 : "space"
}

# Scroll wheel components (Apex BT)
_scrWheel = ("wcounterclockwise", "wclockwise", "wup", "wdown", "wleft", "wright", "wcenter")

This comment has been minimized.

@leonardder

leonardder Apr 11, 2018

Collaborator

To improve consistency, could you rename these to wCounterclockwise, wClockwise, wUp, etc?

186:"semi",
187:"equals",
188:"comma",
189:"hyphen",

This comment has been minimized.

@leonardder

leonardder Apr 11, 2018

Collaborator

I'd call this dash

187:"equals",
188:"comma",
189:"hyphen",
190:"period",

This comment has been minimized.

@leonardder

leonardder Apr 11, 2018

Collaborator

I'd call this dot.

189:"hyphen",
190:"period",
191:"slash",
192:"graav",

This comment has been minimized.

@leonardder

leonardder Apr 11, 2018

Collaborator

I'd call this grave

# Force dot8 here, although it should be already there
arg |= DOT_8
gesture = InputGesture(dots=arg, space=space)
elif command == QT_MOD_TAG:
# BrailleNote QT
gesture = InputGesture(qtMod=arg, qtData=arg2)

This comment has been minimized.

@leonardder

leonardder Apr 11, 2018

Collaborator

Have you actually considered making this a keyboardHandler.KeyboardInputGesture?

@josephsl

This comment has been minimized.

Copy link
Collaborator Author

josephsl commented Apr 11, 2018

@josephsl

This comment has been minimized.

Copy link
Collaborator Author

josephsl commented Apr 11, 2018

josephsl added some commits Apr 12, 2018

Merge branch 'master' into i5992
# Conflicts:
#	user_docs/en/userGuide.t2t
BrailleNote: copyright year update, QT key name changes. Re #5992.
Commented by Leonard de Ruijter: rename QT key names.
Update copyright years.
@leonardder

This comment has been minimized.

Copy link
Collaborator

leonardder commented Apr 13, 2018

@josephsl: Were you also intending to fix de mode change for win32calc.py?

@josephsl

This comment has been minimized.

Copy link
Collaborator Author

josephsl commented Apr 13, 2018

leonardder added a commit that referenced this pull request Apr 30, 2018

@derekriemer

This comment has been minimized.

Copy link
Collaborator

derekriemer commented Jun 10, 2018

What's the status of this?

@josephsl

This comment has been minimized.

Copy link
Collaborator Author

josephsl commented Jun 10, 2018

@michaelDCurran michaelDCurran merged commit 30cd6b8 into nvaccess:master Jun 13, 2018

@nvaccessAuto nvaccessAuto added this to the 2018.3 milestone Jun 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.