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

Several fixes to the ALVA driver #8230

Merged
merged 12 commits into from Jul 17, 2018
Merged

Conversation

LeonarddeR
Copy link
Collaborator

@LeonarddeR LeonarddeR commented May 3, 2018

Link to issue number:

Fixes #8106
Fixes #8074
Fixes #8360

Summary of the issue:

  • Alva Satellite has 2 Satellite key pads, one on the leftside of display and one on the rightside with 6 keys on each key pad, but NVDA treats them as one key pad. This also applies to the BC680 smart pad
  • Some key combinations on ALVA displays trigger internal functionality (e.g. they open the menu). These keys ought to be ignored by NVDA.
  • In some cases, NVDA doesn't recognize a connected BC680 device
  • Using combined emulated keyboard gestures results into an error sound when one of the emulated gestures is removed (i.e. set to None in gestures.ini).

Description of how this pull request fixes the issue:

  • The driver now differentiates between the two groups of smart pad and etouch keys at the model level, while maintaining backwards compatibility with old gesture assignments. For example, the left sp1 on a bc680 has two identifiers:
  • Gestures that activate internal functions are now forcefully bound to None
  • When initializing the driver for an HID display, an extra check has been implemented to make sure that the proper number of cells is used for a device.
  • When checking for combined emulated gestures, make sure that we only deal with non None scripts. Note that this is only a partial fix in the sense that it does not fix Combining emulated keys via a braille display ignores "gestures.ini" #8149. I'll file a separate pr for that issue. I propose to leave this fix out of the bugfixes section for now, as it might cause wrong expectations.

Testing performed:

Several tests by @DrSooom as noted in #8106

Known issues with pull request:

None

Change log entry:

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented May 3, 2018

@DrSooom: just before I ask for review by @michaelDCurran, could you please check whether this try build works as advertised in the pr description? Note that this does not contain fixes for #8149.

@DrSooom
Copy link

DrSooom commented May 3, 2018

File: brailleDisplayDrivers/alva.py

  • Line 417: "kb:alt": ("br(alva):sp2",),
  • Line 435: "kb:alt": ("br(alva):alt",),

This maybe could fail. Please change it to the following:

  • Line 417: "kb:alt": ("br(alva):sp2","br(alva):alt",),
  • Delete line 435.

@DrSooom
Copy link

DrSooom commented May 3, 2018

  1. Well, firstly I was right regarding to the emulated ALT key - it isn't allocated to SP2 by default.
  2. In the Input Help Mode NVDA still speaks "t1" for "lt1" and "rt1" on the ALVA BC680.
  3. "br(alva.bc680):sp1" is shown in the Input Gestures window but now have no effects. That means that all commands has to be allocated to the new l- and r-keys.
  4. Is it possible to implement a replacement command for the "gestures.ini" after the first start of NVDA (installed and portable), so all non-l/r-braille-keys will automatically be changed from "br(alva.bc680):t*" to "br(alva.bc680):rt*". … Ah, no, forget this, because there are too many combinations so the replacement routine maybe will just destroy the whole "gestures.ini". All users of NVDA 2018.1 and 2018.1.1 which have allocated commands to "br(alva.bc680):*" will have to allocate all again.
  5. Or is it possible that NVDA interprets "br(alva.bc680):t1" by default as "br(alva.bc680):lt1". Then all allocations which were done before will still work in 2018.2 and higher but only on the left section on the ALVA BC680. If the user also wanted that it works also on the right section he/she has to allocated it again. That might be a better solution.
  6. Well, and in the end initialling the ALVA BC680 worked correctly without any issues. (Braille split mode was not tested.)

@DrSooom
Copy link

DrSooom commented May 3, 2018

  1. Quick test: Correct recognition of the 15 etouch combos and the 15 SmartPad combos in the Input Help Mode. I didn't test all 31 Thumb combos but most of them should work as well as expected. Well, and pressing SP1+SP2+SP3+SP4 on the left and on the right section at the same time is also correctly recognized by NVDA - all 8 keys are spoken. Testing this was a little bit tricky. ;)

@LeonarddeR
Copy link
Collaborator Author

@DrSooom commented on 3 May 2018, 18:48 CEST:

  1. Is it possible to implement a replacement command for the "gestures.ini" after the first start of NVDA (installed and portable), so all non-l/r-braille-keys will automatically be changed from "br(alva.bc680):t*" to "br(alva.bc680):rt*". … Ah, no, forget this, because there are too many combinations so the replacement routine maybe will just destroy the whole "gestures.ini". All users of NVDA 2018.1 and 2018.1.1 which have allocated commands to "br(alva.bc680):*" will have to allocate all again.
  2. Or is it possible that NVDA interprets "br(alva.bc680):t1" by default as "br(alva.bc680):lt1". Then all allocations which were done before will still work in 2018.2 and higher but only on the left section on the ALVA BC680. If the user also wanted that it works also on the right section he/she has to allocated it again. That might be a better solution.

This is really a bit out of scope for this issue. Note that you should actually only create bindings for model specific gestures if you have multiple models and you really believe that assignments between models should differ. For example, Handy Tech and Hims devices have some key assignments that differ across models by default.

The general advise is to assign gestures to non model specific identifiers, i.e. the ones that start with alva: not the ones starting with alva:bc680.

@DrSooom
Copy link

DrSooom commented May 4, 2018

@LeonarddeR: As long as bug #8108 isn't solved it isn't possible to see in the Input Gestures window if a command is allocated with the obsolete br(alvabc6)- or with the new br(alva)-keys. So at the moment the only solution is to set up device specific hotkeys to see a different in the Input Gestures window. And no, deleting the complete "gestures.ini" is no solution. ;)

  1. Because I forgot this yesterday: The five non-allocated combos don't work as expected – it still combining Win and UpArrow for SP2+SP3+SPUp. None = br(alva.bc680):lsp2+lsp3+lspUp in the "gestures.ini" also didn't helped as well as None = br(alva):sp2+sp3+spUp. But None = br(alva):sp2+sp3 worked as expected - disables the emulating WIN key correctly. Seems we have to null those seven braille hotkeys completely.
    Hmm… dateTime = br(alva):sp2+sp3+spup is working correctly. And None = br(alva):sp2+sp3+spup? No. "spup" and "spUp" makes no different. Well, we could also say kb:control = br(alva):sp2+sp3+spUp. This works fine and makes in the most cases no damages.
    And sadly I just now recognized that you forgot the quotes in line 437 before and after the word "None". Maybe the quotes could solve this issue. Otherwise we just have to null it or allocate it with the emulated CTRL key as a workaround.
  2. And "br(alva):sp2+sp2" is now a valid value.
  • dateTime = br(alva):sp2
    The left and the right SP2 has the same command, but both SP2 at the same time has no command. It doesn't cares if the left or right SP2 was pressed in the Input Gestures window. this worked as expected.
  • dateTime = br(alva):sp2 and say_battery_status = br(alva):sp2+sp2
    The left and the right SP2 still has the same command, but both SP2 at the same time has now the other command. this also worked as expected.

@LeonarddeR
Copy link
Collaborator Author

@DrSooom commented on 4 mei 2018 15:44 CEST:

And no, deleting the complete "gestures.ini" is no solution. ;)

I'm afraid that at some point, there isn't much else for you to do other than that, since a gestures.ini can get quite dirty when you add and remove gestures for different driver many times.

And sadly I just now recognized that you forgot the quotes in line 327 before and after the word "None". Maybe the quotes could solve this issue.

This is intentional. "None" just translates to None in inputCore.py at line 282.

Looking at the code again, I see why assigning all these combinations to None doesn't make sense, because NVDA doesn't differenciate between forcefully disabled gestures and gestures that are just not in use.

I guess in this case, it is the safest to take away the sp2+sp3 assignment to leftWindows. When we do that, sp2+sp3 translates to alt+escape, and sp2+sp3+whatever else won't be in use.

Note that there are more drivers suffering from the issue that some keys are processed by NVDA while activating internal device functionality.

@dkager: What do you think about removing the sp2+sp3 to left windows assignment altogether?

@DrSooom
Copy link

DrSooom commented May 4, 2018

@LeonarddeR: You can also change the alva.py as follows:

  • line 425: "dateTime": ("br(alva):sp1+sp2",),
    To: "dateTime": ("br(alva):sp2+sp3",),
  • line 429: "kb:windows": ("br(alva):sp2+sp3","br(alva):windows",),
    To: "kb:windows": ("br(alva):sp1+sp2","br(alva):windows",),

Edit: And change the following line as follows:

  • line 435: "kb:control": ("br(alva):control",),
    To: "kb:control": ("br(alva):control","br(alva):dot1+dot3+dot4+space","br(alva):dot1+dot3+dot4+dot5+space",),
  • Delete the lines from 436 to 445.

if isNoBC640:
doubledKeyCount = DOUBLED_KEY_COUNTS.get(group)
if doubledKeyCount:
keyName = ("r" if number>=doubledKeyCount else "l") + keyName
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add spaces around >=.

@@ -429,27 +451,39 @@ class InputGesture(braille.BrailleDisplayGesture, brailleInput.BrailleInputGestu

def __init__(self, model, keys, brailleInput=False):
super(InputGesture, self).__init__()
isNoBC640 = model != ALVA_MODEL_IDS[0x40]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you use a constant in place of 0x40?

dkager
dkager previously approved these changes Jun 16, 2018
michaelDCurran
michaelDCurran previously approved these changes Jun 27, 2018
@michaelDCurran michaelDCurran merged commit 66186bf into nvaccess:master Jul 17, 2018
@nvaccessAuto nvaccessAuto added this to the 2018.3 milestone Jul 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment