Prototype for Windows Terminal: Use notifications instead of monitoring for new text

[LeonarddeR]

Note, with many thanks to @codeofdusk

Link to issue number:

Closes #13781

Summary of the issue:

Windows Terminal supports UIA notification events to notify of new text. This is way quicker than the current diffing we use to calculate new text to speak in a terminal.

Description of user facing changes

Note: This is hidden behind a feature flag.
When enabled, this should improve responsivity whereas having minimal user impact. The only thing I noticed is that when typing echo is on, the last typed character/word is spoken after pressing enter, whereas in the old situation, the last typed char/word was always silenced. I'd say this is an improvement.

Description of development approach

Handle the Windows Terminal object as regular Editable Text, i.e. remove the terminal specific stuff from it.

Testing strategy:

Did a short Test in Windows Terminal. Observed that incoming notifications are spoken.

Known issues with pull request:

Character/word echo for passwords doubles as asterisks are written to the terminal and they are picked up by notification events.

Change log entries:

=== New Features ===
- Added an experimental option to leverage the UIA notification support in Windows Terminal to report new or changed text in the terminal, resulting in improved stability and responsivity. (#13781)
 - Consult the user guide for limitations of this experimental option.
 - 
-

=== Changes for Developers ===
- ``NVDAObjects.UIA.winConsoleUIA.WinTerminalUIA`` has been replaced with ``NVDAObjects.UIA.winConsoleUIA._DiffBasedWinTerminalUIA`` and ``NVDAObjects.UIA.winConsoleUIA._NotificationsBasedWinTerminalUIA``. (#14047)

Code Review Checklist:

• Pull Request description:
  • description is up to date
  • change log entries
• Testing:
  • Unit tests
  • System (end to end) tests
  • Manual testing
• API is compatible with existing add-ons.
• Documentation:
  • User Documentation
  • Developer / Technical Documentation
  • Context sensitive help for GUI changes
• UX of all users considered:
  • Speech
  • Braille
  • Low Vision
  • Different web browsers
  • Localization in other languages / culture than English
• Security precautions taken.

[codeofdusk]

Also, if this PR is merged, I'm really thinking it should be gated behind a feature flag. This is a fundamental change to how terminals work in NVDA.

As a note for reviewers, I've written a few thoughts on doing this at #13781 (comment).

[LeonarddeR]

While a fundamental change, it shouldn't have that much user visible differences on the front side. I'm pretty reluctant to introduce a feature flag for every single fundamental change. Feature flags tend to stay for ages without bringing the feature any further.
I'd rather see the approach thoroughly tested before it lands in a release.

[codeofdusk]

Also CCing @carlos-zamora.

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: Unit tests.
• FAIL: Lint check. See test results for more information.
• FAIL: System tests. See test results for more information.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/krojigoeb2bcfqv2/artifacts/output/nvda_snapshot_pr14047-26266,3ed04af5.exe

See test results for failed build of commit 3ed04af591

[mzanm]

I think that some check should be done on the windows terminal version as not all support UIA notifications

[codeofdusk]

I think that some check should be done on the windows terminal version as not all support UIA notifications

No, because:

• With "undocking", the version number isn't always reliable.
• The versions that don't support notifications are no longer supported, and per @DHowett people really shouldn't run them.

[mzanm]

I've noticed that when it announces new text, everything is spoken as one line, so that this code

for i in range(10):
    print(i)

It would read it as 012345... Instead of each one on one line like it should

[codeofdusk]

I've noticed that when it announces new text, everything is spoken as one line

@LeonarddeR I've provided a patched notification handler that should work around this (you'll need to import api, then import config at the top of the file, please place above import ctypes). CC @mazen428

[tspivey]

I tested this briefly.

1. When typing certain punctuation with speak typed characters off (e.g. @#$%), I hear the character. Others don't do it (e.g. ., ,, /). This might be a bug in the terminal.
2. Toggling report dynamic content changes off doesn't stop the notifications from reading. This is sometimes useful.
3. The reading experience isn't that good when reading long text, partially because Windows Terminal has a limit of 1000 characters per notification.
   It's also not guaranteed that you'll get an entire line of text; connection delays might give you half a line no matter how large the notifications are.
   To get around this, notifications can be batched for a short time before speaking them. This won't be perfect, but it should be better than it is now.

[codeofdusk]

2. Toggling report dynamic content changes off doesn't stop the notifications from reading. This is sometimes useful.

Fixed in the suggested change to the UIA notification handler above.

As for the others, I think these are probably terminal issues. I'll ping @carlos-zamora.

[carlos-zamora]

1. When typing certain punctuation with speak typed characters off (e.g. @#$%), I hear the character. Others don't do it (e.g. ., ,, /). This might be a bug in the terminal.

Strange! We shouldn't be doing anything differently, but I'll take a closer look at this later this week.

3. The reading experience isn't that good when reading long text, partially because Windows Terminal has a limit of 1000 characters per notification.
   It's also not guaranteed that you'll get an entire line of text; connection delays might give you half a line no matter how large the notifications are.
   To get around this, notifications can be batched for a short time before speaking them. This won't be perfect, but it should be better than it is now.

I could change this on the Terminal side and make it do less than the limit to fit an entire word/sentence in. But I'm concerned we would see some weird behavior when the newly output text is overwriting existing text or something like that. I think batching the notifications is the right approach here.

Fun fact: @lhecker and I were just discussing batching notifications and the buffer this past Monday haha

[josephsl]

Hi,

Note that the base implementation of UIA notification event handler will ignore background notifications. If it works differently in Windows Terminal object, then this might be something to think about. Also, keep in mind that the nature of UIA notification event is not really that of a dynamic content announcer - it is there to announce accessible event notifications. This is why I intentionally did not tie this setting to dynamic content changes when I first introduced notification event support in 2018, and that is also embedded in Windows App Essentials - to turn off UIA notification via that add-on, one must turn off "report notifications" from NVDA's object presentation settings panel. It is also another reason why I did work on a dedicated setting for UIA notification announcement.

Thanks.

[mzanm]

Wouldn't making this based on EnhancedTermTypedCharSupport, excluding liveText, fix the punctuation issues along with clearing typed characters buffer when pressing tab or enter?

[codeofdusk]

• EnhancedTermTypedCharSupport subclasses LiveText.
• EnhancedTermTypedCharSupport depends on event_textChange for password suppression (in addition to LiveText using it to diff). Registering for this event in terminals introduces serious performance problems (see UIA freezes after a large number of events is received #11002).

[LeonarddeR]

How would we implement batching? Does Terminal know that it is sending a batch? If yes, I think it would help if Terminal communicates in the notification that this is a part of a batch that's not yet finished so NVDA can queue. If that isn't possible, I'm afraid we're getting into business with waiting and timeouts all over again, and I really want to avoid that if possible.

[codeofdusk]

@LeonarddeR Can we find a way not to register for or receive textChange events in wt now that we're not using them? This should seriously improve performance.

[LeonarddeR]

Do you have any proof of that statement (i.e. that it improves performance)? In what circumstances?

Anyhow, you're raising an interesting question. I'm afraid the short answer is no, since we register to text changed notifications in the UIA handler, either globally or locally depending on selective event registration. However, as soon as we register, every text changed notification enters NVDA's python code and therefore can slow things down. I guess we can come up with a clone of AppModule.shouldProcessUIAPropertyChangedEvent, but that's again and again putting the real problem off before us.

[codeofdusk]

Do you have any proof of that statement (i.e. that it improves performance)? In what circumstances?

Yes. In particular, when we're flooded with new text:

1. Run the wtNotifications branch of NVDA.
2. Open a wt instance. Start a Python interpreter, and run the following code: for i in range(10000): print("test")
   • Observe that NVDA reads "test" for a few seconds, then UIA dies.
3. Comment out the mapping UIA.UIA_Text_TextChangedEventId: "textChange" (around line 209 of UIAHandler/__init__.py) and restart NVDA.
4. Repeat step 3.
   • Observe that NVDA remains functional.

[LeonarddeR]

That's pretty disconcerting.
Would you be able to prototype something like AppModule.shouldProcessUIAEvent to see whether that fixes or at least improves it?

[carlos-zamora]

Does Terminal know that it is sending a batch? If yes, I think it would help if Terminal communicates in the notification that this is a part of a batch that's not yet finished so NVDA can queue. If that isn't possible, I'm afraid we're getting into business with waiting and timeouts all over again, and I really want to avoid that if possible.

Terminal won't be able to batch notifications because we get all the text from the shell we're connected to. So we have no knowledge if more text is on the way or not. You could loosely assume that if you received a large amount of text, a bunch more is on the way since you're presumably reading out the content of a large text file. But other than that (and that's a very loose heuristic), we can't really do much.

@lhecker and I chatted a bit more about this earlier today. I've written down some notes in the Terminal repo here: microsoft/terminal#10822 (comment)

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: Unit tests.
• FAIL: Lint check. See test results for more information.
• PASS: System tests (tags: installer NVDA).
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/jgnm632xiewxi05l/artifacts/output/nvda_snapshot_pr14047-26876,a35cfcff.exe
• CI timing (mins):
  INIT 0.0,
  INSTALL_START 0.7,
  INSTALL_END 0.8,
  BUILD_START 0.0,
  BUILD_END 21.6,
  TESTSETUP_START 0.0,
  TESTSETUP_END 0.2,
  TEST_START 0.0,
  TEST_END 13.5,
  FINISH_END 0.2

See test results for failed build of commit a35cfcff40

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: Unit tests.
• FAIL: Lint check. See test results for more information.
• Build execution time has reached the maximum allowed time for your plan (60 minutes).

See test results for failed build of commit 28db16dd22

[Qchristensen]

Userguide changes read well. Thanks!

[seanbudd]

Is there a way to retain backwards compatibility by keeping NVDAObjects.UIA.winConsoleUIA.WinTerminalUIA?

[codeofdusk]

Is there a way to retain backwards compatibility by keeping NVDAObjects.UIA.winConsoleUIA.WinTerminalUIA?

I could make it an alias for the currently default implementation (diff-based for now, then notifications based later), but I don't think this would add very much value. This is an internal class that I highly doubt plugins would use.

[seanbudd]

This is an internal class that I highly doubt plugins would use.

If the new classes are also intended to be internal classes, please prefix them with underscores.
This was marked as part of the public API. According to current policy we aim to keep aliases where there is low ongoing maintenance cost.

[seanbudd]

should these classes be marked as internal?

[codeofdusk]

No, for symmetry with the rest of the module.

[seanbudd]

What do you mean by symmetry?

[codeofdusk]

The other Win*UIA classes in this module don't have underscores.

[seanbudd]

I don't think that it is too late to fix the current practice, if these should be marked as internal, we should mark them.

[codeofdusk]

This would be a significant divergence from other similar classes in NVDAObjects.UIA. A few examples:

• NVDAObjects.UIA.VisualStudio.IntelliSenseList
• NVDAObjects.UIA.wordDocument.UIACustomAttributeID
• NVDAObjects.UIA.wordDocument.ElementsListDialog

[seanbudd]

I think this is the correct divergence to make.
What do you think @feerrenrut ? Note earlier conversation from #14047 (comment)

[feerrenrut]

I can acknowledge this is inconsistent with the existing code, however the inconsistency is due to a change process around add-on APIs. We need to be able to delineate where we commit to a stable add-on API and where we don't. If there is no expectation of add-ons needing the new code, it is safest to mark it internal only. This allows it to change more easily without going through a deprecation process. If add-on authors identify the code as useful for their add-on, and want it exposed, then we can have a discussion around that and consider if it should be exposed directly or via some level of abstraction.

[seanbudd]

What do you mean by symmetry?

[tmthywynn8]

FYI, I don't know if this is off topic for this request, but it looks like there is a spelling error in the change log: responsivity maybe should be responsiveness?