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

Conversation

Projects
None yet
5 participants
@leonardder
Collaborator

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:
    • br(alva):sp1
    • br(alva.bc680):lsp1
      This will also provide a fix for #8074
  • 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 #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:

  • Changes

    • For ALVA BC680 and protocol converter displays, it is now possible to assign different functions to the left and right smart pad, thumb and etouch keys.
    • For ALVA BC6 displays, the key combination sp2+sp3 will now announce the current date and time, whereas sp1+sp2 emulates the Windows key. (#8230)
  • Bug fixes

    • ALVA BC680 braille displays no longer intermittently fail to initialize. (#8106)
    • By default, ALVA BC6 displays will no longer execute emulated system keyboard keys when pressing key combinations involving sp2+sp3 to trigger internal functionality. (#8230)
    • Pressing sp2 on an ALVA BC6 display to emulate the alt key now works as advertised. (#8360)
@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder May 3, 2018

Collaborator

@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.

Collaborator

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

This comment has been minimized.

Show comment
Hide comment
@DrSooom

DrSooom 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 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

This comment has been minimized.

Show comment
Hide comment
@DrSooom

DrSooom 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 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

This comment has been minimized.

Show comment
Hide comment
@DrSooom

DrSooom 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. ;)

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

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder May 4, 2018

Collaborator

@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.

Collaborator

leonardder commented May 4, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@DrSooom

DrSooom 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.

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

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder May 4, 2018

Collaborator

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

Collaborator

leonardder commented May 4, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@DrSooom

DrSooom 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.

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.

@leonardder leonardder requested a review from michaelDCurran Jun 19, 2018

@michaelDCurran michaelDCurran merged commit 66186bf into nvaccess:master Jul 17, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@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