Improve keyboard support for some terminal programs

[codeofdusk]

Link to issue number:

Fixes #513, #1348. Related to #9614.

Summary of the issue:

NVDA fails to announce typed characters and/or words in Mintty, and spells output close to the caret in legacy Windows consoles.

Description of how this pull request fixes the issue:

This PR factors out much of the code for handling typed characters in UIA consoles into a new NVDAObjects.behaviors.TerminalWithoutTypedCharDetection class. The class is now used in Mintty (PuTTY, Git Bash) and legacy Windows consoles on Windows 10 version 1607 and later. In legacy Windows consoles, the old keyboard handling code is disabled when the class is in use, and the new support can be disabled in the advanced preferences in case it is incompatible with some programs or if suppression of passwords is critical.

Testing performed:

Tested typing text in PuTTY on Windows 10 version 1607 and 1903 and found that NVDA reported typed characters and words when the corresponding settings were enabled. Also found that typed characters and words were reported in legacy Windows consoles with the new keyboard support enabled.

Known issues with pull request:

• Since legacy Windows consoles fire textChange rather slowly, offscreen characters (such as passwords) are always reported. Users may disable "speak typed characters" and/or "speak typed words" (using the existing scripts) when entering passwords to suppress this output.
• On Windows 10 version 1607 with the new keyboard support enabled, spurious characters are reported when the dead key (if available) is pressed.

Change log entry:

== Bug Fixes ==

• On Windows 10 version 1607 and later, typed text is now reported in Mintty. (NVDA doesn't announce typed characters in putty #1348)
• On Windows 10 version 1607 and later, output in the Windows Console that appears close to the caret is no longer spelled out. (Some output in command consoels spelled with speak typed characters enabled #513)

[LeonarddeR]

Make sure not to include both TerminalKeyboardSUpport and Terminal in a class list. It won't hurt, but it is not necessary as TerminalKeyboardSUpport inherrits from Terminal.

I also consider the current class name a bit vague. May be TerminalWithoutTypedCharacterDetection covers it better?

[codeofdusk]

Make sure not to include both TerminalKeyboardSUpport and Terminal in a class list. It won't hurt, but it is not necessary as TerminalKeyboardSUpport inherrits from Terminal.

I also consider the current class name a bit vague. May be TerminalWithoutTypedCharacterDetection covers it better?

@LeonarddeR My intent was to make the TerminalKeyboardSupport class a mixin (i.e. it extends the functionality of the Terminal class). Maybe should I make it inherit from object instead?

[LeonarddeR]

In my opinion, using a mixin is only valid when you are adding additional methods without overriding other ones. In this mixin, you are calling super, so that really relies on it being a subclass of Terminal.

[codeofdusk]

In my opinion, using a mixin is only valid when you are adding additional methods without overriding other ones. In this mixin, you are calling super, so that really relies on it being a subclass of Terminal.

Yes, but we only want to enable the keyboard support for some Terminal objects. How would you suggest removing the parent of WinConsole (i.e. Terminal) to add, for example, a TerminalWithKeyboardSupport class that inherits from Terminal (so it isn't in the MRO twice)?

[LeonarddeR]

Yes, but we only want to enable the keyboard support for some Terminal objects. How would you suggest removing the parent of WinConsole (i.e. Terminal) to add, for example, a TerminalWithKeyboardSupport class that inherits from Terminal (so it isn't in the MRO twice)?

Yes. You can make TerminalWithKeyboardSupport inherrit from Terminal (I think it does already), and then everywhere you want to use TerminalWithKeyboardSupport instead of Terminal, just use it and avoid using Terminal.

[JulienCochuyt]

I also consider the current class name a bit vague. May be TerminalWithoutTypedCharacterDetection covers it better?

+1

[codeofdusk]

Yes, but we only want to enable the keyboard support for some Terminal objects. How would you suggest removing the parent of WinConsole (i.e. Terminal) to add, for example, a TerminalWithKeyboardSupport class that inherits from Terminal (so it isn't in the MRO twice)?

Yes. You can make TerminalWithKeyboardSupport inherrit from Terminal (I think it does already), and then everywhere you want to use TerminalWithKeyboardSupport instead of Terminal, just use it and avoid using Terminal.

Right, but how can we still keep the behavior implemented by, for example, NVDAObjects.IAccessible.WinConsole.findExtraOverlayClasses? (i.e. some terminal objects need the keyboard support, others don't, and we don't know which until runtime)

For now, I've made the TerminalKeyboardSupport mixin inherit from object (so Terminal doesn't appear in the MRO twice).

[LeonarddeR]

@codeofdusk commented on 13 Jul 2019, 09:16 CEST:

For now, I've made the TerminalKeyboardSupport mixin inherit from object (so Terminal doesn't appear in the MRO twice).

Is it really True that a class can be in the MRO twice? Shouldn't NVDAObjects.DynamicNVDAObjectType.__call__ deal with this?

As far as I can see, this should work:

1. Make TerminalKeyboardSupport inherrit from Terminal
2. In Putty, do not add Terminal to the clsList

The mro of a legacy console should end up in something like this:

<class 'NVDAObjects.IAccessible.winConsole.WinConsole'>,
<class 'NVDAObjects.behaviors.TerminalKeyboardSupport'>,
<class 'NVDAObjects.window.winConsole.WinConsole'>, 
<class 'NVDAObjects.behaviors.Terminal'>,
<class 'NVDAObjects.behaviors.LiveText'>, 
<class 'NVDAObjects.behaviors.EditableTextWithoutAutoSelectDetection'>,
<class 'editableText.EditableTextWithoutAutoSelectDetection'>, 
<class 'NVDAObjects.behaviors.EditableText'>,
<class 'editableText.EditableText'>, 
<class 'NVDAObjects.IAccessible.IAccessible'>, 
<class 'NVDAObjects.window.Window'>,
<class 'NVDAObjects.NVDAObject'>, 
<class 'documentBase.TextContainerObject'>, 
<class 'baseObject.ScriptableObject'>, 
<class 'baseObject.AutoPropertyObject'>, 
<class 'object'>

[LeonarddeR]

I think this might be a bit ambiguous. is this NVDAObjects.IAccessible.winConsole.WinConsole or NVDAObjects.window.winConsole.WinConsole. I'm sure it will be the former, but a linter might complain that you're importing WinConsole and then override it. Please change the import above to something like:
``from ..window.winConsole import WinConsole as WinConsoleWindow`

[LeonarddeR]

I still consider TerminalWithKeyboardSupport. a bit vague. e.g. I assume every Terminal has keyboard support.

Every class name in Behaviors seems to be pretty explanatory about what the class does, e.g.

• EditableTextWithAutoSelectDetection suggests that the object is able to detect selection changes
• EditableTextWithoutAutoSelectDetection suggests that the object is not able to detect selection changes, and therefore the mixin is responsible for reporting selection.

That's why I came up with TerminalWithoutTypedCharacterDetection. It follows the naming scheme of the SelectDetection classes, telling the user at one glance that the mixin aids in detecting typed characters.

[codeofdusk]

Ah, that name makes a bit more sense now.

…

On 15 Jul 2019, at 13:18, Leonard de Ruijter ***@***.***> wrote: I still consider TerminalWithKeyboardSupport. a bit vague. e.g. I assume every Terminal has keyboard support. Every class name in Behaviors seems to be pretty explanatory about what the class does, e.g. EditableTextWithAutoSelectDetection suggests that the object is able to detect selection changes EditableTextWithoutAutoSelectDetection suggests that the object is not able to detect selection changes, and therefore the mixin is responsible for reporting selection. That's why I came up with TerminalWithoutTypedCharacterDetection. It follows the naming scheme of the SelectDetection classes, telling the user at one glance that the mixin aids in detecting typed characters. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#9915?email_source=notifications&email_token=AAS4QS4EHCA3322J2LQLX6DP7QCCVA5CNFSM4IA5JM4KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZ4XOQA#issuecomment-511276864>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAS4QSZFT5EMJQGYQA3UJPDP7QCCVANCNFSM4IA5JM4A>.

[feerrenrut]

Thanks @codeofdusk

[LeonarddeR]

Could I have a quick look before merging?

…

Op 31 jul. 2019 om 12:47 heeft Reef Turner ***@***.***> het volgende geschreven: @feerrenrut approved this pull request. Thanks @codeofdusk — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[codeofdusk]

Sure!

…

Sent from my iPhone

On Jul 31, 2019, at 06:49, Leonard de Ruijter ***@***.***> wrote: Could I have a quick look before merging? > Op 31 jul. 2019 om 12:47 heeft Reef Turner ***@***.***> het volgende geschreven: > > @feerrenrut approved this pull request. > > Thanks @codeofdusk > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub, or mute the thread. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[LeonarddeR]

Overall, this looks pretty good now, thanks. Sorry for the request of additional changes though.

[feerrenrut]

@LeonarddeR, can you please update your review when you have time.

[LeonarddeR]

Awesome, thanks @codeofdusk!