-
-
Notifications
You must be signed in to change notification settings - Fork 634
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
Improve keyboard support for some terminal programs #9915
Improve keyboard support for some terminal programs #9915
Conversation
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 |
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 |
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. |
+1 |
…erminal doesn't appear in the MRO twice.
Right, but how can we still keep the behavior implemented by, for example, For now, I've made the |
@codeofdusk commented on 13 Jul 2019, 09:16 CEST:
Is it really True that a class can be in the MRO twice? Shouldn't As far as I can see, this should work:
The mro of a legacy console should end up in something like this:
|
from NVDAObjects.behaviors import TerminalKeyboardSupport | ||
clsList.append(TerminalKeyboardSupport) | ||
clsList.append(WinConsole) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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`
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.
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. |
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>.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @codeofdusk
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.
|
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.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this looks pretty good now, thanks. Sorry for the request of additional changes though.
Co-Authored-By: Leonard de Ruijter <leonardder@users.noreply.github.com>
@LeonarddeR, can you please update your review when you have time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks @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:
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.Change log entry:
== Bug Fixes ==