[codeofdusk]

Link to issue number:

Fixes microsoft/terminal#5481. Blocking #10964.

Summary of the issue:

In an old implementation of NVDA's UIA console support, we originally used the standard _getTextLines implementation, which called getTextInChunks to get each line from the console according to UIA. This was extremely slow to run over the entire buffer, so we switched to getting all text and calling splitlines, which was much faster.

In pre-21H1 UIA console, every line ended with a newline character. In 21H1 console, new text can wrap across lines, so every line doesn't necessarily end in a newline. This makes splitlines inadequate for 21H1: when text wraps, NVDA starts reading from the beginning of the visible text for every new line of output!

Description of how this pull request fixes the issue:

This PR:

• Disables the _getTextLines override on the 21H1 UIA console. Pre-21H1 is entirely unaffected.
• Clarifies a comment in initialization at POSITION_LAST based on a conversation with @carlos-zamora.

Testing performed:

Re-tested the case in microsoft/terminal#5481 and verified that it is no longer reproducible.

Known issues with pull request:

None.

Change log entry:

None needed.

[codeofdusk]

Cc @feerrenrut, @michaelDCurran.

[codeofdusk]

I know we're really close to release, but if I may I'd like to merge this into 2020.3. This change has limited impact (only affects users who have enabled the UIA setting), is very unlikely to introduce regressions, and fixes a fairly serious bug.

@carlos-zamora and I are really hoping UIA will be good enough to become the default in 21H1 (see PR #10964), but we want thorough testing to avoid the need to revert as happened last time. Fixing UIA bugs as early as possible on the NVDA side will allow for more testing more easily before the deadline.

[josephsl]

Hi, I may need to look into this PR, but if it contains translatable strings, it cannot become part of 2020.3 as string freeze is in effect (I’m thinking it does not). Thanks.

[codeofdusk]

No string changes.

[Simon818]

I'd definitely like to see this in 2020.3 as well; I've been testing UIA console and have experienced the behavior described here.

[michaelDCurran]

Bounding the UIAConsoleTextInfo to the visible part of the buffer may have certainly removed some of the need for the override of _getTextLines, but I don't think we can state that removing _getTextLines would cause no performance degradation on pre 21h1 at all... as this still means NVDA now having to make roughly 272 cross-process calls (rather than 1) to split the text into lines. That was calculated on 30 lines, seeing that getTextInChunks makes about 9 calls per line to do the splitting.
Obviously for 21h1 and above we have no choice as we can't rely on newline chars being in the output anymore. But for pre 21h1, you really need to be sure that this does not cause a performance impact, or at least, don't remove it for these older builds.
I think, as we are talking about a release of Windows that is not out for a while yet, and because we are so late in our beta cycle, I wouldn't support this going into beta.
However, I'm perfectly happy for it to go to master, once 1 of the two possibilities above is done, that is prove there is no performance degradation for pre 20h1, or keep that code for pre 21h1.

[codeofdusk]

#11639 separately fixes this issue, as it works on the level of all the text (not line-by-line any more) and only requires one cross-process call (to get all text).

Would you be okay with me adding back the override to pre-21H1 for now (and merging that into beta), with an eye of removing it as part of #11639 on master? (I'd like to make the 21H1 console at least fully usable for people on 2020.3 if possible).

[michaelDCurran]

I'll let @feerrenrut have the final say here as release manager, but I am still uncomfortable with taking a change so late in the beta cycle for something that itself is not released for quite a while yet I'm sorry. People who use Windows insider builds can use master if they really need to, as they have already decided to live on the bleeding edge.

[codeofdusk]

I've updated the PR description to reflect the impact of this change: it now only affects 21H1 UIA console users.

[feerrenrut]

We won't take this for 2020.3, the RC is planned for today and we don't like to rush changes into a release unless there is a strong reason to do so. Given this will only impact people running insider builds of windows, those people can also run alpha builds of NVDA for this feature.

Please re-target this to master.

[codeofdusk]

Closing in favour of #11639.

[feerrenrut]

I really think this should be delivered independently of the DMP change if it can be?

[codeofdusk]

Since DMP would presumably be merged before the 2020.4 release, this only seems like it'd benefit users on master for a short window. That PR also fixes this issue as _getTextLines is no longer used at all (so no need for a separate fix here).

[feerrenrut]

That assumes that the DMP change does get merged into 2020.4, is there any harm in merging this first, on the off chance DMP doesn't make it?